Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ADR for branching strategy #4078

Merged
merged 5 commits into from
May 29, 2024

Conversation

rancher-max
Copy link
Contributor

Proposed Changes

This PR adds an ADR to propose changing the way we currently use branches here on Github, ultimately changing the release process to allow for more flexibility.

Types of Changes

Verification

Linked Issues

Further Comments

@brandond
Copy link
Member

brandond commented Mar 30, 2023

I like this idea in that it lets us land things during code freeze.

My primary concern about this is how it will affect tracking things on GH. Picking on 1.24 for example - if we PR all our backports into a dev-v1.24, and then merge dev-v1.24 into release-1.24 or simply fast-forward release-1.24 up to the head of dev-v1.24 after code freeze, GH won't show that. If we look at the backport PR, it will only show the merge into the dev branch - it'll break the link between the commit and the merged PR on the branch we use to cut our actual releases.

Switching back and forth trying to cross-link commits across branches is already challenging when trying to follow a commit to master back into the various release branches and I think this would make that worse.

@matttrach
Copy link
Contributor

When would the dev branches merge into the release branches?
I have a philosophical concern that this gets us further away from Continuous Integration, but I don't have any practical concerns to point out since we will just be continuously integrating into dev branches rather than release branches.

From a practical perspective, this reminds me of how KDM works, which causes a lot of pain in the release process.

## Consequences

- Allows for constant development and no code freeze.
- Any critical CVEs that would necessitate releasing again outside of the standard cadence can be added directly to the `release` branches and then ported back to the `dev` branches.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to avoid additional tagging? The code freeze would still exist on the release branches, but not the dev branches, right? So in the case of a critical CVE which requires an additional release, the release team should cherry-pick from the dev branch to get the fix. Then, after release, we rebase to fast forward the release branch to the dev branch? It kinda seems like we could achieve the same results just using tags, right? (it would rearrange history, but so does the branch solution)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like what you want to get closer to is trunk development with release trains, but we need to isolate changes using a feature flagging system to make that possible.
One simple way to implement this kind of isolation without rearranging our entire system is to commit commented out code. Then create a PR to uncomment the code after release. There are ways to make this easier (open/close principle is one), and other ways to implement the isolation, but this is the easiest that comes to mind.

@rancher-max
Copy link
Contributor Author

All the concerns noted here are very valid and are probably worth not accepting this, but I want to address them for the sake of communication.

@brandond

My primary concern about this is how it will affect tracking things on GH... Switching back and forth trying to cross-link commits across branches is already challenging when trying to follow a commit to master back into the various release branches and I think this would make that worse.

I agree. Changing the branching strategy here may necessitate a different backport strategy as well that makes cross-link irrelevant, but I don't know what that would look like or if it would make sense at all.

@matttrach

When would the dev branches merge into the release branches?

I think it should be as soon as "code freeze" would normally begin. The verbiage could be changed, but it would effectively operate the same as it is today where no "new features" are to find their way into the release branches, but bug changes found from testing could. To go back to Brad's point, this could make tracking very difficult because during this period, there could be a change needed on like release-v1.25, which then would need to be brought to dev-v1.25 as well (where potentially other new features have already been added since that branch is never frozen with this process).

Is this to avoid additional tagging? ... It kinda seems like we could achieve the same results just using tags, right? (it would rearrange history, but so does the branch solution)

The way I envision that is that the CVE changes would be made to the release branches themselves, then ported over to the dev branches. Of course this has its series of own complications. It might be possible to achieve the same results just using tags, yes!

It seems like what you want to get closer to is trunk development with release trains

It's hard to answer this... we're currently basically doing trunk-based dev. This just provides a way to continuously merge code and develop new features while allowing for a single stable trunk that can be released from for critical changes.


All in all, the overarching goal is to get to a place where:

  1. Releasing an r2 for critical CVEs or bugfixes can happen quickly and be isolated to ONLY those changes
  2. There never has to be a code-freeze that slows down development.

There is likely a better way to achieve those goals than what this ADR presents.

@rancher-max rancher-max force-pushed the branch-strategy-adr branch from 4fa37d2 to 3773ebb Compare May 16, 2024 19:02
@rancher-max rancher-max marked this pull request as ready for review May 23, 2024 19:39
@rancher-max rancher-max requested a review from a team as a code owner May 23, 2024 19:39
- Allows for constant development, with code freeze only relevant for the release branches.
- This requires maintaining one additional branch than the current workflow, which also means one additional issue.
- Testing would be more constant from the master branch.
- Minor release captain will have to cut the new branch as soon as they bring in that new minor version.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per slack discussion - pushing the new release branch should be the responsibility of the engineer that merges the PR bumping Kubernetes to the new minor. The new branch should diverge at the first commit using the new Kubernetes minor. Once the branch is ready for release, it can be handed off to the release captain.

Suggested change
- Minor release captain will have to cut the new branch as soon as they bring in that new minor version.
- Minor release captain will have to cut the new branch as soon as they bring in that new minor version.

@brandond
Copy link
Member

brandond commented May 24, 2024

yay codespell:

./developer-docs/updating_rke2_charts.md:22: consumin ==> consuming
Error: Process completed with exit code 6

@rancher-max
Copy link
Contributor Author

yay codespell:

./developer-docs/updating_rke2_charts.md:22: consumin ==> consuming
Error: Process completed with exit code 6

huh, I wonder how that got in in the first place since it's not touched here as part of this PR!

@brandond
Copy link
Member

codespell is externally updated and will randomly pick up new words. It doesn't have a full dictionary, just whatever stuff the maintainers add. It's quite irritating.

I fixed the issue in another already-merged PR, if you want to rebase.

@rancher-max rancher-max force-pushed the branch-strategy-adr branch from cfe9967 to f553623 Compare May 28, 2024 16:12
@rancher-max rancher-max merged commit e7c6468 into rancher:master May 29, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants