50

So I started a new (software development) job a few months back, and they use a "scrum-inspired" means of management. One of the rules is that the team lead for each team must sign off on and personally merge every pull request to main, irrespective of other team members' / other teams' reviews. (Note that this isn't necessarily a full code review; just a check that the code fits the requirements and other standards.)

This is onerous at best, but is made worse by the fact that my team's lead claims to be very busy with meetings (which I don't doubt) and regularly "forgets" to do the reviews in a timely manner. I've had to apologize to product for having a feature miss a (biweekly) release cut twice now, and my "personal record" is 8 PRs lined up for him.

I often bring up "waiting for code review" as a blocking issue during the daily scrum meeting. I've been leaving it at that rather than "I'm not getting software shipped because you, yes you, won't do your job", of course, but I could say it in a more assertive way.

This procedure isn't his policy; it comes from management and all (5 or so) teams follow it. I have some sympathy for him because it does appear to be a lot of work (the team's size is pushing the limits of what I would be willing to manage), but at the end of the day it's his responsibility, for better or worse.

What can I do to make myself and the team more efficient without jeopardizing our relationship?

5
  • 68
    “ and they use the Scrum method of management”…No, they don’t 🙂
    – Matt
    Mar 13 at 22:04
  • 102
    Well, to be fair, if you went to a sports SE and said "we are playing baseball, but I have a lot of problems dodging the guy with the chainsaw", you would get plenty of posts saying that's not baseball and only a few on professionally dodging chainsaws.
    – nvoigt
    Mar 14 at 7:24
  • 2
    @nvoigt The analogy is kind of OK. Though I would say that generally baseball is played to rules, and generally agile is not played to rules (if you could call various schools of thought rules). The author has asked us to "put aside" the fact this is not really agile. I'd also want to point out that if you implement agile against the religious tombs that dictate how agile should be implemented, people get quite upset, so I think it's worthwhile trying to avoid mentioning it. Mar 14 at 8:01
  • 11
    You're kind of shooting yourself in the foot by apologising for other people. Just say "I'm still waiting for a code review on this", and redirect them to the person causing the blockage when they ask how long that'll take or why it's been in code review so long. Do not, at any point, say the word "sorry". This doesn't solve the underlying problem, but helps eliminate/reduce the amount of blame you take for it.
    – NotThatGuy
    Mar 14 at 20:48
  • 9
    You might think that you are in a pretty sh*tty position, but that TL has it worse. Do you have any idea how boring it would be to just do meetings and code reviews, all day? And STILL get blamed for delays because you "won't do your job"? Have some sympathy for them, they are as much of a victim of the AWFUL practices this company has as you are. And they have it worse, much, much worse. Work with them to try and change the system, or stick to your job (which ends with the PR created), but please have some sympathy for that team lead.
    – Blueriver
    Mar 14 at 21:52

10 Answers 10

63

Unfortunately, there's probably not a whole lot that you can do to remedy this situation, especially as a new hire outside of a management role. However, I can give you a few suggestions.

First, don't apologize for your work missing the release cut when it's not missed because of your fault. It doesn't sound like there's an agreement for how long it should take for the "Scrum Master" to complete this review. As long as you are submitting your work to enter this review gate in a timely fashion, you shouldn't be thinking that you're doing anything wrong.

Second, use the Sprint Retrospective. If your organization is truly agile (which they may not be, based on your description), then there should be opportunities to reflect on the way of working on a regular basis. Keep bringing up the problems with this process - the long queues of reviews, the missed releases. There are probably other problems with this, around getting work integrated and delivered, so any time not having reviews handled in a timely manner affects you, bring it up and be specific about the impact.

You may also want to talk to your manager to make sure that your performance is not being negatively viewed because of someone else's constraints. If it is, you may want to find a way to get ahead of this so the quality and timelines of your work are clear.

9
  • 4
    Well, the worst part with that last bit is, he is my nominal manager. I think we've only had one retrospective since I've been here (yes, I know that's not that often) but I will be sure to specifically mention this during the next one. thank you!
    – fmt
    Mar 13 at 23:12
  • 13
    @Actorclavilis Definitely a lot of what I would consider anti-patterns here. Scrum Masters shouldn't be involved in the development of the product nor should they be managers of anyone on the team. Not only is too much being asked of one person, but there are conflicts between these different roles. Mar 14 at 0:35
  • 5
    You should have retro every sprint. If your "scrum master" is also too busy to manage that, they need to have fewer teams they're assigned to. Retros are the most important part of scrum and working as a team, so I would also bring up that they should happen more regularly (and if you don't mind rocking the boat, volunteer to schedule and lead them for your team) Mar 14 at 1:41
  • 4
    "First, don't apologize for your work missing the release cut when it's not missed because of your fault." That depends on whether they're apologizing on their own behalf, or as a representative of the team. Mar 14 at 20:33
  • 1
    @BrtH I've been in dual developer/Scrum Master roles before. I can't recommend it. It's less harmful in a team-of-team environments where there's someone in a dedicated coaching role across all the teams and people in a dual role on the individual teams. The biggest problem is facilitation - it's extremely difficult to facilitate events while also fully participating in them, with the retro being the hardest of all of them. Just because it's supported by the Scrum Guide doesn't mean that it's a good idea and I just can't endorse it. Mar 15 at 12:51
39

There are going to be a few scrum-aware answers here, which is great, but I'm going to come at it from a more... traditional perspective.

If you suspect there is something that is holding the team back, it would be appropriate to discuss it with your manager. It doesn't have to be overly formal, or dramatically accusatory, you just indicate that you think there is a systematic problem.

If this person is your manager, the conversation is a bit awkward, but it should still occur. But once you are certain they are aware of the problem, it really isn't your problem to deal with anymore. Just ensure you are as productive as possible.

I actually don't think inherently improper for one person to be the gatekeeper of a repository (they may be a SME for example), though there are obvious concerns around bus factor. So I am certainly willing to assume there may be good reasons why things they are the way they are.

I agree with Joe that you should not apologise for things that are out of your control, and for the record, I am a bit proponent for apologising when you do make mistakes.

However, you have a bit of an obligation to ensure your tickets get merged in a timely manner, as much as you can.

So, as you approach the deadline, you need to make things as visibile as possible for the blocker. This may involve sending them a instant message with a prioritised list of outstanding reviews. Example:

Hey Bob. I have a few things I need merged. Including some stuff that needed to be merged last week. I've grouped things by when they are due, so hopefully you can merge these ASAP.

TICKET OVERDUE

  • [PR-123] Add important feature for customer blah
  • [PR-234] Add some other feature

TICKET DUE FRIDAY

  • [PR-345] Make widget darker

TICKET DUE NEXT FRIDAY

  • [PR-456] Fix speiling

You need to make it as easy as possible for them to target the things that are immediately important, as well as highlighting problems. Also by using days of the week names, rather than dates, it's easier to understand the due dates when the message is received.

Obviously if things get worse and worse, more and more things are going to slip into the overdue category, and that looks more and more obviously a problem.

And lastly, I just was to say, if this is a systematic problem which is highlighted all the time, you really just need to try to make your stuff as easy to merge as possible. Because you don't want the situation where other people's stuff gets merged and yours doesn't because it's harder to review.

8
  • 6
    I agree that using the names of the days of the week makes it easier on the person, however I would suggest always adding the actual date in brackets "Ticket due Friday (13/01)" This makes things much easier when you are going back to past emails to show how things were delayed. Also I would add dates to the Overdue tickets so it becomes clear how long they have been delayed. "[PR-123 due Tuesday 1st March]
    – Dragonel
    Mar 14 at 17:32
  • 1
    This also shields the OP from claims its their fault, by providing a trail. Excellent.
    – Stilez
    Mar 14 at 19:36
  • I tried sending reminder mails. Making them distracts from other work, and if you're using anything like JIRA, you'd just duplicating that tool's purpose manually, and the mail will end in th mail backlog of things. While a personal mail may help, a direct talk does not give him the option to push the issue into the backlog.
    – toolforger
    Mar 16 at 9:58
  • @toolforger You have an obligation as much as possible to try to get your stuff merged. If it takes you 15 minutes to compose a message of important things that need to merged, so be it, that's the cost you pay. Mar 16 at 10:53
  • @toolforger Unless you stand over the guy while he actually does it, they're still going to decide their own priority and in a busy environment a verbal discussion can easily get forgotten about whereas at least an email will be seen again at some point.
    – Andy Hames
    Mar 16 at 15:45
25

There isn't a dichotomy between doing nothing and being accusatory. My suggestion would be to raise the issue as an issue, not as an accusation. You suggested:

"I'm not getting software shipped because you, yes you, won't do your job"

Don't do this, obviously (you know this already). Rather, you can say something like this:

I believe that our system whereby you have to approve every single PR before it gets merged is not efficient. You're a very busy guy, and you can't spend all your time approving PRs. As a result, we have backlogs of features that aren't getting merged and deployed to production on time. Can we consider revisiting this requirement in order to improve efficiency?

This isn't avoiding the problem (as you seem to be doing now) nor is it being accusatory and a personal attack, as your suggested alternative. It's raising the issue as a process problem, rather than a person problem and asking if that process problem can be resolved. It's also acknowledging that simply telling your manager to work harder (obviously a bad idea) is not going to work (for many reasons, but primarily because he's already working hard).

2
  • 1
    Part of the process problem here is very likely that the goal of the process isn't even being met. I'll bet you anything that the manager is not checking to ensure "that the code fits the requirements and other standards" and is instead just blindly clicking "merge" in order to clear the backlog once he finally gets around to it. At any rate, discussing it as a process problem rather than a person problem is very sound.
    – Beanluc
    Mar 15 at 19:31
  • Thanks, @ertai87. I've tried phrasing it this way, and it seems to be a system no one truly enjoys. Unfortunately, "team lead" is not exactly a high-ranking position, and I don't think I can fairly try to get him personally to change anything about it.
    – fmt
    Apr 4 at 13:33
14

In the situation you describe, the problem is not that your team lead is acting as a gatekeeper, it's that the team lead is forced to be a gatekeeper, and is running out of bandwidth.

The problem is the process, not the team lead, and the solution is to fix the process, not the team lead -- so there's no reason even to think of being accusatory.

In your daily standups, you should mention it when you are blocked waiting for PRs, or even when waiting for PRs is slowing down your work by making you juggle multiple branches, etc.

At sprint retrospectives, you should bring up the matter again and try to help out with solutions. One thing you might suggest, for example, is to add a second line of reviewers that can pick up and approve the more trivial PRs, escalating to the team lead if there is anything they aren't sure about.

1
3

You shouldn't apologize for a feature missing a deadline due to this internal process. Your work ends once you submit the code for review/merge. From your perspective, it's submitted and "done" at that point since you have no authority or control after that point. If someone asks why a feature missed the cut, you can tell them "I completed the work for that feature and submitted the changes, but it wasn't included in this release for some reason". Point them towards your team lead for more information. This does two things. It lets the other person know that the work has indeed been completed, and it gives your leader a chance to explain what happened. Maybe he didn't have time to process the PR, or maybe there's some other reason that you're not aware of. The goal here isn't to blame or embarrass your leader. The goal is to change the conversation from "you didn't get this done on time" to "these procedural requirements make it difficult to complete work on time". You are more likely to see procedural changes when more people are aware of the problem.

To make living with these requirements easier, one thing your team can do is to implement a code freeze at some point prior to the deadline. Any changes submitted prior to the code freeze will be included in the release, and anything submitted afterwards waits for the next release. This freeze can be time based (e.g., 3 days before the deadline), or adjusted based on the size of the backlog. Ultimately, the idea is that the time period between the freeze and the deadline is long enough to process all of the eligible PRs that are backed up in the queue. That way, there's a phase of the development cycle where your team lead is almost solely responsible for making progress. They can block off that time on their calendars, decline meetings, and otherwise ensure that they have nothing else distracting them from doing what they need to do. When PRs trickle in unpredictably, it's hard to prioritize them and easy to shrug them off for "later". Others may complain about your team lead's unavailability during that time but again, remind them that it's a side effect of the policies that your team is required to operate under.

It might also help to have an informal chat with some of the folks on the other software teams to see if they're doing things differently than your group. You implied that your group was larger than the others, but there could be other factors at play here. Sharing best practices between teams is a great way to get new, already-tested ideas.

3

Other answers are good around leveraging leadership and agile ceremonies like retro to expose the constraint. Two tactical things you could do on top of that:

  1. Assuming you use a tool such as JIRA or Azure DevOps to manage the process, create a column/status for "waiting for PR merge". It will be clear to the scrum master what they need to accomplish during that day. It will demonstrate to leadership why items are stuck and/or then they can choose to manage it.
  2. If that's already in place, can you consider a different git flow-like model where all the engineers work off develop (branched from main) and you collectively at end of the week create a single PR to main? I concede this requires maintenance from the team but it shouldn't be too much churn and relieves the constant dependency on the Scrum Master.

This larger gatekeeping was likely put in place because someone merged a feature in that wasn't ready yet and management added process to protect it. This may be a hard wall to tear down. Suggestion 2 helps your scrum team to do this and could demonstrate a new way all teams work in the organisation to manage this concern.

2
  • 4
    'above two answers' is only current for as long as the amount of answers and/or the vote ordering between maintains that ordering. Mar 14 at 22:17
  • Understood. Thanks! Mar 16 at 2:28
3

As a lead who has unfortunately been in this PR gatekeeper position before I will give you a slightly different perspective on this. Because while you might not be able to change the process, there are things you can do to help.

First, understand the source of this. It is very likely that this process exists because at some point in time "the team" did a bad job at code reviews. This happens a lot because when no one is on the hook, then no one feels directly responsible. It's a kind of Bystander effect This can sometimes result in PR wars or the opposite a parade of "LGTM" reviews. Neither is good.

Second, I want to say that, there have been a lot of really good answers on how to approach this on a systemic basis in your development process, retrospectives, one-on-one communication, putting it into blocked. All good ideas. I would caution to not make it personal or point fingers at individuals or be passive-aggressive about it. A major part of being on a team is having empathy for everyone. And that brings me to your lead...

Leads spend a lot of time in meetings, on planning, both on the architectural level and people level. After all, this, pull requests often come into this as a low priority. Understanding this could lead you to help understand how your and other developer actions can affect this process. Anything you can do to make my life easier when I am doing code reviews will speed up the process.

  • Keep your pull request up to date with the main branch. Check it and sync it several times a day. At least in the morning and sometime in the afternoon. Nothing is more annoying than having to sync a pull request prior to doing a review, hoping that there will be no conflicts. (and that leads to...)
  • Resolve your conflicts. Make sure you are resolving any conflicts that your branch might have with the main branch as soon as you can. If I see a conflict I might take the time to resolve it, or if I don't have the time I'm going to kick it back to you.
  • Make sure PR pipeline is green. If you have automated tests or static analysis in your PR process make sure they are all passing. That often means that after you have submitted the PR you need to check on it a few minutes later to make sure all lights are green. I cannot tell you the number of times I've come across PRs that I am ready to review but I stop because tests or static analysis is red or yellow.
  • Write great unit tests. You should be doing this anyway but if I see unit tests in a PR, and the pipeline is passing all the unit tests, that is a good sign. Those tests are going to be the first thing I look at in the PR. Do they look correct? Are they testing the right things? Are both positive and negative cases covered? Good passing tests give me confidence in the rest of your code.
  • Don't keep your PRs atomic. This goes against conventional wisdom but so does this entire process. Most devs have been taught that one issue -> one branch -> one PR. However, you need to know sometimes it is a good idea to break from this. If I'm reviewing PRs I would much rather review 1 medium size PR than 5 or 6 tiny PRs that all play in the same area of the code. In particular, if they touch the same code, as a reviewer I start to get concerned about interactions between these changes. I have often kicked these back to the developer or developers and asked them to merge the changes into one branch for a combined PR. More than once this has then caught conflicts or issues that otherwise might not have been seen. Remember devs don't own branches, you can share them.
  • Leave a great pull request comment. Introduce the change, tell me what you did and why. Don't just tell me the ticket or tickets this is related to, provide me a link to them. Include the important bits of the ticket in the PR comment. Tell me the importance of this PR and the dependencies. Just telling me it is due for the sprint or by Friday is not helpful. I know this. Everything is due Friday. If it is a UI change, include some screenshots.
  • Leave great comments in your code. Commenting in the code not only helps with the current PR but also all future developers. All code is "self-documenting" when you are writing it because you have it all in your head. Imagine that you are a new developer seeing this code in a year knowing nothing about it. Always use comments to explain the why.
  • Feedback your own PR. Nothing prevents you from reviewing your own PR and leaving comments. If there is something that you think I might look at with greater scrutiny then you can preemptively comment on that line. If I see you've commented on it then maybe I won't have to ask you about it. The one caution here is that if you feel the need to do this first ask yourself if you should just put the comment in the code itself. (see Leave great comments in your code.)
  • Learn from previous PRs. If I have reviewed your code before and left comments on what needs to be changed. Don't submit code that does the same thing again. At best I'll miss it and we will have code that doesn't match the rest of the codebase. At worse, I'll just flag it again and stall your PR. Additionally, now I'm going to start looking at your code even closer.
  • Don't be clever. "Clever" code is fun to write but a pain in the ass to review. Write simple and clear code using constructs and conventions that are common to your language and your code base. Don't submit brain teasers in your pull requests. I might waste my time trying to figure out how it works or I might just send it back to you. Even if I do understand it, I'm likely going to send it back to you to simplify it.
  • Be a great reviewer yourself. Assuming the entire team can review code then you can help with all of the above on everyone's pull requests. If I jump into a PR and see that everyone else has taken an effort to review it (and not just a bunch of "LGTM" comments) then I feel more confident about the process and I need to spend less time to review it. If you see two PRs that could be merged into one, talk to the other dev and merge them. If you see other PRs sitting with unresolved conflicts or broken pipelines, fix them or have the original dev fix them. If you see a PR lacking a good comment, flag it and talk to the dev. If you see a PR with code similar to code that has been rejected before, comment on it.

This last bullet point I cannot stress enough. There are several benefits to this. First, I will start to trust your code more which means less scrutiny on your PRs. Second, If you ever want to get to a state where the team is doing code reviews as a team and not with a gatekeeper then the team needs to demonstrate that it can do effective PR reviews. All of this is necessary for that to occur. Last, you will have evidence that the team is ready to change the process when the gatekeeper is able to primarily leave "LGTM" comments because the team has caught everything that might be an issue.

1
  • 2
    This answer is great because it can improve not only the current OP's situation, but any situation where people are reviewing PRs, including what one might hope full be the OP's future situation. One more thing I can suggest for PRs is to make the sequence of commits tell a story that's clear to follow in both the messages and the diffs. E.g., if you need a refactoring, split it out into a separate commit and put that commit before the commit whose changes benefit from this. A sequence of clear, small changes is much easier to review than a single large set of unrelated changes.
    – cjs
    Mar 17 at 0:18
1

The Scrum Guide says:

Scrum Teams are cross-functional, meaning the members have all the skills necessary to create value each Sprint. They are also self-managing, meaning they internally decide who does what, when, and how.

If that were true for your team, your team would be held accountable for the missed deadlines, and reflect on how to improve process during the next retrospective, and implement the necessary process changes.

However, management has not given your team the authority to make the obvious change (have pull requests merged by more than one person), so you'll likely need to involve whoever makes such policy decisions at your organization.

In organizations, whoever makes a decision should be accountable for that decision. If management makes a decision, and you're held accountable ("have to apologize to product"), this principle is violated. To fix this, you should make transparent what caused the delay, so the correct person is held accountable, and is thus aware of and motivated to fix the process failure.

How to do this depends on how your organization is organized. In an agile org, the team is held accountable for work outcomes, so the team would apologize, and then change its process.

In a hierarchical organization, the team lead is accountable, so the team lead should apologize to product (and be more prompt about reviews in the future) or inform product that policy X is responsible, to allow an escalation to the proper person. In either case, you should not apologize, because you can't fix the problem. Don't take responsibility for things you can not fix.

As an aside to managers, this is why mixing hierarchical decision making with agile-style accountability is problematic. If you mix agile and hierarchical management, it's possible that people will have a different understanding of where one ends and the other begins, and it's thus very easy to end up with a model where decision rights and accountability are not aligned.

But back to you: Don't take responsibility for things you can not fix, but direct people to those who can. For instance like this:

Yeah, that sucked. The problem is that, according to policy, Joe is the only person allowed to merge changes, and he didn't get around to it. I hear he's been very busy with X ...

if you like Joe, you can also give him a heads up that product was pissed.

And if you see something headed for disaster, warn the person who can fix it:

Hey Joe, have you seen my PR for issue #1875? According to JIRA, it's still unmerged, but scheduled for the current sprint ...?

or

Hey Joe, have you seen my PR for issue #1875? I need your feedback by ... if we are to deliver it this Sprint.

(and if that person has a habit of throwing you under bus, you may wish to proactively inform the person who will be affected about the handover of responsibility, so they can follow up as needed)

0

Before you escalate into a potential confrontation with the team-lead, ask peers on your team if they have the same issue with late reviews. Maybe they have some tips for getting the team-lead's attention when it's important, maybe having to wait a while on reviews is "normal"/"acceptable" in your company, or maybe they also have the same issue and you can escalate together, increasing your credibility and minimizing your individual risk.

1
  • The way the question is phrased clearly suggests that this is something to escalate with the team lead, as the cause is policy imposed from above or him being overtasked. Mar 17 at 7:47
-1

I'm gonna come at this from another direction and say the biggest problem is not your company policy; it's your branching system. It sounds like your team tends to have pull requests into Main which is built into your release packages, and the volume of PRs is exacerbating your manager shortage.

Ideally Version control branches should go some manner of {Feature} → {Develop} → {Release} but some common branch types are:

  • Feature: A branch for working on a specific thing, ideally tied to a task on the board. No PR needed
  • Epic/MajorFeature: Ties to an Epic instead of a task. PR should include subject experts or other devs working the same epic
  • Develop: The communal starting branch. PR includes the whole team.
  • Staging: For when the team isn't allowed to merge to release without some extra steps. PR should be the whole team and Product Owner
  • Release: The Immutable Branch ready for Release.

At your next standup, ask your teammates if you can start using more interim Branches to consolidate your work. The larger PRs will look intimidating but will be easier to review than a pile of pulls. It also creates a review and retrospective cycle. Talk with your manager and find out where he wants to come into this. They might say they want to approve everything going into develop and deal with Major Feature PRs, or maybe just the total review at the end. If Major Features are independent of each other it may be best not to merge them until they are Validated by your Product owner/Customer at the end of sprint.

As for the scrum side of things, you've fallen into a fairly common scrum trap, your scrum manager has become more like a product owner. So somebody has to step up and do more of the scrum management. If nobody else is scheduling backlog grooming, reviews, or retrospectives then you just schedule them.

Scrum is for the team and Dark Scrum is for management, effective Scrum Leaders are slowly pushed into management which leads to Dark Scrum if no one steps up to take over.

8
  • 1
    When the source of a problem is excessive ceremony (in this case, having everything go through PRs that only a busy team lead can merge), adding more ceremony is usually not going to improve things. Nor are larger PRs generally easier to merge; more often they are more difficult to merge. Justin Ohms's answer gives much better advice on making merges easier.
    – cjs
    Mar 17 at 0:27
  • @cjs Larger Reviews are harder, but in scrum they have to be done. Mid sprint everyone merges to Develop with verified code, but nobody touches Release until the PO validates that code which is supposed to happen at sprint review. Its a step that has to happen anyways. Justin makes a good point that Major feature -> dev is also a great place to ask for a validation PR.
    – Sir Demios
    Mar 18 at 17:23
  • No, you do not have to do scrum that way. Some (including me) would argue that such heavyweight ceremony is exactly what agile is trying to avoid. (In my projects, we release a lot more often than once per sprint.)
    – cjs
    Mar 20 at 7:11
  • @cjs, yeah you can merge to release more than once a sprint but I don't own that branch, the PO does. and I agree that agile is supposed to simplify your life but being on a team that meets 15 minutes every morning and never talks sucks; half the team is duplicating effort, the other half is making the wrong thing, no one knows what we are actually making and its hard to care about it because it doesn't matter. Planing, Review, Standup. its not excessive ceremony, its the minimum that keeps you from wasting 6 months on a product no one wanted.
    – Sir Demios
    Mar 20 at 21:22
  • You have described two agile anti-patterns. 1) Having anything "owned" not only introduces bottlenecks and discourages sharing of expertise and responsibility, but also discourages a global view of the system. Feature changes, bug fixes and even refactorings often require changes across multiple parts of the system, such as in both code and database schema. The story owner should be the one managing this and understand reasonably well all parts of the system he's changing, getting help from others when he needs education, expertise or review of the changes.
    – cjs
    Mar 22 at 5:41

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .