We decided that everyone in the team is allowed to approve changes. If no one has reviewed your change within 24 hours you are allowed to approve it yourself. It will usually come up in the daily sync that a self approval is imminent, which usually leads to someone taking a look.
genius!
Surely this will just devolve into “no reviews ever”.
More like “Jerry reviews everything”
We very seldom resort to self approvals. Everyone in the team see code reviews as important. But also that progress trumps code review.
Self-approval leads to a road of sadness. For example, a theoretical company needs to self-renew an ssl cert. No problem, the cert will be stored with the rest of the secrets and retrieved in a secure way on deployment. Unfortunately if you don’t store the cert key in a secure way, the deployment still works fine and you don’t need to figure out the “onerous” encryption process.
So you push the private key to the company git repo, and then deploy the cert! Done and Done.
We have well established ways to deal with secrets. Also, everyone is responsible enough to not self approve changes where they do things they are uncertain of.
If you don’t establish an encryption mechanism for secrets that allows for automatic, in memory decryption on deployment from the start of your project, then your project is run by incompetent developers/ops specialists/architects/management/etc. and deserves to fail.
which usually leads to someone taking a look
Nevermind the idea that one reviewer is somehow sufficient, this sounds like pure fantasy. Did you forget a “/s”?
Who said anything about only requiring 1 reviewer? And no, I did not drop an /s. You should try working for a healthy team where everyone takes collective responsibility and where the teams progress is more important than any one person’s progress.
I get the feeling you feel like I was somehow calling you out. I want to clarify the the intent of my message was more in the spirit of “wow must be nice” than “you’re making that up”. But also I’m just interested in how different your experience is from mine.
Who said anything about only requiring 1 reviewer?
I must have misunderstood. You said “If no one has reviewed your change within 24 hours you are allowed to approve it yourself.” To me, that sounds like, after 24 hours of no review, one self-approval is considered sufficient. That, in turn, seems to imply that before 24 hours, one non-self-approval is probably sufficient, no?
You should try working for a healthy team where everyone takes collective responsibility and where the teams progress is more important than any one person’s progress.
I’ve had team members in the past who are very self-focused, they tend to close a lot of tickets and look good, then get promoted out, leaving an unmaintainable mess behind. Allowing that is generally a failure of leadership. But right now, that’s not our problem, and what you describe is pretty much how we operate.
I’d love to work on a team where everybody took code review a lot more seriously, believe me, it’d be nice, but my team does generally get everything approved, with at least two non-self approvals, in under 24 hours. If something is getting ignored because people are busy and it’s a large change because we aren’t perfect, and there is some reason to get it in soon, it just takes a quick request on Slack to get the needed attention.
What I found surprising about your description was more that the potential of a self-approval coming up would, in itself, get people’s attention, rather than somebody reaching out personally and asking for a review.
Our big weakness is review quality, not quantity. It’s crazy the number of times I look at something and see the two or three approvals already, start going through it, and find issue after issue. I see that on other teams as well, where there’s usually only one or two devs who ever really make any comments on a review, it seems to be very common.
Ughh I’m currently waiting on a review and I’ve pinged people multiple times but nothing. It’s blocking all my work for the rest of the week.
LGTM +
I’ll review it
You get paid hourly? If so sounds like you shouldn’t say anything and get paid to do nothing
I’m salary, but my boss loves nothing more than firing people.
I mean you’ve done your job and even reminded them everyday that they need to do theirs for you to do yours. Take screenshots and if they try to sack you, straight to court/your fair work ombudsman
I live in a place with at will employment, so they can fire me whenever they want.
I get paid double my last job, but it’s like Netflix, a lot of people get fired from Netflix.
uggg. Another multi thousand line PR. Again.
I’ll leave it to tomorrow.
Tomorrow: fuck this. Ive got shit to do.
It is also a “refactoring”.
Unless those lines are autogenerated I’d be rather concerned
I wish they were auto-generated. The most senior engineer, clever as he is, has a thing for re-architecturing things and doing “refactors” and “tidies” at the same time.
I’ve voiced my concerns and at least the big re-architecturing sometimes get broken into child-jiras, but it doesn’t always help.
Then theres the merge conflicts on my branches i have to resolve. AHHH
As a senior at my last big company job, basically all I did was conduct meetings and do PRs. It’s such a grind.
My opinion now is that most PR is worthless anyway. Most people give, at best, a superficial skim for typos, lack of comments, or other low-hanging replies (that usually, really, a static checker or linter should be dealing with).
Reading the code base in little chunks like that doesn’t give you proper context for the changes you’re reading. Automated unit and integration tests would be better for catching issues like that, but of course then who is reviewing and verifying the tests? Who’s writing them for that matter?
Ideally, pair-programming or having extra people on projects to create knowledge redundancy would help. But companies want to replace juniors with AI now, so that’s not looking good. Senior devs and architects might know the major pieces of much of the code, but can they “load it into working memory” sufficiently to do a quality PR that will catch something the tests didn’t and QA wouldn’t? Not in my experience.
I think the best actually-implementable solution for most teams is to get rid of PR expectations and take a multi-pronged approach to replacing that process.
- use tooling to check for and fix basic stuff. Use a linter, adopt a code standard, get a code formatting tool that forced adherence to the standard and run it on every PR.
- Unit tests if you got them, start if you don’t. You don’t need 90% code coverage, just make sure critical paths are covered.
- Turn one of your useless meetings into a code review session. Each week/sprint, one Very Important Code section is presented by the developer that works on it most or that last changed it. This helps the whole team learn the code base, gets more eyes on the important stuff regularly, and enforces not just a consistent style but a consistent approach to solving and documenting problems.
- PR (and the github PR approval stuff or its equivalent for you) should be streamlined but preserved. Do have a second person approve changes before merging, just to double check that tests have finished and passed and all that. If your team is so busy that no one ever approves PRs then allow self-approval and be done with it. This will make regular code review very important for security and stability, since any dev could be misbehaving unseen, but these are the trade-offs you make when burning out your team is more important than quality.
This comment seems like a lot of work to read, I’ll pretend I didn’t see it
So you’ll just hit approve?
I caught a junior trying to reimplement an existing feature, poorly, in a way that would have affected every other consumer of the software I’m a code owner on a week or two ago. There’s good reason to keep them around.
PRs suck to do, but having a rotating team of owners helps, and linting + auto formatting helps with a lot of the ticky tacky stuff.
Honestly, the worst part is “newGuy has requested your review on a PR you requested changes on but he hasn’t addressed” that’ll get you in the ignored pile real quick.
I generally agree and like this strategy, but to add to the other comment about catching reimplemented code, there’s just some code quality reviewing that cannot be done by automating tooling right now.
Some scenarios come to mind:
- code is written in a brittle fashion, especially with external data, where it’s difficult to unit test every type of input; generally you might catch improper assumptions about the data in the code
- code reimplements a more battle tested functionality, or uses a library no longer maintained or is possibly unreliable
- code that the test coverage unintentionally misses due to code being located outside of the test path
- poor abstractions, shallow interfaces
It’s hard to catch these without understanding context, so I agree a code review meets are helpful and establishing domain owners. But I think you still need PR reviews to document these potential problems
these are the trade-offs you make when burning out your team is more important than quality.
Yep.
Many directors and CIOs know exactly where they stand regardin the classic value proposition: deliver something trivial before next quarterly earnings statements - at the low easy cost of losing all organizational understanding of the code base.
I will never not hate scrum. Screw all this corporatization of programming.
I would be fine with it if my job was just that review stuff.
But man when you are in the middle of something and you have to review something because they need it soon… It’s annoying as fuck.
Reviewing code is part of your job. But you also deserve uninterrupted focus time. Just block focus time blocks in your calendar and check if your peers need reviees 2-3 times a day.
As a required reviewer on a lot of things, I envy that bear so, so much.
don’t review just blind accept and merge it’s more fun that way
LGTM
I haven’t done any serious programming in a long time. Is this mostly about corporate process and hierarchies for programming or does this apply to open source projects as well?
Seems really demoralizing putting in the work to add something to an open source project and having it waste away unreviewed and unappreciated.
It’s more about scale. Small open source projects might get one PR a month. Your average tech company is dealing with dozens of PR every single day. Review fatigue is real in these environments
Certain this works 100% in the wild. Main issue will be trying to SSH into the server, unless you can borrow their hotspot.
No! Ask him to leave a rating on your encounter with him.
God damn it.
To be fair, we sometimes have to look through multiple related documentation and tickets to make sure the change was actually reviewed and approved by the necessary teams (network, security, etc.). We also have an SLA for PR reviews/approvals and some people have a habit of sending it out for approval at the last minute of the change window.
Or! Alternatively! Bring your wife to the situation and have her talk about her dreams.
