Jump to: navigation, search

Swift/feature branches

< Swift
Revision as of 17:59, 30 April 2018 by Notmyname (talk | contribs)
(diff) ← Older revision | Latest revision (diff) | Newer revision → (diff)

How and When Swift Uses Feature Branches

When do we use a feature branch?

A feature branch is used when you've got a long-term set of work that you want to eventually add to master in one step, but it will require collaboration and experimentation to get done. For example, adding storage policies to Swift needed a feature branch. It was a huge change that needed a lot of input from people throughout the community, but it wasn't clear what the final design or changes would be when we started. The feature branch allowed us to work together and experiment before discovering the solution that finally merged to master.

How to create a feature branch

Upstream feature branches can only be created by members of the openstack-infra team. The PTL should go to the -infra IRC channel and ask for it to be created. The PTL should have request the name of the feature branch and the specific SHA from which it should be committed.

Feature branches are always named in the form "feature/<name>". For example, Swift has used "feature/storage_policies", "feature/ec", "feature/crypto", and "feature/hummingbird". In this document, we'll use "feature/foo".

The name of the feature branch should be unique across all OpenStack projects. The CI system tests same-named branches against each other, so the feature branch must be unique across projects to ensure that cross-project integration tests still work. (However, if two projects are cooperating on a single feature, they may use the same branch name to test them together.)


Note: This process now is automated through the openstack/releases repo. http://git.openstack.org/cgit/openstack/releases/tree/README.rst

It's also a good idea to update the gerritbot reporting so your IRC channel is updated with activity on this branch. Update `openstack-infra/project-config/gerritbot/channels.yaml` and add both "feature/<name>" and "feature/<name>-review" to your IRC channel config. The "-review" one is handy for later. See the section below on merging a feature branch to master.

Life on a feature branch

Life on a feature branch is largely the same as life on the master branch. There are a couple of things we've found that are helpful.

  • frequently merge from master (only a core can do this)
  • allow experiments

Since the feature branch is long-lived, it's important to merge changes from master into the feature branch. Depending on the pace of development in master and the feature branch, this should happen every week or two, at the most.

Remember that the feature branch is for experiments. Some of the normal rules for merging to master may be relaxed (this is up to the whole team). Perhaps incomplete patches should be allowed to merge. Perhaps patches don't need full docs before landing. Perhaps migration/upgrade strategies don't need to be fully fleshed out before landing a patch. Perhaps you want to land stuff with only one +2.

Importantly, there will be code that lands on the feature branch that is never merged to master.

Experience strongly recommends that at least one core reviewer be actively involved in the day-to-day of the feature branch.

Merging the feature branch to master

After the feature being developed on feature/foo is complete and ready to be used, it should land on master. However, since feature/foo is full of experiments and a confusing history, it should be cleaned up before being presented to the wider community for review.

First, create a new feature branch called "feature/<name>-review". Using our example from above, we'd create "feature/foo-review". This review branch is for the code that will land on master. It will be a relatively small number of patches that are logical components of the overall whole. This is so that reviewers of the patch chain are able to easily grok the whole by understanding small parts at a time.

Designate one person (a core reviewer) to manage the patch chain on the review branch. Most likely, this will be a core reviewer who has been involved with the feature branch for a long time. This person will propose the patch chain to the review branch and collect each day's review comments. Every day, this person should push a new version of the whole patch chain, fixing things raised in review and dealing with any rebasing needed.

Experience strongly recommends that the docs for the feature be the very last patch in the review patch chain.

While the feature/foo-review branch is being reviewed, the PTL should impose a soft-freeze on master. This does two things. First, it avoids gratuitous merge conflicts with the feature review branch. Second, and most importantly, it forces the whole community to focus on the feature and gives a deadline to encourage people to do the work.

The soft-freeze and review work should be time-limited. Start with one or two weeks (max), depending on the scope of the feature being reviewed. At the end of that time, if the review branch isn't merged to master yet, reevaluated and extend the deadline as necessary.

Reviewers should review the review branch as normal. Most likely, the community will want more than the standard 2 +2s to land this though. If you've got a feature that is big enough to need a feature branch, you probably want more than 2 cores to approve it before landing it in master. Reviewers should not land (+A) patches on the review branch. Reviewers should never push over a patch on the review branch (let the person coordinating do it); instead, reviewers should leave diffs in review comments.

Once there is a consensus of reviews approving the patch chain on the review branch, the PTL should land the patches to the review branch. Then the PTL should create a single --no-ff merge commit to master. When this merge commit lands on master, you're done.

The most important thing

During development on a feature branch, and even more so during a feature-review process, the community must communicate with each other. IRC is great, but phone calls and in-person meetings are very helpful, if they are possible.

Summary

feature/foo

  • created when the work starts upstream (don't forget to update .gitreview as the first commit)
  • code, review, repeat
  • when functionally complete, go to foo-review
  • expect feature development to go slowly at first, and accelerate as more eyes get to it (especially in the final review phase)


feature/foo-review

  • create when the feature branch is pretty much done (don't forget to update .gitreview as the first commit)
  • propose a patch chain to it that has the final feature in a logical series of patches. could be as many as 20, but something closer to 5-8 is more reasonable and likely
  • soft freeze on master, everyone reviews the foo-review branch
  • PTL "plugs" the gate with a -2 on the first patch
  • reviewers add their reviews, as normal (just without any +A)
  • the patch chain manager will collect and apply reviews once a day
  • reviewers must not push over the patches, but leave diffs instead
  • when the whole patch chain has been thoroughly reviewed (likely looking for more than 2 +2s on each), then uncork it and land the chain on foo-review
  • PTL then proposes a merge patch (--no-ff) to master, bringing in the feature on master in one commit. the commit message contains the union of every author and co-author for the patch chain
  • PTL lands merge patch on master and master lifts soft-freeze
  • always put the docs as the last patch in the chain on the review branch