Lighthouse: Add stable/unstable branches, remove master

Created on 23 Nov 2020  Â·  10Comments  Â·  Source: sigp/lighthouse

Introduction

Currently, PRs are merged directly into master. This means that all PRs must have the full suite of CI run on them individually. We've had some bugs merged into master, which means we probably need to increase our CI coverage. At the same time, CI is running very slow and it would be great to run less CI on each individual CI.

This indicates to me that we should introduce a "staging" environment for code that we intend to push to users and should have a big fat CI suite run on it.

Proposal

I propose that instead of the single master branch, we have two long-living branches: stable and unstable.

The master branch naming is contentious and vague, we express ourselves more clearly with stable or unstable. It seems like a "tradition" to have a master branch, but I don't see what it gives us so I think we should axe it. It might be annoying to run git checkout master and not find anything, but I propose that's better than running git checkout master and not knowing if you're going to get production code or some PR I whipped up last night.

To achieve this, I propose:

  • Add a stable branch.
  • Rename master to unstable

    • I'm unsure if unstable or dev is better here. I'd take either.

  • Introduce the concept of a "release candidate", a release that has not yet passed the full suite of CI.

Note: I've chosen unstable as the default branch so we don't have to always change the PR base (and consequently forget to change it and break stable).

Workflows

Feature PRs

These are everyday PRs that add a feature, fix a bug, etc. These will run standard CI and aren't expected to have already run on testnets/canaries.

Process

  • git checkout unstable
  • git checkout -b my-feature-name
  • Create a PR which proposes to squash-merge my-feature-name into unstable.

Cutting a Release Candidate

A "release candidate" expresses the intention to make changes to master, triggering additional CI and other testing (e.g., running on testnets, canary nodes).

Process

  • git checkout unstable
  • git checkout -b my-release-branch
  • Create a PR which proposes to squash-merge my-release-branch into unstable.
  • After review and squash-merge, tag the commit with: git checkout unstable && git tag vx.x.x-rc1 (note the rc1 suffix).

Cutting a Release

A release is a change to master which has undergone enough testing for us to declare it is production ready.

  • Ensure all the -rc CI and checklists pass.
  • Create a PR that proposes to merge some commit from unstable into stable.
  • Remove -rc prefix from semver tag (e.g., v0.1.1-rc -> v0.1.1)
  • After review, merge the PR into stable.
    After review and squash-merge, tag the commit with: git checkout stable && git tag vx.x.x (no rc1 suffix).

CI/Testing

I think we should have two varieties of CI; minimal and full.

  • Minimal CI: runs on every PR and commit to unstable.
  • Full CI: runs on every time we add a tag to unstable.
  • Full CI: runs every time we add a tag to stable.

Build Server

I think we should have a build server which automatically produces binaries:

  • Whenever a tag is added to unstable or stable.

Open Questions

We haven't yet defined the difference between full/minimal CI, nor have we described the process to move a RC into a real release. I think we can start to work on these things once we have this PR structure started.

Inspiration

This is based upon @q9f's suggestions here which were in-turn based upon an Atlassian doc.

I've not fully adopted the git flow approach, since I think using a git-wrapper is not really ideal for a team of systems-programmers (no cotton-wool wrapping plz). I also don't see the benefits of enforcing strict branch naming with a fairly small engineering team.

Also, I've skipped the rc branch since I feel like we can just use PRs and tags to replicate it on unstable, ultimately reducing complexity.

A0 RFC

Most helpful comment

I think we should keep master and add a new dev branch at least until we are through the v1.0.0 release. Removing master right before users are going to be forced to update within a limited timeframe seems like adding an unnecessary source of confusion (however small).

Yeah, I've also been thinking this. Removing master and/or changing the default branch will potentially impact genesis validators.

How about this:

  1. Create stable/unstable
  2. Set stable to be default (for the time being, at least)
  3. Keep master and get it to track stable.

That way we give the master branch a little sunset period. This way people who aren't really considering if master is stable can keep using it (and get the stable stream). People who are wondering about stability can use the aptly named stable/unstable.

All 10 comments

I think this is reasonable, however I think we keep master and have a dev branch. So essentially your stable is master and unstable is dev.

I've seen this used in other repo's and it avoid the confusion to not having a master branch, when it seems to me that stable is actually a master branch.

Then only stable code gets merged to master, so there is no uncertainty when you pull master that you are getting some random PR.

I was thinking we should add a long-lived devbranch also.

I'm in favour of the two branch approach and I quite like the symmetry of the stable/unstable names. I can also see the argument for keeping master and adding a dev branch (user momentum behind git pull origin master, etc).

when it seems to me that stable is actually a master branch.

But, stable is only a "master" branch if you assume that "master" should be stable, which is unclear. For us, master pretty much unstable.

Perhaps another example is that it seems fine to have either:

  • master/dev
  • master/stable

This indicates to me that master is an ambiguous word and that we'd be better off using something more explicit.

avoid the confusion to not having a master branch,

I'm not really convinced it's that confusing to not have a master branch. When you git clone you'll get the default branch and you can run make without thinking about anything. If you're going to start playing around with branches it's probably useful if you're forced to go and understand which branch is stable and which one the devs are working on.

I would also suggest that the typical flow of git checkout master && git checkout my-feature is not what we want users to do anyway, they should be generally be doing git checkout unstable ...

In general, I just still don't see what value master has beyond following a tradition that repos have a branch named master which is probably the stable branch, but also might be something else entirely. Since Github has allowed changing the default branch and old tooling has been forced to handle a lack of master branch, I'd argue that it's a tradition that's dying rather than strengthening.

I was thinking we should add a long-lived dev branch also.

@AgeManning I'm not sure what you mean here? Maybe a typo?

Github allows changing the default branch, so we can name them as we see fit


I see two kinds of test suites and three workflows for this system:

Test suites

  • Standard: Focus on linting, ETH2 standards, and unit testing
  • Release: Focus on integration testing

Workflows

  • Feature
  • Release Candidate
  • Stable Release

Workflow - Feature

This is what most devs and contributors will do

  • git pull unstable
  • git checkout -b feature/my-feature
  • Implementation loop / developer magic 🦄
  • git push
  • Do a Pull Request
  • Standard suite of tests

    • If is a contributor, such suite runs in their repositories

    • If it is a @sigp member, runs in our box, Gazorpazorpâ„¢

  • Loop over tests or review observations
  • Lighthouse Team approves PR
  • Merge into unstable

    • Triggered with bors

Workflow - Release Candidate

TLDR: Like a feature, but we don't do implementation, and run more tests

These activities are to be executed by a team member wishing to cut a release

  • git pull unstable
  • git checkout -b release/my-release
  • git push
  • Do a Pull Request
  • Run the RELEASE suite of tests

    • Here we run every test on earth we can conceive:

    • The Standard suite plus integration, testnet, etc

  • Loop upon any situation found during testing
  • Team approves PR

    • Triggered with bors

    • Merge into unstable and tag the release candidate

    • git tag vx.x.x-rc1 (note the rc1 suffix)

    • Publish unstable binaries

Workflow - Stable Release

At this stage this is more an admin procedure, as we _already passed_ the RELEASE tests

Expect reports of the unstable binary from different actors in the ecosystem

Once it is decided a release candidate can become a stable release the steps are:

  • git pull vx.x.x-rc1
  • Create a PR that proposes to merge the above branch into stable
  • We already ran the RELEASE suite of tests for this commit

    • Also, as specified above, we collected feedback from ecosystem actors

  • Team approves PR

    • Triggered with bors

    • Merge into stable

    • Remove -rc prefix from semver tag (e.g., v0.1.1-rc -> v0.1.1)

    • Publish stable binaries

Build server

Just summarizing from the comments above

Standard Tests

  • For contributors

    • They will run Standard tests in their own repositories

  • @sigp members

    • They will run Standard tests in our box, Gazorpazorpâ„¢

    • Hooked to github

    • Provided they push a feature/.* branch

Release Tests

  • To be triggered by bors on release candidate workflow

Binary creation

  • To be triggered by bors on

    • Release candidate workflow

    • Release workflow

  • Binaries to be signed and published for other automated process to pick them up

This indicates to me that master is an ambiguous word and that we'd be better off using something more explicit.

I dont think so. I think if pick a random repo on github and pull master, I expect it to compile, run and work and be stable in the sense it has the latest code waiting to be added to a release. A more stable version would be to checkout one of its release tags. In fact with a lot of projects I actively assume this. The default branch doesn't necessarily indicate the stable branch. For example the default branch for the eth2-specs is the dev branch. But I know to checkout master to get the latest stable (without having to search for any documentation about how the repo is set up).

I think its just the naming here. I agree with everything else suggested.

@AgeManning I'm not sure what you mean here? Maybe a typo?

I was saying that I agree we should have another long-lived branch separate to master that we build alongside for things that are not yet stable.

I just think it will add confusion to users to not have a master branch. I understand we can change the default branch, but when every other project in the ecosystem has a master branch and external contribs generally know not to make commits to master for their PRs I think it will add confusion to not have a master branch.

I still don't quite get the argument for why we want to rename master and go against the current standard, there is no ambiguity in the traditional name master from my perspective.

If others think we should change it, happy to go along, just seems like it will add a lot of confusion for non-lighthouse people without any real benefit.

I think if pick a random repo on github and pull master, I expect it to compile, run and work and be stable in the sense it has the latest code waiting to be added to a release. A more stable version would be to checkout one of its release tags.

If I understand correctly, here you expect master to:

  • Be less stable than release tags.
  • Contain the latest PRs that haven't made it into a release.

This is the case for:

But I know to checkout master to get the latest stable

But here, I understand that you want master to:

  • Be as stable as release tags.
  • Only contain PRs that are considered stable.

This is the case for:

there is no ambiguity in the traditional name master from my perspective.

I think there's definitely ambiguity. From my reading, a stable-master vs unstable-master seems to be as varied as merge or squash-merge. If you go to eth2.0-spec and pull master you're going to get something that's behind dev. If you go to rust-lang you're going to get something that's ahead of stable.

Try Googling this: https://www.google.com/search?hl=en&q=stable%20master%20branch. There seems to be a lot of people out there unsure if master is stable and varied responses to them. At least one random person agrees with me (in a confused way):

"Ask the owner of the repository. "stable" might just mean "this is stable and intended for release" whereas master could mean "this has already been released". But again, you must ask the owner of the repository what meaning they attribute to the branches. There is no fixed answer that anyone can provide here because it depends"

Also take Afri's doc as an example, it has two strategies and one of the primary differences between them is whether or not master is stable.

Moving away from a master branch and having branch names that are less ambiguous is strongly recommended.

Having a stable/unstable or release/develop tuple is much more clear to potential contributors than master or main as mentioned earlier, different release strategies handle them differently.


Edit, my personal favorite is still the stable/beta/unstable triple but that requires a more involved strategy (e.g., as the rust compiler does).

I think we should keep master and add a new dev branch at least until we are through the v1.0.0 release. Removing master right before users are going to be forced to update within a limited timeframe seems like adding an unnecessary source of confusion (however small). Just adding dev and keeping master the default branch will minimize confusion. After v1.0.0, I think switching to stable and unstable is totally reasonable but I'm also fine with keeping master/dev.

I'm in favor of keeping master (or eventually stable) the default because it makes it clear what code we suggest people run. It will add a bit more overhead to people looking to contribute, but those people will be more familiar with git.

Note: I've chosen unstable as the default branch so we don't have to always change the PR base (and consequently forget to change it and break stable

Could we ensure we don't break stable by having CI fail for PRs to stable without -rc in the branch name?

I think we should keep master and add a new dev branch at least until we are through the v1.0.0 release. Removing master right before users are going to be forced to update within a limited timeframe seems like adding an unnecessary source of confusion (however small).

Yeah, I've also been thinking this. Removing master and/or changing the default branch will potentially impact genesis validators.

How about this:

  1. Create stable/unstable
  2. Set stable to be default (for the time being, at least)
  3. Keep master and get it to track stable.

That way we give the master branch a little sunset period. This way people who aren't really considering if master is stable can keep using it (and get the stable stream). People who are wondering about stability can use the aptly named stable/unstable.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

paulhauner picture paulhauner  Â·  5Comments

paulhauner picture paulhauner  Â·  4Comments

q9f picture q9f  Â·  4Comments

q9f picture q9f  Â·  4Comments

paulhauner picture paulhauner  Â·  3Comments