43

A colleague of mine told me that he is thinking in making our CI server to revert commits that failed the build, so the HEAD in master is always stable (as in passing the build at least).

Is this a best practice or it may be more problematic than just leaving master broken until the developer fixes it?

My thinking is that reverting the commit will make more complex the task of readding the commit and fix (developer will have to revert the revert and then commit the fix, which will also clutter the git log) and we should just leave the commit and then commit the fix. Although I see some advantages in having master stable, this revert of failing commits does not convince me.

edit: Doesn't matter if it is master or any other development branch, but the question stays the same: should the CI system revert a commit that failed the build?

another (lenghty) edit: Ok, we're using git in a strange way. We believe that the concept of branches goes against real CI, because committing to a branch isolates you from the other developers and their changes, and adds time when you have to reintegrate your branch and deal with possible conflicts. If everyone commits to master this conflicts are reduced to the minimum and every commit passes all tests.

Of course, this forces you to push only stable (or you break the build) and program more carefully to not break backwards compatibility or do feature-toggling when introducing new features.

There are tradeoffs when doing CI this or that way, but that is out of the scope of question (see related question for this). If you prefer, I may reword the question: a small team of developers work together in a feature branch. If one developer commits something that breaks the build for that branch, should the CI system revert the commit or not?

  • 38
    Failed builds should never have reached master to begin with. That's what development & feature branches are used for. Those changes go then in something like an integration branch where you can test if all new features of several developers will work together and only if this is tested in can go into master. Or at least that's one possible workflow. – thorsten müller Aug 21 '15 at 8:09
  • 1
    @thorstenmüller well imagine then that it is in a development branch that all developers use. Should the CI system revert commits that fail the build? – Carlos Campderrós Aug 21 '15 at 8:21
  • 7
    It seems you are using git in a strange way. Generally, people should be working on their own repos in their own branches, and only push to main once CI for their personal build verified that the changes are OK. – Wilbert Aug 21 '15 at 9:08
  • 4
    >"this conflicts are reduced to the minimum"; you get less conflicts at merge time, but you get issues with bad merges a lot more. The solution is to continually merge from master to your branch as part of your process, not to not branch. – deworde Aug 21 '15 at 15:44
  • 2
    ... Why not make it so failing builds don't get accepted into master to start with? – immibis Aug 22 '15 at 6:40
55

I would be against doing this for the following reasons:

  • Any time you set up an automated tool to change code on your behalf, there is the risk that it will get it wrong, or that a situation will arise where you need it to stop making that change (e.g., the latest version of Google Mock had a bug in it, so it's not your code failing) and you have to waste time reconfiguring it. Plus, there's always a slight risk that the build will fail because of a bug in the build system, rather than a bug in your code. For me, CI is about gaining confidence that my code is correct; this would merely turn it into another source of potential problems for me to worry about.

  • The kinds of bugs that break "the build" should be silly mistakes that take very little time to fix (as you've indicated in a comment, this is true for you). If more subtle and complicated bugs are regularly making it onto master, then the correct solution is not to "fix it faster", it's to be more careful when reviewing feature branches before they get merged.

  • Leaving master unbuildable for a few minutes while the bug gets fixed properly doesn't hurt anyone. It's not like the CEO will personally check out master and publish the code straight to clients at any random moment (at least, hopefully not without your involvement). In the highly unlikely event that you need to release something before you can fix the bug, then you can easily make the decision to revert manually before publishing.

  • 1
    Besides, only successful builds should trigger a creation of a "build drop" that could be deployed. If a build fails, there should be no deployable build drop, so there should be no risk that someone will publish bad code to clients. – Mark Freedman Aug 28 '15 at 11:47
  • 1
    there is definitely another option that is used and better than this one I think and in heavy use(we use it at twitter). don't put failed builds on master AND silly mistakes are easy to fix still as well. See my full answer below. – Dean Hiller Sep 30 '16 at 1:00
26

Let's agree on terms first.

I personally use the terms Continuous Build and Continuous Integration to distinguish two different scenarios:

  • Continuous Build: a tool that checks periodically if the repository changed since the last build, and build/test if it did.
  • Continuous Integration: a tool that takes Pull Requests and validate them against the latest head prior to making them visible.

The latter, Continuous Integration, means that the repository it protects is always green1: it is strictly better.

Your question only really makes sense for Continuous Build, so I will answer assuming this is your setup.

1: Environmental causes can also screw up a build, for example a test with a hard-coded year (2015) may start to fail January of 2016, a disk can get full, ... And of course there is the plague of unstable tests. I haughtily ignore those issues here; else we'll never get anywhere.


If you have a Continuous Build setup, you can indeed automate reversal of commits that may have broken the build, however there are several subtleties.

  • You cannot actually strip the commits: other co-workers might have already checked them out and will push them back in the next time they attempt to commit. Instead, a reversal should be committing a reverse diff. Oh, and co-workers will hate you for reverting their work when it was correct as they'll have to find a way to push it back again...
  • You cannot actually only strip the last commit (it's a merge), but need to strip all commits... up to a certain point. For example, the last known good commit (beware when boot-strapping the system).
  • You need to think about external causes (environmental issues), and avoid a setup that reverts everything to day 0. Thankfully, reverting to the last known good commit sidesteps this issue.
  • You need to think that the last known good build might no longer build (environment issues), in which case it is likely that all further commits will be reversed. Ideally, in case of failure, and before reverting, you would checkout the last known good build, and re-test it. If it passes, revert, otherwise, raise an alert.

Note that with this system, in case of an unstable test or a co-worker often committing crap, many good commits will get reversed. Your co-workers will then hate you.


Hopefully my horror tale has exposed the issues of allowing a broken repository and you will now implement a proper Continuous Integration pipeline where PR are never directly pushed to the repository but instead queued up for merging in a work queue, and integrated one at a time (or by roll-ups):

  • fetch head of repository locally
  • apply pull request(s)
  • build and test
  • in case of success, push to repository, otherwise mark as failing
  • move to next requests

Having attempted both, this is strictly better.

  • 2
    This is indeed the right answer -- the right solution is to prevent bad changes from ever reaching the master branch, not to let them land and then have to deal with rolling them back. – Daniel Pryden Aug 21 '15 at 22:35
  • I think the problem here is that the questioner believes that the stated costs of "isolating you from the other developers and their changes" outweigh the benefits. The stated costs are the increased danger of non-trivial merges the longer two people diverge. IMO the benefit of being isolated from broken code is obvious. The questioner wants to take an "optimistic" strategy, where broken code is briefly available in master to be pulled, and then fix this situation once the tests fail. Everyone else takes a "pessimistic" strategy as you advise, and only makes passing code available to pull. – Steve Jessop Aug 22 '15 at 12:44
  • (where by "available to pull", I mean "pull from master", which ideally is something developers can do willy-nilly, but to achieve that you must delay commits arriving in master before they're tested and pass. If a developer wants to cherry-pick untested or tested-and-failed code that's fine too, and the code is "available" in that sense, it's just not what I'm referring to) – Steve Jessop Aug 22 '15 at 12:47
  • The right solution is to prevent bad changes from ever reaching ANY branch. Failed commits should never be made public, ever. – Miles Rout Aug 26 '15 at 22:38
5

Is this a best practice or it may be more problematic than just leaving master broken until the developer fixes it?

It is problematic. A person deciding "the master HEAD is broken; I will revert the top change" is completely different than the CI system doing the same.

Here are a few disadvantages:

  • Errors in the automated reversal process will screw up the repository;

  • this assumes a single changeset (the topmost) screwed up the build (which is unrealistic)

  • Maintainers will have more work to do to fix the issue, than just investigation and commit (they will also have to look at reverse history)

We believe that the concept of branches goes against real CI, because committing to a branch isolates you from the other developers and their changes, and adds time when you have to reintegrate your branch and deal with possible conflicts.

This belief (branches vs. CI) is incorrect. Consider keeping one stable branch, where you commit only unit tested changesets. The rest (feature branches and local branches) should the responsibility of each developer and not part of your CI policy in any way.

In feature branches you want to be isolated from other developers. This allows you to:

  • perform exploratory coding

  • experiment with the code base

  • perform partial commits (effectively commit non-working code) to set up backup points (in case you screw up), to create more meaningfull change history (through commit messages), and to back up your work and switch completely to something else (in the time it takes you to write "git commit && git checkout ")

  • perform low-priority tasks that take a long time (e.g. you want to perform a refactoring that alters all 80 classes of the data layer: you change two per day, until you change all of them an the code compiles (but you can do this without affecting anyone until you can make a single commit).

If one developer commits something that breaks the build for that branch, should the CI system revert the commit or not?

It shouldn't. Committing stable code on your CI branch is a responsibility of the committer, not an automated system.

2

I would suggest using a Gerrit + Jenkins environment to keep your master branch always in a good shape. People push their new code to Gerrit which triggers a Jenkins job to pull that patch, builds, tests, and so on. If other developers like your patch and Jenkins completes its job successfully, then Gerrit will merge that piece of code to your master branch.

Its a similar environment described by @brian-vandenberg

Besides keeping your branch in a good shape you also add a code review step which improves code quality and knowledge sharing about your software.

[1] Jenkins https://jenkins-ci.org/

[2] Gerrit https://www.gerritcodereview.com/

1

The CI should never alter the commit history of the repo.

The correct solution here is for no commits to be added to master branch if they haven't been tested and verified.

Do you work on feature branches, have the CI run automatically on those, and if the builds fail, don't merge them into master.

You can have an additional build that tests merges if those are a concern, by running on the feature branch, and during the build merging master/integration/whatever into the local branch, then running tests.

  • 1
    This doesn't answer the question in any way. If the build fails in a feature branch, should the CI revert the commit? – Carlos Campderrós Aug 21 '15 at 13:19
  • What if the build succeeds on the feature branch, but fails after the merge? – Matthieu M. Aug 21 '15 at 13:19
  • @MatthieuM. merge is a commit, should the CI step that merges revert the build? – Carlos Campderrós Aug 21 '15 at 13:21
  • @CarlosCampderrós: I would personally never have a setup that attempts to revert commits; much too complicated. – Matthieu M. Aug 21 '15 at 13:25
  • I addressed the comments. – Daenyth Aug 21 '15 at 14:42
1

We use Jenkins for our build server and use the gatekeeper model for pushing commits -- where a combination of Jenkins and commit triggers (that ensure peer reviewers have done their job) is the gatekeeper.

Commits are pushed indirectly through a curl to Jenkins, where it clones the master repo then pulls in the commit(s) to be merged and performs all required builds (for Linux/solaris). If all builds complete, the commit gets pushed.

This avoids many if not all of the problems discussed so far:

  • history alteration
  • getting history correct if you're the dev who has to fix the breakage
  • instability (in the form of broken builds) is never introduced

It also allows us to directly enforce other requirements such as unit tests completing successfully.

  • re: the downvote, would you mind commenting on what you didn't like about my answer? – Brian Vandenberg Aug 24 '15 at 17:33
0

How many times have you gotten that automated email saying your last commit broke the build? How many times is it wrong? But now you have to go check to see if it really was you, or someone else who did another commit around the same time. Or maybe it was something environmental.

If the system doesn't know for sure, then I certainly don't want to automate it.

0

The question asked is flawed. I respect this statement though

"We believe that the concept of branches goes against real CI, because committing to a branch isolates you from the other developers and their changes"

What you should be doing though is these steps

  • work off master if you like(that is fine and keep pulling changes from everyone) BUT do NOT commit to master locally
  • JUST BEFORE you are going to commit your changes to master, create a branch with submit_XXXXXX
  • have your automated build pick up all submit_XXX branches build
  • Option 1: build breaks, or merge breaks...change rejected and it never lands on master
  • Option 2: build works, jenkins pushes master and updates it

THEN, what we do is put a git commit hook in preventing EVERYONE from actualy committing to master. It works great....NO broken builds ever and NO reverting commits from master either.

later, Dean

protected by gnat Aug 22 '15 at 10:45

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.