I cannot concede to the author’s point. It sounds like their problems are more due to logistics than anything else. Overall, this article sounds more like “micro-managing development via JIRA sucks”. That, and I think the author is approaching their job with a bit of immaturity. The idea of their ideal workflow is actually destructive in large teams, if not outright dangerous – i.e. working without pull requests, pushing directly to master, with little care placed towards incident documentation.
One has to distinguish documentation from bureaucracy. Not all documentation is bureaucracy, not all bureaucracy is documentation; it’s not black and white.
“I create a new branch for my fix, also via JIRA”
If your VCS tools integrate with your bug tracker (they bloody well should), then it doesn’t matter where the branch is created. I think JIRA & Stash can be set so that you can tag your commits or branch name with a ticket number and JIRA will realize that this is the branch where the fixes are.
(Whether project or customer management, or the incident reporters, should care about branches, is a completely different topic.)
“You don’t file a ticket which says “the headline font size should be increased by 0.5px”, you just go and do it.”
Yes, you do. It doesn’t have to be an actual ticket – it can be anything, as long as it’s documented. Now, I’m not particularly in favour of JIRA-driven development – or letting any tool do dictate development – but jesus christ. This statement belies an extreme misunderstanding of software development practices. One doesn’t simply roam around in a code base looking for problems unless that is the specified mode developers should operate in. Otherwise you’re just doing hobby-project development at work and it is very likely that you’re oblivious to the long term goal (providing working software) and focusing on short term yak shaving (fixing font size because I think they’re ugly).
I mean it’s a completely different thing to have a backlog and clear it, but this is something that needs co-ordination, not because you have to indulge in processes and management, but it helps to organize backlog clearing into actual goals, instead of patrolling around a codebase as if it were an insane asylum.
The author’s disparaging of code reviews seems to be rooted in logistical problems, negligent co-workers, than due to any actual fault in the idea of code reviews. Done badly, they are a nuisance, done well, they are extremely valuable.
“You don’t file a ticket which says “the headline font size should be increased by 0.5px”, you just go and do it.”
You also don’t need a ticket to open a pull request. I do PR development for my personal projects. It just lets me track the unit I did changes in and look back over the history of them. It also gives me a way to make sure the change passes my checklist (tests) before bringing it into master. It isn’t really expensive and it saves me from letting some silly mistakes in. I don’t make tickets, though.
It doesn’t have to be an actual ticket – it can be anything, as long as it’s documented.
A commit is documentation. A PR is documentation.
One doesn’t simply roam around in a code base looking for problems
Of course you do — proactively finding and fixing tech debt, test coverage, style errors, etc. is part of being a professional, and the less bureaucracy or process between me and my ability to do that, the better. Ideally it is zero.
Of course you do — proactively finding and fixing tech debt, test coverage, style errors, etc. is part of being a professional, and the less bureaucracy or process between me and my ability to do that, the better. Ideally it is zero.
You did not read the latter half of the sentence. It says, with emphasis, that one shouldn’t do bug hunting unless that is the mode one is operating in. For example, one could be pruning the backlog while responding to incidents. This is fairly common.
However, that sort of work should be somewhat organized. Doing this without any sort of co-ordination or communication is asking for trouble.
And I doubt the author is currently engaged in this mode.
This is true. It was even known back to Fagan with his Software Inspection Process in 1970’s that you should really separate the two activities. The mindset is different enough that he wanted people to focus on building stuff for probably hours/days, then focus only on looking for specific kinds of defects for period of time, then focus on fixing defects, and repeat. Keeps people in the zone with the right information in subconscious mind. The repetition of it with checklists to glance at as reminders.
The code review complaints here sound like an organization that’s poorly managed and/or have developers that don’t give a shit. On top of JIRA workflow that also looks bad.
From recent experience with 3 large teams that do pull-request-development, I reject the author’s premise that people don’t actually review code. I’m tasked with reviewing pull requests for security and I usually lose the race to point out flaws in code; the non-security reviewers beat me to the punch.
Most PRs I see have multiple stylebook and efficiency critiques and almost all get iterated at least once and usually multiple times before being merged.
I also reject the premise that the data being captured in Jira tickets is already present in the git log. No, it isn’t. The git log is “what”, but not “why”. Most Jira tickets have at least 2 people commenting in them, something no git commit comment has. Also: being able to demonstrate traceability of code back to tickets is super valuable if you have regulated security requirements.
From what I can tell, in real teams, this process works really well.
Minor note: there is a case where people doing “code reviews” get bogged down in stylistic things or best practices and can gate and slow down deployment of new features.
That is a problem with the people, and not with the idea–it works pretty well once that’s addressed.
EDIT: Downvote incorrect? How, exactly, is this incorrect? I’ve seen this happen multiple times on teams that use code review. Fixing it is part of getting the team up to speed.
A codebase should have a style guide. If the code is within the style guide, then there is no problem. If the code doesn’t fit the style guide then fix it. It’s not the reviewers fault that the author didn’t follow the style guide.
Sometimes I have seen people try to dismiss review comments as stylistic when they are really more than that. Spaces around parameters is a stylistic issue. Validating input and checking error codes are not. Avoiding code duplication is not a stylistic issue. Documenting code is not stylistic issue.
Often times the style comments come first because they jump out at the reader when they are reading the code to try to understand it. If the comments never progress beyond style then the reviewer hasn’t done their job.
get bogged down in stylistic things or best practices
If it is being described as “bogged down” then it might be too much, but in general a large part of code reviews should be making sure everyone is following the team coding standards and best practices. Tests can ensure the thing actually does what it is supposed to so reviews aren’t really about verifying functionality.
If your team isn’t actually providing meaningful feedback during code reviews, that’s usually a problem with the team, not the process. My team is hit or miss, we have a few people that don’t read anything and hit approve, and some that actually provide meaningful feedback. And that’s a people problem that we’re actively working on fixing.
But you’re not a runner, you’re a member of a team, all needing to coordinate together and with the rest of the teams and people inside the business. IMO the hardest part is usually not the coding, but the communication.
Having even rubber-stamp reviews means at least two people you’re coordinating with are simply aware of the work you’ve done. This is valuable!
But perhaps not all issues should be logged. You don’t file a ticket which says “the headline font size should be increased by 0.5px”, you just go and do it.
This is “Running Around With Your Hair On Fire” Driven Development. Prioritizing your time by recency of request leads to behaviour in which you’re constantly swapping out of one request/emergency to cope with an even more recent one.
Always direct the client to open a ticket for the request, and prioritize the request with the rest of your work. Keep working on the work you were doing before the request came in, or the project is never going to get anywhere.
That said, don’t use JIRA. JIRA is dogshit.
It takes more time for my commit to get into master. Experience shows that master is the only branch that receives any testing (did you see the footnote?), so if my commits introduce any problems, it takes more time for those problems to be noticed.
This is why you want PR-based merges to master. Never merge someone’s code if tests to exercise the code in the PR aren’t also part of the PR. You’re not just relying on manually poking at the app to test, are you?
The master branch enables your developers to collaborate, to share code.
Pull Requests provide a semi-organized way for a team to raise consensus on doing things one way or another, and not be constantly angry that “that one idiot committed his dumbshit Docker changes again and he’s really bad at Docker”. If a team can’t raise consensus around a PR, it shouldn’t get merged.
Prioritizing your time by recency of request leads to behaviour in which you’re constantly swapping out of one request/emergency to cope with an even more recent one.
I did this for too long at a previous job. It only (sorta) worked because I decided to invest gobs of time outside of work to shore up matters behind the scenes in an attempt to stave of more forseeable emergencies that would have resulted from previous ones.
I don’t recommend that, by the way, because it probably won’t get recognized.
Bug trackers are terrible. Don’t track bugs, just fix them. I managed to do this in a team for 8 years. At any given time our software had 0 known production defects. A bad year was when 2 bugs were found in production. Our secret was to just fix all bugs we found immediately. No tracking, no long process. Fix, test, deploy.
Hello fellow squasher! I came here to post basically the same thing. If you prioritize zero bugs – it ends up feeling a lot like Inbox Zero – you simply don’t accumulate them. It does require absolute buy in from all tiers relevant tiers of management, which can be half the problem… and once you enter an organization with a huge bug queue – it can be challenging to say “no new features for 4 months, entire team is burning down bug list”.
The key thing I have found is making the TEAM halt and fix bugs, all work stops, bugs are fixed.
I think management buy in is key. The trick I used is to simply ignore the standard process. Ask for forgiveness not for permission. This usually works because we delivered superior software at 10x the speed. No good manager is going to mess with that gravy train.
The reason “master” is sacrosanct is because having a known good state of your application code is an absolute must. The reason we use tickets, pull requests, and branches is so that we can ensure that we:
Know what is in master
Why it is in master
Coordinate multiple teams working on the same code-base
Is this overkill for some projects? Absolutely. I have a pile of projects where I’m the only developer. I don’t even use branches on those (I also never squash commits because I prefer a half million tiny commits to feature-level commits, and it’s my goddamn code, so I can do what I want).
I struggle to see how this one person’s anecdotal experience can suddenly be applied in the macro. And with regard to the authors opinions on code reviews.
You have your own issues to work on, and my interruption already did some damage by de-focusing you.
What is the rush if you have been working on the code for a week? Is there not some means of putting them into a queue and letting people review their code when you would not interrupt them? We use a Slack channel to post our code reviews to.
Reading code is hard. Nobody really does it.
Yes, it is that is why it takes time and generally some back and forth to review code well. Just because it is hard does not mean that it is not worth doing.
But wait, speaking of mandatory code reviews, did you think about this? So you don’t trust your own developers, and require attention of at least 3 people for a commit to get in.
This isn’t a matter of trust. I want my code to be reviewed because a good review can help me see the problem in a new way and sometimes find a better solution than I initially came up with.
I absolutely love code reviews and don’t think I could work in a place that didn’t care enough to help its developers be the best they can be and the product be the best it can be. This post completely ignores the value of collaboration on solving problems.
The author makes several good points, but many of his observations don’t jive with mine.
Branches are hugely useful and allow developers to collaborate without stepping on each other’s toes. They allow one person to collaborate with themselves without stepping on their own toes ; ). I have a branch for just about every idea I have. It’s unfathomable to me that someone could consider a branch just “bureaucracy”.
Re: code reviews. I don’t typically review the minified css/js file. There’s usually an unminified delta that’s reviewable, and simple obvious one-line changes are where code review is most effective. It’s tough to review a 1k line delta and see problems. It’s easy to look at a one line delta and say (to use the author’s example): “Oh, if you bump the H1 font size you need to bump H2, H3 proportionately”.
“How do we keep the master branch stable?” The answer is: you don’t.
This has literally never been the case at any company I’ve worked at in the last decade. Not everyone I’ve worked with has been an A player. Keeping the master branch stable is not that hard. My team isn’t perfect, but if I had to guess master is buildable > 99% of the time. Bugs that creep in are usually limited to an hour or two.
Master should always build. Tests should be ran prior to merge. I work on a product involved in every single request to a multi-billion dollar site and the only different between our master and prod branches is that master undergoes a couple more hours of regression testing that we haven’t automated away. yet.
At best, you can provide stylistic objections, like “hey why did you indent code like that?”
Really? That’s the best piece of code review feedback you’ve ever gotten? This is hard for me to believe, but then again I’ve heard of so-called “software development shops” that use a rubber ducky or a potted plant as the “lock” to checkout a module and work on it. Because there’s no source control on the shared ftp server with their product’s source.
/me buries his head in the sand and imagines a world where branches, testing, and code review are universal.
I believe there are ways to make that work. For example, “somebody mentions a problem” should happen by opening an ticket. Checkout, acknowledge ticket, and create branch should be one command. You are a developer. Automate it.
I am infinitely annoyed by Jira myself, to the point of building myself tools that will ease the workflow that I so, so hate. That being said, I think Jira is much more about planning work upstream ahead of time than it is about imposing craptastic workflows on the users. It’s not only about the dev work.
I’ll leave the Scrum criticism for another time, but the whole lifecycle of a story looks like this in general, at least for me:
- Some need, request, feature or problem is translated into a summarized description.
- The Product Owner and the dev team refine the story until it’s ready to be dealt with (precise enough, concise enough, perhaps a while in advance if it’s big.) Note that this can be either very long, or very short (bugs usually are quicker to send through this process, I find).
- The team plans out a collection of those stories in a relatively sizeable chunk.
- The team deals with the sprint
- The PO, scrummaster, whoever else, runs reports on tasks that are either grouped together by kind (story, bug), timeline, whatever else criteria is allowed in the shiny queryable UI
Note that most of the time, almost everyone outside of the immediate dev team is not comfortable with Git.
Also note that most of the time, there’s more than one team involved.
I understand the process is annoying, I’m just saying we came from somewhere, and this is where we (or at least a few of us) are right now.
A script where you type a single line description of fix, it creates a branch and all the goodies on JIRA, you push your fix, then run a “complete” command that does the stuff after coding
Two more steps but infinity times more respect for the people working with you on the project
Since pretty much all comments seem to be critical, I’m just starting a new thread rather than replying to a certain comment.
First off: yes, I agree that for open-source, PR is the way to go. One can’t open the repository to complete strangers. I was talking about work, on a closed product, by a relatively small team; we all know each other.
About master being stable at all times, I maintain that this is a fallacy. Yes, of course master should build and the test suite should run flawlessly. But “tests can only prove the presence of bugs, not their absence” (Dijkstra). Inevitably, bugs will enter master and sometimes (quite often!) the fix is only a single line of code. You should not require a PR and code review for that.
I think this meme is a cultural signaling mechanism. In my circles a developer would never even say the words “it’s just a single line of code” without using an ironic tone of voice, eye rolling, etc. :)
You should not require a PR and code review for that.
You should actually. For one, if it is literally one line of code you probably didn’t write a test to make sure it doesn’t regress again which you should and the reviewer should make you do.
Another thing is that one line of code changes quite often have unforeseen effects elsewhere. Review can help suss this out.
Finally, having at least a PR (if not a review) is some automatic documentation that there was an issue and it was fixed.
My suspicion: The author is kind of fed up with the atlassian stack, JIRA and Stash aren’t nearly as nicely integrated as Github’s issues with PR (and probably also Gitlab, etc.).
Nevertheless, there are arguments to be made for trunk-based development, namely early integration. I have seen quite a few PR with valuable changes which died before merging because the master branch had moved on considerably. Also, with PRs boy-scout changes and refactorings are on average more discouraged (by reviewers insisting on “this doesn’t belong in this PR, etc.).
IMHO the key question one should ask when deciding between a PR or a trunk-based workflow is slightly different:
Do you embrace change, or do you want to moderate change?.
Trunk based development is definitely on the change-is-good side of things, most code bases don’t start with a pull request on an empty commit. Some projects are better of with tighter control, for example OSS (with many potential contributors), or more mature software projects where a change can easily mean a regression (in features, or code quality).
When I worked in a team trunk-based, we definitely had code review. It was just retroactively. it worked, because the team had enough cohesion so people incorporated review suggestions reliably.
I cannot concede to the author’s point. It sounds like their problems are more due to logistics than anything else. Overall, this article sounds more like “micro-managing development via JIRA sucks”. That, and I think the author is approaching their job with a bit of immaturity. The idea of their ideal workflow is actually destructive in large teams, if not outright dangerous – i.e. working without pull requests, pushing directly to master, with little care placed towards incident documentation.
One has to distinguish documentation from bureaucracy. Not all documentation is bureaucracy, not all bureaucracy is documentation; it’s not black and white.
If your VCS tools integrate with your bug tracker (they bloody well should), then it doesn’t matter where the branch is created. I think JIRA & Stash can be set so that you can tag your commits or branch name with a ticket number and JIRA will realize that this is the branch where the fixes are.
(Whether project or customer management, or the incident reporters, should care about branches, is a completely different topic.)
Yes, you do. It doesn’t have to be an actual ticket – it can be anything, as long as it’s documented. Now, I’m not particularly in favour of JIRA-driven development – or letting any tool do dictate development – but jesus christ. This statement belies an extreme misunderstanding of software development practices. One doesn’t simply roam around in a code base looking for problems unless that is the specified mode developers should operate in. Otherwise you’re just doing hobby-project development at work and it is very likely that you’re oblivious to the long term goal (providing working software) and focusing on short term yak shaving (fixing font size because I think they’re ugly).
I mean it’s a completely different thing to have a backlog and clear it, but this is something that needs co-ordination, not because you have to indulge in processes and management, but it helps to organize backlog clearing into actual goals, instead of patrolling around a codebase as if it were an insane asylum.
The author’s disparaging of code reviews seems to be rooted in logistical problems, negligent co-workers, than due to any actual fault in the idea of code reviews. Done badly, they are a nuisance, done well, they are extremely valuable.
You also don’t need a ticket to open a pull request. I do PR development for my personal projects. It just lets me track the unit I did changes in and look back over the history of them. It also gives me a way to make sure the change passes my checklist (tests) before bringing it into master. It isn’t really expensive and it saves me from letting some silly mistakes in. I don’t make tickets, though.
A commit is documentation. A PR is documentation.
Of course you do — proactively finding and fixing tech debt, test coverage, style errors, etc. is part of being a professional, and the less bureaucracy or process between me and my ability to do that, the better. Ideally it is zero.
You did not read the latter half of the sentence. It says, with emphasis, that one shouldn’t do bug hunting unless that is the mode one is operating in. For example, one could be pruning the backlog while responding to incidents. This is fairly common.
However, that sort of work should be somewhat organized. Doing this without any sort of co-ordination or communication is asking for trouble.
And I doubt the author is currently engaged in this mode.
This is true. It was even known back to Fagan with his Software Inspection Process in 1970’s that you should really separate the two activities. The mindset is different enough that he wanted people to focus on building stuff for probably hours/days, then focus only on looking for specific kinds of defects for period of time, then focus on fixing defects, and repeat. Keeps people in the zone with the right information in subconscious mind. The repetition of it with checklists to glance at as reminders.
The code review complaints here sound like an organization that’s poorly managed and/or have developers that don’t give a shit. On top of JIRA workflow that also looks bad.
From recent experience with 3 large teams that do pull-request-development, I reject the author’s premise that people don’t actually review code. I’m tasked with reviewing pull requests for security and I usually lose the race to point out flaws in code; the non-security reviewers beat me to the punch.
Most PRs I see have multiple stylebook and efficiency critiques and almost all get iterated at least once and usually multiple times before being merged.
I also reject the premise that the data being captured in Jira tickets is already present in the git log. No, it isn’t. The git log is “what”, but not “why”. Most Jira tickets have at least 2 people commenting in them, something no git commit comment has. Also: being able to demonstrate traceability of code back to tickets is super valuable if you have regulated security requirements.
From what I can tell, in real teams, this process works really well.
I agree with your points overall. We do PR development and we do good, real reviews.
However, the git commit messages should be “why” not “what”. “what” is for the git diff, the message should tell you why it was changed.
A good point that that is only one person’s point of view though, and something like jira will often have multiple people commenting.
Edit: One thing I thought of, also the git summary line is typically “what” rather than “why”.
Minor note: there is a case where people doing “code reviews” get bogged down in stylistic things or best practices and can gate and slow down deployment of new features.
That is a problem with the people, and not with the idea–it works pretty well once that’s addressed.
EDIT: Downvote incorrect? How, exactly, is this incorrect? I’ve seen this happen multiple times on teams that use code review. Fixing it is part of getting the team up to speed.
A codebase should have a style guide. If the code is within the style guide, then there is no problem. If the code doesn’t fit the style guide then fix it. It’s not the reviewers fault that the author didn’t follow the style guide.
Sometimes I have seen people try to dismiss review comments as stylistic when they are really more than that. Spaces around parameters is a stylistic issue. Validating input and checking error codes are not. Avoiding code duplication is not a stylistic issue. Documenting code is not stylistic issue.
Often times the style comments come first because they jump out at the reader when they are reading the code to try to understand it. If the comments never progress beyond style then the reviewer hasn’t done their job.
If it is being described as “bogged down” then it might be too much, but in general a large part of code reviews should be making sure everyone is following the team coding standards and best practices. Tests can ensure the thing actually does what it is supposed to so reviews aren’t really about verifying functionality.
If your team isn’t actually providing meaningful feedback during code reviews, that’s usually a problem with the team, not the process. My team is hit or miss, we have a few people that don’t read anything and hit approve, and some that actually provide meaningful feedback. And that’s a people problem that we’re actively working on fixing.
But you’re not a runner, you’re a member of a team, all needing to coordinate together and with the rest of the teams and people inside the business. IMO the hardest part is usually not the coding, but the communication.
Having even rubber-stamp reviews means at least two people you’re coordinating with are simply aware of the work you’ve done. This is valuable!
As an engineer, I loathe simplistic auto-stereotyping like “As a developer, I hate bureaucracy”.
This is “Running Around With Your Hair On Fire” Driven Development. Prioritizing your time by recency of request leads to behaviour in which you’re constantly swapping out of one request/emergency to cope with an even more recent one.
Always direct the client to open a ticket for the request, and prioritize the request with the rest of your work. Keep working on the work you were doing before the request came in, or the project is never going to get anywhere.
That said, don’t use JIRA. JIRA is dogshit.
This is why you want PR-based merges to master. Never merge someone’s code if tests to exercise the code in the PR aren’t also part of the PR. You’re not just relying on manually poking at the app to test, are you?
Pull Requests provide a semi-organized way for a team to raise consensus on doing things one way or another, and not be constantly angry that “that one idiot committed his dumbshit Docker changes again and he’s really bad at Docker”. If a team can’t raise consensus around a PR, it shouldn’t get merged.
JIRA isn’t a bug tracker. It’s a system for performing state transitions on entities, of which one configuration happens to resemble a bug tracker.
Yes, that is one overcomplicating decision that makes it such dogshit to deal with ;-)
I did this for too long at a previous job. It only (sorta) worked because I decided to invest gobs of time outside of work to shore up matters behind the scenes in an attempt to stave of more forseeable emergencies that would have resulted from previous ones.
I don’t recommend that, by the way, because it probably won’t get recognized.
Bug trackers are terrible. Don’t track bugs, just fix them. I managed to do this in a team for 8 years. At any given time our software had 0 known production defects. A bad year was when 2 bugs were found in production. Our secret was to just fix all bugs we found immediately. No tracking, no long process. Fix, test, deploy.
Hello fellow squasher! I came here to post basically the same thing. If you prioritize zero bugs – it ends up feeling a lot like Inbox Zero – you simply don’t accumulate them. It does require absolute buy in from all tiers relevant tiers of management, which can be half the problem… and once you enter an organization with a huge bug queue – it can be challenging to say “no new features for 4 months, entire team is burning down bug list”.
The key thing I have found is making the TEAM halt and fix bugs, all work stops, bugs are fixed.
I think management buy in is key. The trick I used is to simply ignore the standard process. Ask for forgiveness not for permission. This usually works because we delivered superior software at 10x the speed. No good manager is going to mess with that gravy train.
I think there are a few missed points, here.
The reason “master” is sacrosanct is because having a known good state of your application code is an absolute must. The reason we use tickets, pull requests, and branches is so that we can ensure that we:
Is this overkill for some projects? Absolutely. I have a pile of projects where I’m the only developer. I don’t even use branches on those (I also never squash commits because I prefer a half million tiny commits to feature-level commits, and it’s my goddamn code, so I can do what I want).
Scale your process to your organization.
I struggle to see how this one person’s anecdotal experience can suddenly be applied in the macro. And with regard to the authors opinions on code reviews.
What is the rush if you have been working on the code for a week? Is there not some means of putting them into a queue and letting people review their code when you would not interrupt them? We use a Slack channel to post our code reviews to.
Yes, it is that is why it takes time and generally some back and forth to review code well. Just because it is hard does not mean that it is not worth doing.
This isn’t a matter of trust. I want my code to be reviewed because a good review can help me see the problem in a new way and sometimes find a better solution than I initially came up with.
I absolutely love code reviews and don’t think I could work in a place that didn’t care enough to help its developers be the best they can be and the product be the best it can be. This post completely ignores the value of collaboration on solving problems.
The author makes several good points, but many of his observations don’t jive with mine.
Branches are hugely useful and allow developers to collaborate without stepping on each other’s toes. They allow one person to collaborate with themselves without stepping on their own toes ; ). I have a branch for just about every idea I have. It’s unfathomable to me that someone could consider a branch just “bureaucracy”.
Re: code reviews. I don’t typically review the minified css/js file. There’s usually an unminified delta that’s reviewable, and simple obvious one-line changes are where code review is most effective. It’s tough to review a 1k line delta and see problems. It’s easy to look at a one line delta and say (to use the author’s example): “Oh, if you bump the H1 font size you need to bump H2, H3 proportionately”.
This has literally never been the case at any company I’ve worked at in the last decade. Not everyone I’ve worked with has been an A player. Keeping the master branch stable is not that hard. My team isn’t perfect, but if I had to guess master is buildable > 99% of the time. Bugs that creep in are usually limited to an hour or two.
Master should always build. Tests should be ran prior to merge. I work on a product involved in every single request to a multi-billion dollar site and the only different between our master and prod branches is that master undergoes a couple more hours of regression testing that we haven’t automated away. yet.
Really? That’s the best piece of code review feedback you’ve ever gotten? This is hard for me to believe, but then again I’ve heard of so-called “software development shops” that use a rubber ducky or a potted plant as the “lock” to checkout a module and work on it. Because there’s no source control on the shared ftp server with their product’s source.
/me buries his head in the sand and imagines a world where branches, testing, and code review are universal.
I believe there are ways to make that work. For example, “somebody mentions a problem” should happen by opening an ticket. Checkout, acknowledge ticket, and create branch should be one command. You are a developer. Automate it.
I am infinitely annoyed by Jira myself, to the point of building myself tools that will ease the workflow that I so, so hate. That being said, I think Jira is much more about planning work upstream ahead of time than it is about imposing craptastic workflows on the users. It’s not only about the dev work.
I’ll leave the Scrum criticism for another time, but the whole lifecycle of a story looks like this in general, at least for me:
- Some need, request, feature or problem is translated into a summarized description.
- The Product Owner and the dev team refine the story until it’s ready to be dealt with (precise enough, concise enough, perhaps a while in advance if it’s big.) Note that this can be either very long, or very short (bugs usually are quicker to send through this process, I find).
- The team plans out a collection of those stories in a relatively sizeable chunk.
- The team deals with the sprint
- The PO, scrummaster, whoever else, runs reports on tasks that are either grouped together by kind (story, bug), timeline, whatever else criteria is allowed in the shiny queryable UI
Note that most of the time, almost everyone outside of the immediate dev team is not comfortable with Git. Also note that most of the time, there’s more than one team involved.
I understand the process is annoying, I’m just saying we came from somewhere, and this is where we (or at least a few of us) are right now.
A fix for all this:
A script where you type a single line description of fix, it creates a branch and all the goodies on JIRA, you push your fix, then run a “complete” command that does the stuff after coding
Two more steps but infinity times more respect for the people working with you on the project
[Comment removed by author]
Since pretty much all comments seem to be critical, I’m just starting a new thread rather than replying to a certain comment.
First off: yes, I agree that for open-source, PR is the way to go. One can’t open the repository to complete strangers. I was talking about work, on a closed product, by a relatively small team; we all know each other.
About master being stable at all times, I maintain that this is a fallacy. Yes, of course master should build and the test suite should run flawlessly. But “tests can only prove the presence of bugs, not their absence” (Dijkstra). Inevitably, bugs will enter master and sometimes (quite often!) the fix is only a single line of code. You should not require a PR and code review for that.
The vast majority of production outages I’ve seen have started with “this fix is only a single line of code”.
I think this meme is a cultural signaling mechanism. In my circles a developer would never even say the words “it’s just a single line of code” without using an ironic tone of voice, eye rolling, etc. :)
You should actually. For one, if it is literally one line of code you probably didn’t write a test to make sure it doesn’t regress again which you should and the reviewer should make you do.
Another thing is that one line of code changes quite often have unforeseen effects elsewhere. Review can help suss this out.
Finally, having at least a PR (if not a review) is some automatic documentation that there was an issue and it was fixed.
My suspicion: The author is kind of fed up with the atlassian stack, JIRA and Stash aren’t nearly as nicely integrated as Github’s issues with PR (and probably also Gitlab, etc.).
Nevertheless, there are arguments to be made for trunk-based development, namely early integration. I have seen quite a few PR with valuable changes which died before merging because the master branch had moved on considerably. Also, with PRs boy-scout changes and refactorings are on average more discouraged (by reviewers insisting on “this doesn’t belong in this PR, etc.).
IMHO the key question one should ask when deciding between a PR or a trunk-based workflow is slightly different:
Do you embrace change, or do you want to moderate change?.
Trunk based development is definitely on the change-is-good side of things, most code bases don’t start with a pull request on an empty commit. Some projects are better of with tighter control, for example OSS (with many potential contributors), or more mature software projects where a change can easily mean a regression (in features, or code quality).
When I worked in a team trunk-based, we definitely had code review. It was just retroactively. it worked, because the team had enough cohesion so people incorporated review suggestions reliably.