Testrunner code cleanup and refactoring

I ran several branch jobs thru using this code and they turned out the way I expected, with one exception. Upon running another job of that same type that passed as well.

Next will be for some contrib jobs, class jobs, etc.

Uploading a patch of this branch for dreditor code review purposes.

From what I see the following things are changing in the branch:

• Re-architected how build step processing runs, and adding “argument prioritization”.
• This is not fully implemented yet
• Added JobCodeBase
• Updating how error handling works for job buildsteps.
• Job Definition validation.
• Various code cleanup and renaming of variables.
• Renaming all the drupalci to DrupalCI stuff, renaming things like jobType, getBuildVar etc.
• It would be best to have variable renaming and such in a separate branch from structural/functional changes.
• Changing DCI_Variable names
• This can have impacts on the Jenkins jobs, as they set those environment variables

.
These changes do move us in a positive direction:

• Having the Codebase as an object with properties will be very useful.
• Encapsulating a JobRequest and separating out the JobDefinition is definitely the way we’d like to go, we can eliminate passing around arrays of data as arguments.
• Error handling can be centralized for a job

There are a few things I would like to see improved or help improve:

• Avoid array and string manipulation where possible for known patterns: Use pathinfo to get the extension of a file.
• Use a yaml parser to turn a yaml file into data.
• We should not rename variables to be less descriptive (avoid using a variable name like ‘$arg or $data’. Dockerphp made this mistake with a $data variable recently which caused the output stream to sever itself (https://github.com/ryanaslett/docker-php/pull/1)).
• Uncaught exceptions that are known should be caught in a try/catch block.
• Ideally we want to avoid using class methods. Output::error is something we want to get rid of, as we cannot test for it.
• Consistent coding standards (PSR2 or Drupal Coding standards) would help speed up review time.
• Creation of the job codebase, job definition, and build_id, etc. - I believe all of that should go into the constructor of the job, rather than the execute portion of RunCommand. I see those as all parts of the job, not parts of the run command. Using RunCommand spreads logic about what’s happening around, instead of encapsulating what’s going on with a job into one place.
• Wherever possible, smaller, compartmentalized branches for changes will make review much easier and faster. Many of the bullet points above could have been separate branches.

As we’ve talked about rather extensively outside this issue - we’d prefer to have input into new features, refactoring, changes in architectural direction before the time is put into making those changes. Your roadmap and vision for DrupalCI is not the same as our own roadmap. When there are changes of this magnitude it will take us some time to review them. With the current state of the codebase lacking tests we are very paranoid about deploying any code and are trying to ensure we’ve done thorough internal review and as much manual testing as we can.

Overall, this is code we can move forward with, and as mixologic outlined above it does some great things for us. However, with webchick’s expressed concerns about risking the stability of the test runner we need to be certain that doing so won’t be disruptive to the RC process or Barcelona sprints. Because it is not critical to RC1 it simply feels safer to merge after Barcelona. Ideally, we should be writing tests so that we can verify we don’t regress the codebase in refactoring.

Please understand that we’re hard focused on the priorities raised by the core maintainers- and the most important priority right now is stability. This is why we’re leery about any refactor or rearchitecture happening at this stage. Though it may be the ‘right’ way to solve some of the issues that are on our priority list, less invasive solutions are preferable wherever possible. If you will make an effort to include us in your thinking, we can and will make an effort to be more detailed and specific in our technical feedback. Mixologic’s comment is a good faith effort to do that and I hope it will be taken as such.

Since I'm being drawn up into this from my comments as a core maintainer...

I can't say whether the changes represented in that branch present risk or not. That's for you guys to work out. I just know we are severely under the gun over here to try and get to zero critical issues (it's currently 5) by Sunday the 13th, so Wednesday, Sep. 16 is available as a possible RC1 date. That's 65 hours from now, so even 1 hour caused by testbot instability is too much.

This deadline is important, because it is the only remaining release candidate window left prior to Barcelona, and meeting it makes the difference between Barcelona being a celebration of 4.5+ years of hard work by 3000+ people, or it being yet another bug fixing slog fest where dozens of already burnt-to-a-crisp people sit toiling away in a dark room at all hours while the rest of the attendees are out on a beach somewhere. And, pardon my French, but fuck that. :P

That said, it does make a lot of sense to have the code base in a place that will enable the best sprinting in Barcelona that is possible. You would have a window on Sep 17/18 which is a) after the RC1 deadline of Sep. 16 and b) prior to the extended Barcelona sprints, which start Saturday the 19th. The only caveat is key people might be traveling those days.

I understand this has been a bit challenging for all those involved. Everyone is working towards the same goal of making DrupalCI better.

@basic has committed to merging the refactor feature branch into dev while we continue making minor hotfix / bugfix releases to production (and dev) branches. He and I also took a bit of time this afternoon to update the Git branching for Drupal.org projects documentation. It will be best for everyone if we follow a process that roughly matches how the team interacts with code contributions for inclusion into a production service. We prefer small, incremental code changes—which is the whole idea behind continuous integration.

I'll check with the team and see how we feel about a merge on the 17th. Right now we are stable enough to provide the testing needed for the Con... which is an awesome milestone.

@jthorson, all of us truly and genuinely appreciate all of the time you put into keeping the old testbots alive, for the work you did to hand off those testbots when staff could pick up the maintenance and support, and for the work you've done to help get DrupalCI testbots into a state where the team could push that into a true production environment. That work was huge.

Your role on the DrupalCI should change a little now that we are well past the point that 90% of the work getting done at the Cons. Staff are carrying the pager (metaphorically, as there are all these great apps now) and getting the calls when the service is now. Our budget (thanks sponsors) is funding the environment (this level of CI stuff ain't cheap). It means we take the heat when bots crash or act up :)

Let's talk more at Barcelona face to face rather than relying on the issue queues for this.

I've merged this to dev. This should give the foundational branch for you to continue working on a syntax linting feature branch.

commit fa309f84d8e346f12743141eb24c5a2d7ca042b8
Merge: d95e2d9 ef5bbda
Author: Rudy Grigar <basic@drupal.org>
Date:   Thu Sep 10 20:07:01 2015 -0700

    Merge branch '2558929-jeremy_code_cleanup_and_refactor' into dev

dev has been merged into production, and production is live.