Workflow that enforces code review and keeps the integration branch pristine (git, Stash, TeamCity)

Question

I'm trying to design a new workflow that follows these principles:

• Only commits that have passed automatic verification (CI) should be merged into the mainline (specifically the merged state should pass CI to keep the integration branch as pristine as possible)
• Only commits that have passed human code review should be merged into the mainline
• Only commits that have passed automatic verification should be reviewed by another human (if it doesn't even build and pass tests, don't waste someone else's time on it)
• Feature branches should be covered by CI (independently, not merged into the integration branch - this is helpful for devs during development)

We are using git, Stash and TeamCity. This is what I have come up with, but none of them are perfect. I'm looking for tweaks to my suggestions, or new ideas.

WORKFLOW 1

• Developer develops on feature/VHPC-1 (feature branches are covered by normal CI in TeamCity)
• When the feature is done, the developer creates a pull request: feature/VHPC-1 --> mainline

• When the pull request is approved by another developer, it is automatically merged into the mainline by Stash (mainline is covered by normal CI in TeamCity)

• Problem: Stash will not perform the merge if there are merge conflicts, but the merged state is not tested at all until after the merge to mainline. We don't know if the mainline will break until it does.

WORKFLOW 2

• Developer develops on feature/VHPC-1 (feature branches are covered by normal CI in TeamCity)
• The developer pushes from feature/VHPC-1 to feature/VHPC-1/review (where we rebase the integration branch and then do CI (testing the merged state))
• When the feature is done, the developer creates a pull request: feature/VHPC-1/review --> mainline

• When the pull request is approved by another developer, it is automatically merged into the mainline by Stash (mainline is covered by normal CI in TeamCity)

• Problem: The time (20 min - 1 day?) between the merged state is tested and the integration happens allows changes in the integration branch which could mean the merged state no longer works. Chance that integration branch breaks.

• Problem: Twice the number of branches means some extra complexity and work.

WORKFLOW 3

• Developer develops on feature/VHPC-1 (feature branches are covered by normal CI in TeamCity)
• When the feature is done, the developer creates a pull request: feature/VHPC-1 --> feature/VHPC-1/review

• When another developer approves and merges the pull request, feature/VHPC-1/review will be automatically built and tested in the merged state, and automerged to mainline on success

• Problem: Twice the number of branches means some extra complexity and work.

• Problem: Commits must always be integrated into mainline, and only cherry-picked to release branches.

WORKFLOW 4

• Developer develops on feature/VHPC-1 (feature branches are covered by normal CI in TeamCity)
• When the feature is done, the developer creates a pull request: feature/VHPC-1 --> mainline TeamCity automatically adds itself a reviewer, and tries to build and test the pull request - in the merged state (using Stash's undocumented refs/pull-requests/merge)

• If the automatic verification passes, TeamCity will approve the pull request. A human may then review, approve and merge it into the mainline.

• Problem: TeamCity monitors refs/pull-requests/merge which changes when the integration branch changes, meaning a build would be triggered on all open pull requests when the integration branch gets one new change.

Answer 1

I would recommend Workflow 4 but with slight modifications

• Run CI on all feature branches whenever there is a check-in . Report the results to Stash using teamcity-stash plugin

• Enable Stash to do automatic merge whenever there is at least

  • x number of approvers
  • and y number of successful builds.
• Whenever a developer is ready , they can create pull request into mainline
• Run CI on all pull requests and report this status to teamcity . The advantage here is that whenever a pull request is approved git automatically merges that commit into other pull requests and hence re-triggers CI on all pull requests
• Once the reviewers and approvers approve the code and the pull requests have passed CI succesfully , stash automatically merges the pull request
• Run your build and test on every check-in into the integration branch so that you can pinpoint which check-in started failing any specific tests(if you have any tolerance limit for tests)

Answer 2

ex-Stash developer here. Just some general thoughts.

The Stash team (and most teams I've worked with and observed) do "optimistic" branch builds. That is they build the source branch under the assumption/hope that if it merges cleanly it probably won't break the mainline build. And in my experience this is almost always the case.

Some options:

1. Do nothing - if/when mainline does eventually break fix it quickly
2. Do nothing but auto-revert any merge commit that produces a red build (and possibly do it manually to start with)
3. Require that PR merges are fast-forward only, and developers need to manually rebase/merge from mainline before merging back
4. Add a custom gatekeeper plugin/button into Stash to all you to "Merge on green build after integration" which does a one-time merge build first before doing the actual merge.

I agree that in general building from Stash's hidden 'merge' ref, depending on how long your pull requests are open, is probably going to result in too many builds (as you pointed out).

Not sure if that helps?