Workflow

These are the rules we try to follow when working on Cockpit.

• No merge commits on master.

  See https://sandofsky.com/blog/git-workflow.html for the motivation. In brief, merge commits are confusing when rolling back history to find the commit that introduced a particular bug/feature.

  Unfortunately, github does not seem to agree to the points made in the article above. This means that we can't let github merge our pull requests.

• Each commit on master should be easy to read.

  Commits are for people to read, so try to tell the story of a new feature clearly. For example, refactor the code in a preparatory commit to make the actual change in the next commit easier to understand. Try to separate changes to separate pieces of the code base.

  Historical accuracy of how you figured out the final form of a change is ususally not very interesting, but it doesn't need to be totally hidden of course. If rewriting historical commits is tricky and has a high risk of introducing bugs, don't do it.

• Each commit on master should always pass make check and make check-memory.

  This is 'obvious' but can be subtle in practice. The tests behave differently depending on whether you run them as root or not and whether phantomjs is installed or not, for example. Also, changes outside of Cockpit can cause them to fail, of course. (Maybe glib introduced a new memory leak which our tests will find.) So this needs some common sense, as always.

  Travis runs these checks for pull requests and you shouldn't merge any pull request that is not "Travis Green". Travis will only check the tip of a pull request, so for extra perfection points, run "make check-memory" on each commit when you add your "Reviewed-by" line.

• The tip of master must always pass check-verify.

  Since check-verify takes so long and is so annoying to use, we only require the current tip to pass it, not each individual commit. This ensures that people always have a stable base for new work.

  Hubbot runs check-verify for each pull request. It rebases the pull request onto master and it will thus also find pull requests that need manual rebasing.

  Be careful when merging a pull request that isn't "Hubbot Green". The integration tests performed by check-verify are slow and brittle, and not all failures are caused by bugs in the pull request, but don't just blame every failure on the crappy tests.

  Hubbot only works on pull requests by a small number of explicitly trusted people. (Otherwise it would be too easy to run arbitrary code on our CI machines.) You can trigger hubbot manually for other pull requests with the trigger-hubbot script available from https://github.com/mvollmer/hubbot.

• Each commit on master should have been reviewed. (Almost each.)

  And have a Reviewed-by line at the bottom.

  The commits made during a release to bump the version number etc don't need to be formally reviewed.

• The subject of a commit should start with a short <topic>: prefix.

  This is usually the package name for frontend code, such as shell, base, or server-systemd, or some other suitable directory name. Check the existing commits for examples.

• If a commit fixes an issue, it should have a Fixes #NNN line.

  At the bottom, before the Reviewed-by line.

• Force pushing to master is allowed, BUT...

  ...write an email to [email protected] to explain what has been done and why.

  Don't force push master lightly of course. One reason would be to remove an accidental merge commit, or to quickly correct wrong or missing Closes or Fixes lines. When in doubt, don't force push master, close/reopen issues manually as needed, and live with the shame.

• The main cockpit-project/cockpit repository should not have any work-in-progress branches.

  Otherwise, there will be a huge number of these branches over time. We could delete them, but that throws away information, and people would still have them in their local clones.

  Instead, each developer (including the core developers) should make his/her own clone and submit pull requests from there. This makes it slightly harder to take over a pull request from another developer, but it can be done.

• A pull request with a WIP prefix in the name is not yet ready for serious review.

  You can make those pull requests to more visibly share some of your work with the rest of the team.

• A pull request that has been reviewed and needs action from the submitter has the needswork label.

  This includes changes to the code, or just replies to comments. Once you have done all that work, remove the needswork label again.

• A pull request is updated with copious rewrites and rebases until it has a small number of 'perfect' commits.

  These commits should be fit for master and the pull request is merged by rebasing these commits onto master.

  The Reviewed-by and Closes lines are added by the reviewer during the rebase.

• Review comments are made on the 'diff' of a pull request, not on its commits.

  Comments on commits disappear completely when those commits are rewritten. Comments on outdated diffs remain accessible, and are by default hidden.

• A temporary commit with a change for a specific review comment is added to a pull request with a FIXUP prefix in its subject.

  This might make the re-review easier. It's best to agree with the reviewer how to handle this in detail. Usually it doesn't matter much.

  Of course, these FIXUP commits need to be merged into the 'real' commits before the pull request can be merged into master.

• The top commit of a pull request should have a Closes #NNN line.

  At the bottom, before the Reviewed-by line.

  This is our usual way to signal to github that a pull request has been merged. Unfortunately, github labels them as "closed" which looks like they have been rejected.

• A pull request that depends on other pull requests declares that in its description.

  When the commits of a pull request sit on top of the commits of another pull request, it's not easy to see from github where one pull request ends and the other begins. Thus, it is import to note dependencies explicitly so that the reviewer is less likely to get confused.

• New test machine images are made via pull requests.

  [ This is work in progress, the following describes how it is supposed to work, but we haven't done this very much yet. So there might be bugs. ]

  The filesystem images used to create the test machines for check-verify do not change spontaneously. Thus, check-verify is reasonably isolated from changes in our base distributions.

  Of course, the test machine images should not get too old, and we need to change them when the dependencies of Cockpit change.

  To update them, make a pull request and change the 'tag' of the flavor/os combinations that you want to create new images for. The tags are defined in .../test/cockpit.conf and .../test/ipa.conf. You should use a tag that has never been used before. We don't have any fixed rules yet for how to choose a new tag, so this needs some coordination.

  When hubbot works on that pull request for the first time, new images will be created and published automatically, regardless of whether the tests pass or not. (Of course, when image creation fails, no images will be published.)

  Once an image for a given tag has been created, it should never be changed. If the image is wrong, you need to change the tag in the pull request again to trigger the creation of new images.