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

Brainstorming on Feature Branches #566

Closed
bgrant0607 opened this issue Apr 21, 2017 · 22 comments
Closed

Brainstorming on Feature Branches #566

bgrant0607 opened this issue Apr 21, 2017 · 22 comments
Labels
area/code-organization Issues or PRs related to kubernetes code organization lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/release Categorizes an issue or PR as relevant to SIG Release.

Comments

@bgrant0607
Copy link
Member

bgrant0607 commented Apr 21, 2017

Feature branches have been discussed as a possible way to stabilize both the master branch and releases.

http://nvie.com/posts/a-successful-git-branching-model/
https://github.com/nvie/gitflow
https://www.atlassian.com/git/tutorials/comparing-workflows
https://dev.acquia.com/blog/pragmatic-guide-branch-feature-git-branching-strategy
https://news.ycombinator.com/item?id=11190310

This issue is intended to be a home for discussion on that topic.

cc @idvoretskyi @thockin @jagosan

@jagosan
Copy link
Contributor

jagosan commented Apr 21, 2017

The branching model in Brian's first link, generally known as gitflow, is what I've seen work best.

Kubernetes currently runs on a quarterly release cadence. The he impact of this is that there is tremendous pressure to hit the release date. Corporate performance evaluations and personal satisfaction can hinge on hitting the deadline; the cost of missing the release because something isn't quite done is 3 months - too long for most. Worse, for PMs or organizations, if their feature is not prioritized for inclusion in the next release, their feature is at-best 6 months out (the release date for the following release). So multiple, large-scale features are merged to master in a very short period of time, some of which are not complete and may not be fully tested. As more contributors enter the fray, the interaction of various, possibly interdependent, features exacerbates the issue. It's virtually impossible for the first release to be fully functional, and it anecdotally it sounds like the 1.x.0 releases have generally had issues.

The other major issue is that master is continuously changing, so devs can become skittish about when to merge from master into local repos for fear that some partially-complete feature will stomp on their current work. The longer they wait, though, the worse it becomes. Most projects hit issues much earlier than kubernetes. I think it is a testament to the caliber and dedication of the engineers on the project that so much still gets done, and that releases have worked as well as they have for the past year or so. That said, I think it could be improved by adopting a branching strategy more conducive to the project complexities.

Feature branches help. Whatever branching model is used, keeping collaboration on large features in a separate branch until relatively well baked is helpful for anyone who isn't working on that feature - life can continue. The downside is that there is some more merging from master into the feature branch before then merging back to master... but it's generally a good thing. But feature branches on their own is not a complete answer.

The gitflow approach espouses a 'develop' branch which behaves the way it appears kubernetes master branch works today: code that compiles and is correct, but perhaps not complete is checked in frequently. In contrast, in gitflow the commitment is that master, always, always works; features are fully baked and fully tested. Culture or rules often restrict who can merge to master.

A note about feature branches: There is often more emphasis on feature branches than I think is necessary or useful. While trivial and near trivial changes can be committed directly to develop, feature branches can be extremely small and may be created just for local experimentation, or they can be much larger affairs, with contributions by multiple teams. The main benefit I've seen is that it keeps half-baked ideas from being checked into develop (or master) just for purposes of collaboration or remote backup.

release branches are also suggested in gitflow. This is often used as a finer grained mechanism for managing the time from feature complete to production-ready. New development can continue along as before in the develop branch (they'll catch the next train) while bug fixes are applied directly to the release branch. When the release branch is stable, it gets merged back to master.

@calebamiles
Copy link
Contributor

cc: @philips

@thockin
Copy link
Member

thockin commented Jul 17, 2017 via email

@bgrant0607
Copy link
Member Author

@thockin Development in a single branch means that there may never be a releasable branch.

I think the choices are repos or branches/forks. No new major feature development should happen in the kubernetes repo directly. We need to be able to gate half-baked, broken, untested, undocumented code.

@bgrant0607
Copy link
Member Author

How the Linux Kernel uses forks and branches:
http://blog.ffwll.ch/2017/08/github-why-cant-host-the-kernel.html

@k8s-github-robot k8s-github-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 15, 2017
@cblecker cblecker added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Aug 16, 2017
@k8s-github-robot k8s-github-robot removed the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Aug 16, 2017
@bgrant0607 bgrant0607 added the sig/release Categorizes an issue or PR as relevant to SIG Release. label Aug 21, 2017
@mattfarina
Copy link
Contributor

@bgrant0607 how long would you expect feature branches to live for? If they are long lived it could be more difficult to merge the changes in.

I've been a fan of gitflow and github flow having used them quite a bit. The danger has to do with development of long lived branches. Sometimes it's easier to put early features behind flags and iterate on a feature via multiple feature branches.

@bgrant0607
Copy link
Member Author

@mattfarina How long branches would live is TBD. I understand that branches increase the burden on contributors, but that is a necessary tradeoff at this point. I think the alternative is to apply a lot more rigor to every code review, require docs and tests to be written before the code, and other process changes that would also increase the burden on contributors, just in other ways.

I have seen feature flags used to good effect in other contexts, also, but it isn't clear to me how we could achieve the necessary level of rigor to make them work on this project. At minimum, before that approach would be viable, we'd need to radically improve the quality and coverage of our testing.

@idvoretskyi
Copy link
Member

idvoretskyi commented Oct 8, 2017

How long branches would live is TBD.

@mattfarina @bgrant0607 I'd suggest if feature branch lifecycle will be equal to the feature lifecycle itself - since the early design proposal stage until GA.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 10, 2018
@bgrant0607
Copy link
Member Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2018
@duglin
Copy link

duglin commented Feb 16, 2018

I think the alternative is to apply a lot more rigor to every code review, require docs and tests to be written before the code, and other process changes that would also increase the burden on contributors, just in other ways.

Big +1 - any change that go into master (or any official branch) should be fully baked, tested and documented - as if it were going to be released the next day. If they can't meet that criteria then it shouldn't be considered ready to be merged since the minute it goes in other PRs may then overlap it and backing it out could be a real challenge.

@davidopp
Copy link
Member

davidopp commented Apr 9, 2018

FWIW, a rant on (against) feature branches:
http://www.davefarley.net/?p=247

@jagosan
Copy link
Contributor

jagosan commented May 17, 2018

FYI - API Machinery is currently running an experiment on feature branch development for server-side apply here. The goals are to explore limits and issues with this approach in the main repo vs. in individual forks, especially regarding automation and testing. The broader goal is to explore working on something ambitious and far-reaching in a feature branch, protecting master from disruption along the way.

@fejta
Copy link
Contributor

fejta commented May 18, 2018

The docs team is also trying to use feature branches: kubernetes/test-infra#8058

@BenTheElder
Copy link
Member

@kubernetes/test-infra-maintainers we should keep up with the results here :-)
/cc

@bgrant0607 bgrant0607 added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jul 9, 2018
@quinton-hoole
Copy link
Contributor

Comments from a separate email thread:

Date: Fri, Oct 12, 2018 at 12:23 PM
Subject: Feature branches
To: kubernetes-sig-architecture kubernetes-sig-architecture@googlegroups.com

Hi sig-arch

Followup to yesterday's discussion (initiated by thockin) on merging incomplete features and feature branches as a possible solution to that. I'm not sure where the broader discussion on feature branches is happening, but while this is top of mind, let me dump some thoughts here in writing (feel free to redirect me to the appropriate other place if that exists):

  1. We've used feature branches internally to resolve this issue. In general feature branches work very well for us.
  2. There are two main failure modes we've seen, both of which are fairly easy to avoid:
    2.1 PR's to feature branches need to be as thoroughly reviewed as PR's to the master branch. So same reviewers, approvers etc apply. The only difference relative to master branch PR reviews is "incomplete is OK", whereas thockin's proposal for master branch PR's is "incomplete is not OK" (which I fully support).
    2.2 PR's to feature branches need to be treated as first class citizens, and not left to languish unreviewed and consequently unmerged, because they're "not to master". If they do languish, people won't use feature branches. Clearly just before a release of master, review bandwidth might be more focussed on master branch PR's, but this should not be the general case all the time.

Assuming that the above process is followed, the bulk of the code review effort happens in the feature branch. The master branch PR review (when the feature branch is merged into the master branch) becomes substantially simpler, almost trivial, boiling down to:

  1. Has all of the code already been appropriately reviewed and approved in the feature branch? Defaults to yes, if the feature branch is managed in the normal way.
  2. Is the feature complete? Defaults to yes, as above. PR's against master are only generated once the feature has been reviewed and deemed to be complete in the feature branch.

Hope this helps...

Q

@quinton-hoole
Copy link
Contributor

One more point regarding keeping the feature branches in sync with master (i.e. periodically rebasing against master):

  1. The primary cause of rebase headaches is merge conflicts.
  2. Merge conflicts occur when two people edit the same chunk of code (inevitably the same file, and often the same method).
  3. The above is typically (but not always) a sign of poor code factoring.

So to avoid rebase headaches:

  1. Make sure that the vast majority of your feature code additions are in separate files, or at the very least separate methods. Strict plugins are one good way of achieving this, but there are other good ways too.
  2. If the above is impossible or terribly hard, it probably means that the existing code in master/HEAD is badly factored. To make your life easier, "prefactor" (to use @thockin 's term, which I like) the code in master/HEAD.

In summary, if rebasing against master/HEAD a few times a week results in large numbers of merge conflicts, you're probably doing things wrong.

@apelisse
Copy link
Member

apelisse commented Apr 4, 2019

Sharing thoughts about the feature-branch in the context of server-side apply: https://docs.google.com/document/d/1l-y_rgEpktXcWjCle7intCVKh7cAxhCHv735wLz1UwY/ (shared with kubernetes-dev@)

@dims dims added the area/code-organization Issues or PRs related to kubernetes code organization label Apr 14, 2019
@justaugustus justaugustus added this to Backlog in SIG Release Apr 24, 2019
@mrbobbytables
Copy link
Member

Is this still relevant? It's been open and frozen for some time now.
I'm going to mark it as rotten for now, if it should be left open please update or re-freeze it 👍

/remove-lifecycle frozen
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels May 18, 2020
@mrbobbytables
Copy link
Member

Woops, was hitting the contribex frozen issues and hit this one as well. I'll re-apply the freeze.
/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels May 18, 2020
@dims
Copy link
Member

dims commented Mar 11, 2021

sent a final email out to sig-arch and sig-api-machinery:
https://groups.google.com/g/kubernetes-sig-api-machinery/c/TTUxPiynt1I/m/G83OiOrTAwAJ

/close

@k8s-ci-robot
Copy link
Contributor

@dims: Closing this issue.

In response to this:

sent a final email out to sig-arch and sig-api-machinery:
https://groups.google.com/g/kubernetes-sig-api-machinery/c/TTUxPiynt1I/m/G83OiOrTAwAJ

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@LappleApple LappleApple moved this from Backlog to Done/Closed (1.21) in SIG Release Mar 11, 2021
@LappleApple LappleApple moved this from Backlog to Completed in code-organization subproject Mar 12, 2021
danehans pushed a commit to danehans/community that referenced this issue Jul 18, 2023
* Add test triagers

* go mod tidy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/code-organization Issues or PRs related to kubernetes code organization lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/release Categorizes an issue or PR as relevant to SIG Release.
Projects
SIG Release
  
Done/Closed
Development

No branches or pull requests