Wile I disagree with skipping the code reviews, one main advantage of the feature flag part of the strategy is you don't have to constantly rebase and your code goes through a production ready code review. Feature flags may not feel right, but they minimize risk spectacularly, which is the ultimate goal that any solution should put first. The use of the word "feature" is misleading to me and makes it sound like they should be used for new features only and not changes or rewrites of brittle code.
Plus the whole feature flag "solutions" and "tooling" people complain about is wrong. Once that you are forced to put your (small) changes to production all the time, you will start thinking about backwards compatibility in a whole new way.
It's not about feature flags, it's about shipping constantly and knowing after each step if you've made a mistake or not.
To me personally, this method is a huge pill against headaches.
Here are a couple of ways in which feature flags can be better than branches: QA can try in-development features out together easily. Some flags can be used operationally to turn off non-essential features during periods of high load (for a service, that is).
There's no free lunch, but flags can be a useful tool.
The fun starts when you have a multi tenant system, and you end up with some users having the flags enabled and some not having them enabled. You may find that you have to support both "branches" in the long run. Which can be a good thing in the sense your product is more configurable, but also entails a lot of the same grievances of branching because you end up supporting multiple variations of a feature. Or multiple combinations thereof
Basically they took the easy way out of just pushing code to the master and letting the consumers worry about consequences instead of fixing the actual problem (which was long living branches caused by other underlying issues with general processes). Not really a huge fan of that.
Nothing prevents you from having a constantly synced "master" branch and a carefully curated "release" branch. Nothing, except the effort required for curating.
But between that and the pain of having several long-running parallel dev branches that tend to diverge, cross-pollinate in weird ways, have hard time merging to the production branch, etc, my choice is obvious.
As explained in the article: Because experimentally, "smaller diffs" cause people to nitpick on pointless things like whitespace instead of performance, features, and future-proofing the architecture (i.e. the fucking point).
This means code review process is completely wrong. All these "nitpicks" should be automated by linters and code review should be more about implementation, structure, performance, etc..
Yes, it does mean that code review is completely wrong.
If you read the article, they suggest strongly that it is hard to get a "correct" code review process (perhaps because it encourages the nitpickers, or perhaps for other reasons).
If you've got a bunch of experienced people spending a year on it that can't solve it, perhaps they just can't solve it.
And at that point: What's the difference between something that's wrong, and something that they can't do right?
Most review processes are wrong. Sometimes because it rubber stamp everything, other times because the most aggressive one is not the most knowledgeable one and yet other times cause they turn into competition of who is more petty.
Good code review is hard. Catching non petty problems is much harder then lengthy obsessing over function names or 'if' vs '?' or whether slightly more or slightly less abstraction or whether two 6 line long function vs one 12 lines long.
> Skipping code reviews for faster pushes to "trunk" does not sound like a stable solution.
I think a important context here is that this is engine code (a.k.a. library code).
This isn't a web service or web site where what you ship is hitting end users. The end users of this code produce new products, and the potential issues are a) that the users of the engine (i.e. game studios, internal users of Stingray etc) are being held up in their work, or b) that bugs sneak through to their users (the gamers/end users).
In this context, the idea of a quicker turnaround in exchange for some bugs leaking through is much easier to defend. The studio might want the buggy code faster rather than the fixed code later. This is probably not the case for a system deployed directly to end users.
> Why not just encourage faster reviews and smaller diffs..
That was the motivation of switching to trunk based (i.e. removinbg the incentive and slowness of larger PR's). You can't "encourage" smaller diffs other than having the process inherently do that. It's not "encouraging" to send an email to the team telling them to make smaller diffs.
In particular, this rings true to every complex source control process I've used that isn't actually enforced with software:
You could of course say that all of these mistakes are the result of human error, and if the same people just read the documentation and learned from their experiences, everything would be fine. But I have seen these mistakes being made so many times, by otherwise competent developers, that I don't buy this argument. There's a saying I love: "If one person tells you you have a tail, ignore them; if a hundred people tell you that, look behind you". If these mistakes happen over and over again (and I can confirm based on my experience that they do), made by different people in different circumstances, then there's no rational alternative to admitting there has to be something fundamentally flawed about the method being used.
So... It seems like, from the list of grievances, all they really needed to do was get rid of pull requests and just allow people to merge. I never understood pull requests vs merges to be a required part of Gitflow. Atlassian seems to agree with me:
"After adding a few commits, Mary decides her feature is ready. If her team is using pull requests, this would be an appropriate time to open one asking to merge her feature into develop. Otherwise, she can merge it into her local develop and push it to the central repository..."
We tried GitFlow for a while but it became hell on Earth to manage it. So we came to this alternative, which is way simpler and is working like a charm. I've made a post about how to implement and use it here: https://hackernoon.com/still-using-gitflow-what-about-a-simp...
The first job I ever had, I had brutal code reviews from a great developer. I never enjoyed them, but appreciate them now.
I haven't had code reviews since, until my new job. Where every PR is supposed to be code reviewed. I really enjoy the idea that someone will find problems in my code and suggest better ways to do it so I can further develop my skills. I also spend lots of time coaching junior developers, and like that I can review their PR's to see if my coaching is helping, or if they still need help with concepts.
Writing solid code bases without code reviews seems like a fantasy. Deferring code reviews until a huge new branch is complete and expecting a busy engineer to review the whole thing, and then suggest improvements, and have the original developer then decide to rewrite the entire branch to incorporate them, also seems like a fantasy.
I don't understand how people find GitFlow to be complex or slows them down. If feature branch is long lasting just merge regularly from develop branch so you don't end up with a tough merge when the feature is finished. Apart from that, just trust your devs. People are not stupid, gitflow process is sane and simple, nobody will start pushing to master like crazy if you allow merges without pull reviews. If you dont trust the devs to do the simple tasks right, do you trust anybody in your organisation? Do you check every step/change your devops/sysadmin/dbadmin make?
The problem with GitFlow is that it takes a fairly common and sane master+feature/bugfix-branches approach and makes it needlessly complicated with the whole develop+master branch split and some dubious recommendations like 'git merge --no-ff myfeature'.
Do you have any further details on this? The tabs always looked weird to me but I don't know why.
We practice "gitflow" at my work, except we deviate from it to fit or workflow better. We have feature/fix branches and a master branch that we all use on our dev machines every day, we never ship our master branch though, we ship versioned releases which get backplate of bug fixes as needed. We have hundreds of clients on their own clusters and we stagger new releases to the clusters in order of customer priority. We try to ensure PRs are well tested before merging, bit of minor issues crop up on master we file a bug and sprint it ensure it is fixed before the next release (and generally by whoever did the initial work that caused the issue).
But preserving merge commits isn't a dubious recommendation, it's just about creating a consistent history. One could consider it a stylistic choice but, at least for me, it does make the history more readable.
Combined with squashing and rebasing git flow leaves a very nice history... I can imagine if you're slamming a bunch of uncurated commit messages along with lots of merges you're gonna see a chaotic history.
A consistent history is a must-have with a distributed team, IMO. GitFlow's tooling is nice in that context to enforce consistent operations across environments and teams where not everyone is 100% perfect with Git.
I wonder if solution to `PR commentary is lost.` would be simply to dump all the information created during code-review into some metadata in the commit itself. We also have this problem at RhodeCode that basically code-review in pull request puts lots of valuable information into the code review tool rather then the source code itself.
"No shared view of the product. Since everyone is working on a long feature branch of their own, there is no consensus view of the state of the product. Managers can’t look at it and get a feeling for what is going on. Features get no “free” QA testing from other developers, because only the developer working on that branch will see them. It is only when everything is integrated (in a scramble, just before the release) that the true status of the product is known."
This "free" QA testing is NOT free. It is in fact what branching is meant to remove. You don't want to mix your dev's development and integration work, they are completely different processes.
What you need is your QA team(or your devs with their QA hats on) actively merging all "ready" branches into an "integration" branch and testing that. You need people to be able to signal "yeah, my branch is stable enough to be merged for integration testing" or "no, don't merge my broken branch that I'm still working on, you'll just waste your time". Incidentally, that's what a pull request is.
The integration branch is mentioned in gitflow but not really expanded upon. I agree with the author that the way they present master/develop, there's not much benefit to develop. However, you need a branch where you're continuously integrating(cough).
This is "extra work" that you need to do, but there's no way around it. You can "do it for free", essentially forcing every dev to be a QA at the worst possible time, disrupting their own workflow, or you can separate it as its own workflow. Whatever works for your team.
I read Gitflow differently. Feature branches should be merged to develop as soon as possible. This lets others integrate the changes into their features, current and future, and build upon it instead of having a single multimerge from hell later. [0]
Release branches are where you do integration testing of all the merged features. If you have fixes, you merge them into the release branch. Then, once it's OK'd for release, you merge those patches down to develop. Note that this is also exactly how hotfixes work, as they're just a different type of release.
[0] And yes, you will find bugs in other people's code... I don't think anyone is claiming that source control processes can fix that. And if they are, I will call them a silly goose.
Interesting article. There's really no best way to do this and it depends on the project and team in my opinion. How big is the team? How experienced is the team? How likely is it developers working on separate features are going to cause merge conflicts? How painful are merges? How big an impact is it if a bug gets on to production?
My belief is that it's long-lived branches that are the problem, since trying to keep diverging code-bases in synch is a huge source of pain. Switching to branchless development is probably an overreaction, though.
I'm no proponent of long-lived branches, as such, but isn't holding branches like that in synch that exactly what `git rebase` is for?
It's all product specific, naturally, but I've had occasions where isolated multi-month branches made sense... They got rebased before merging, though, so all refactoring and merging pain was taken on the side of the divergent branch and the post-squash post-merge history was really clean. Even major code-base wide refactorings should be trivial to incorporate as long as you're not working at distinct cross-purposes with other people (which is a planning issue).
It sounds like encouraging regular rebasing of diverging branches would be the solution here.
Yeah, but that's work that people have to remember to keep doing, and if they fall behind, the pain gets worse and worse when they do finally have to merge. Some teams seem to do fine with this, but I've seen it go pretty badly, too.
I don't understand how any of the problems mentioned are related to git flow (or even to feature branches, which is what they're really complaining about)
And I don't see how they're any better off all committing to master
eg
> Merge problems become hairier and hairier. With everybody working on a long-lived branch, there will be multiple conflicts once they eventually merge. Large scale refactors are discouraged because they will “break everybody’s branches”.
How is that different if everyone commits to master? As soon soon as someone does a large-scale refactor you'll break everyone's "master" and have merge conflicts. It's no different.
Their real problem is not knowing how to manage a team of developers, making huge PRs etc etc
The secret there is to not do large-scale refactors, but break them up into smaller units of work. The author also mentions that big changes happen behind feature flags, to enable parallel development.
Feature toggles work great most of the time. But it's kind of hard to put a feature toggle on a refactoring. Sometimes the new features involve refactoring shared code that was otherwise stable.
I use gitflow because it helps split up my development work from others on my team that need a code base that Just Works (tm). I want them to be able to checkout master and have no issues. From this point of view, my colleagues are also my customers, and I use master as a release branch - this is beyond all else, most important for me.
Further, it makes sense to have separate 'feature' branches coming off of develop that can be experimental in nature and scrapped at any time instead of rewinding the develop history to a stable point.
EDIT: I mean to split it up for my colleagues because they are not in primarily code-related functions and don't need to be editing the source.
PRs on my team are done at the request of the author, who maintains full responsibility for the code. When review is complete, the author merges when they are ready (they have full access rights). The author has the right to ignore whatever feedback they want, since they will share the burden of whatever bugs are introduced. However, merging bugs knowingly and/or ignoring feedback is frowned upon.
Reviewers are expected to provide whatever type of review the author requests, including manual testing and review of system impacts. Their comments do not require a response from the author, unless they ask questions. It is frowned upon to comment too much on code formatting.
In practice, our developers submit PRs for just about everything. Most PRs are small, and most feedback is addressed. Reviewers are thanked and given kudos for finding issues. Knowledge is shared, and the codebase is improved. I’d estimate that 90% of our bugs are caught in review.
Product quality has improved drastically since we implemented this practice. Developers consistently state that our PR process is their favorite part of the job. They get to share their work, get feedback, and become better developers.
I've had the same positive experience working like that in a small team.
In addition, I shared pre-commit git hooks set up to run a few tools that a) enforced the team's code format and documentation standard; and b) performed static code analysis. This mostly prevented code reviews from devoting time to code format and basic documentation issues.
> It is frowned upon to comment too much on code formatting.
Do you have standards for those though? Like gofmt, prettier, ide formatting configs, etc? I and many others can be rather ocd about maintaining a consistent code style, although I will admit that it's far too easy to focus on that in code reviews (low-hanging fruit). Which is why it should be automatically enforced by tooling so it isn't a distraction during reviews.
Yes, we try to follow industry standards for code formatting as much as possible, but we also try not to be too picky about formatting if it doesn't meet those standards exactly. Usually authors will make the same formatting mistakes multiple times, so rather than comment on each and every badly formatted line, we try to comment on the type of mistake once and note that there are other instances in the same PR.
I agree that auto-formatting is the way to go. The big thing holding us back from using those tools is that we'd first need to reformat the existing code, otherwise the we'd wind up with a lot of unrelated formatting changes in our PRs (assuming that whole files are formatted when saved, rather than just new lines). Certainly not an impossible feat, just something we haven't done yet.
52 comments
[ 4.7 ms ] story [ 39.7 ms ] threadIt's not about feature flags, it's about shipping constantly and knowing after each step if you've made a mistake or not.
To me personally, this method is a huge pill against headaches.
There's no free lunch, but flags can be a useful tool.
https://martinfowler.com/articles/feature-toggles.html
I've been in that multi tenant hell, and concur that it sucks.
But between that and the pain of having several long-running parallel dev branches that tend to diverge, cross-pollinate in weird ways, have hard time merging to the production branch, etc, my choice is obvious.
If you read the article, they suggest strongly that it is hard to get a "correct" code review process (perhaps because it encourages the nitpickers, or perhaps for other reasons).
If you've got a bunch of experienced people spending a year on it that can't solve it, perhaps they just can't solve it.
And at that point: What's the difference between something that's wrong, and something that they can't do right?
Good code review is hard. Catching non petty problems is much harder then lengthy obsessing over function names or 'if' vs '?' or whether slightly more or slightly less abstraction or whether two 6 line long function vs one 12 lines long.
> Code review happens through a small window. When reviewing a PR you only look at the fraction of the code that just changed.
Their complaint is that code review makes it easy to miss deviations from global goals & style, not that they nitpick minor presentation issues.
Although I wonder what sftware they are using that would only show them a small fraction of a change…
I.e. all of the code that changed, which is a fraction of the total codebase.
I think a important context here is that this is engine code (a.k.a. library code).
This isn't a web service or web site where what you ship is hitting end users. The end users of this code produce new products, and the potential issues are a) that the users of the engine (i.e. game studios, internal users of Stingray etc) are being held up in their work, or b) that bugs sneak through to their users (the gamers/end users).
In this context, the idea of a quicker turnaround in exchange for some bugs leaking through is much easier to defend. The studio might want the buggy code faster rather than the fixed code later. This is probably not the case for a system deployed directly to end users.
> Why not just encourage faster reviews and smaller diffs..
That was the motivation of switching to trunk based (i.e. removinbg the incentive and slowness of larger PR's). You can't "encourage" smaller diffs other than having the process inherently do that. It's not "encouraging" to send an email to the team telling them to make smaller diffs.
In particular, this rings true to every complex source control process I've used that isn't actually enforced with software:
You could of course say that all of these mistakes are the result of human error, and if the same people just read the documentation and learned from their experiences, everything would be fine. But I have seen these mistakes being made so many times, by otherwise competent developers, that I don't buy this argument. There's a saying I love: "If one person tells you you have a tail, ignore them; if a hundred people tell you that, look behind you". If these mistakes happen over and over again (and I can confirm based on my experience that they do), made by different people in different circumstances, then there's no rational alternative to admitting there has to be something fundamentally flawed about the method being used.
https://www.atlassian.com/git/tutorials/comparing-workflows#...
"After adding a few commits, Mary decides her feature is ready. If her team is using pull requests, this would be an appropriate time to open one asking to merge her feature into develop. Otherwise, she can merge it into her local develop and push it to the central repository..."
They have huge organizational problems.
Relevant link, Conway's law
https://en.wikipedia.org/wiki/Conway%27s_law
They were a small shop and they are now part of Autodesk. Autodesk is a big shop. Hence problems. Hence article.
Edit: they were a small shop and then were aquired by Autodesk (they have since moved on).
http://ourmachinery.com
Updated my previous post: should be past tense in both cases.
Yowsa! This sounds like developers who get code reviews write bad code…
I haven't had code reviews since, until my new job. Where every PR is supposed to be code reviewed. I really enjoy the idea that someone will find problems in my code and suggest better ways to do it so I can further develop my skills. I also spend lots of time coaching junior developers, and like that I can review their PR's to see if my coaching is helping, or if they still need help with concepts.
Writing solid code bases without code reviews seems like a fantasy. Deferring code reviews until a huge new branch is complete and expecting a busy engineer to review the whole thing, and then suggest improvements, and have the original developer then decide to rewrite the entire branch to incorporate them, also seems like a fantasy.
We practice "gitflow" at my work, except we deviate from it to fit or workflow better. We have feature/fix branches and a master branch that we all use on our dev machines every day, we never ship our master branch though, we ship versioned releases which get backplate of bug fixes as needed. We have hundreds of clients on their own clusters and we stagger new releases to the clusters in order of customer priority. We try to ensure PRs are well tested before merging, bit of minor issues crop up on master we file a bug and sprint it ensure it is fixed before the next release (and generally by whoever did the initial work that caused the issue).
A consistent history is a must-have with a distributed team, IMO. GitFlow's tooling is nice in that context to enforce consistent operations across environments and teams where not everyone is 100% perfect with Git.
Interesting idea to try out for us
This "free" QA testing is NOT free. It is in fact what branching is meant to remove. You don't want to mix your dev's development and integration work, they are completely different processes.
What you need is your QA team(or your devs with their QA hats on) actively merging all "ready" branches into an "integration" branch and testing that. You need people to be able to signal "yeah, my branch is stable enough to be merged for integration testing" or "no, don't merge my broken branch that I'm still working on, you'll just waste your time". Incidentally, that's what a pull request is.
The integration branch is mentioned in gitflow but not really expanded upon. I agree with the author that the way they present master/develop, there's not much benefit to develop. However, you need a branch where you're continuously integrating(cough).
This is "extra work" that you need to do, but there's no way around it. You can "do it for free", essentially forcing every dev to be a QA at the worst possible time, disrupting their own workflow, or you can separate it as its own workflow. Whatever works for your team.
Release branches are where you do integration testing of all the merged features. If you have fixes, you merge them into the release branch. Then, once it's OK'd for release, you merge those patches down to develop. Note that this is also exactly how hotfixes work, as they're just a different type of release.
[0] And yes, you will find bugs in other people's code... I don't think anyone is claiming that source control processes can fix that. And if they are, I will call them a silly goose.
If you are deploying code multiple times a day then it only results in a bunch of meaningless merge commits
It's all product specific, naturally, but I've had occasions where isolated multi-month branches made sense... They got rebased before merging, though, so all refactoring and merging pain was taken on the side of the divergent branch and the post-squash post-merge history was really clean. Even major code-base wide refactorings should be trivial to incorporate as long as you're not working at distinct cross-purposes with other people (which is a planning issue).
It sounds like encouraging regular rebasing of diverging branches would be the solution here.
And I don't see how they're any better off all committing to master
eg
> Merge problems become hairier and hairier. With everybody working on a long-lived branch, there will be multiple conflicts once they eventually merge. Large scale refactors are discouraged because they will “break everybody’s branches”.
How is that different if everyone commits to master? As soon soon as someone does a large-scale refactor you'll break everyone's "master" and have merge conflicts. It's no different.
Their real problem is not knowing how to manage a team of developers, making huge PRs etc etc
Gitflow vs trunk-style is a kneecap kind of decision.
Further, it makes sense to have separate 'feature' branches coming off of develop that can be experimental in nature and scrapped at any time instead of rewinding the develop history to a stable point.
EDIT: I mean to split it up for my colleagues because they are not in primarily code-related functions and don't need to be editing the source.
Reviewers are expected to provide whatever type of review the author requests, including manual testing and review of system impacts. Their comments do not require a response from the author, unless they ask questions. It is frowned upon to comment too much on code formatting.
In practice, our developers submit PRs for just about everything. Most PRs are small, and most feedback is addressed. Reviewers are thanked and given kudos for finding issues. Knowledge is shared, and the codebase is improved. I’d estimate that 90% of our bugs are caught in review.
Product quality has improved drastically since we implemented this practice. Developers consistently state that our PR process is their favorite part of the job. They get to share their work, get feedback, and become better developers.
In addition, I shared pre-commit git hooks set up to run a few tools that a) enforced the team's code format and documentation standard; and b) performed static code analysis. This mostly prevented code reviews from devoting time to code format and basic documentation issues.
Do you have standards for those though? Like gofmt, prettier, ide formatting configs, etc? I and many others can be rather ocd about maintaining a consistent code style, although I will admit that it's far too easy to focus on that in code reviews (low-hanging fruit). Which is why it should be automatically enforced by tooling so it isn't a distraction during reviews.
Yes, we try to follow industry standards for code formatting as much as possible, but we also try not to be too picky about formatting if it doesn't meet those standards exactly. Usually authors will make the same formatting mistakes multiple times, so rather than comment on each and every badly formatted line, we try to comment on the type of mistake once and note that there are other instances in the same PR.
I agree that auto-formatting is the way to go. The big thing holding us back from using those tools is that we'd first need to reformat the existing code, otherwise the we'd wind up with a lot of unrelated formatting changes in our PRs (assuming that whole files are formatted when saved, rather than just new lines). Certainly not an impossible feat, just something we haven't done yet.