27

I work in a small company, and my manager is involved in managing 2 projects as well as doing his own coding. I am increasingly frustrated as there is a queue of 50 Pull Requests waiting to be either reviewed or merged from another developer and myself. I review my coworker's PRs to make it easier for my manager. He doesn't make reviewing/ merging PRs his priority and once or twice a week he will do a few PRs, but it doesn't keep up with the current development and increasing backlog.

It's very frustrating having all mine and my coworker's work sitting in the PR list for several months.

I have offered to help with merging the pull requests, but he doesn't want me to do it. Yet, he doesn't have time or make time to keep up with the list. Do you have any suggestions?

Improve this question
2
  • 1
    Comments are not for extended discussion; this conversation has been moved to chat. – Neo Nov 22 '19 at 12:46
  • Followup: after having a discussion with him, where he said that he wants to review and test all the code before approving the merge, we instituted a weekly code review meeting where we go through the previous week's code and some of the backlog and then make the appropriate changes and review again the next week. Hopefully this will help at least partially.. – Frustrated Nov 28 '19 at 7:26
46

For the non-software developers: As a software developer, you typically write code that exists only on your machine, and then when you think it’s fine, you issue a “pull request”, and someone reviews it, and either asks for changes to be made, or combines it into your company’s product. As long as the ”pull request” isn’t handled, it’s as good as if the work had never been done. Plus other developers can’t build on top of that work. And QA is blocked because they have no chance to test your work.

Pull requests should be handled on the same or the next day, except in exceptional circumstances. 50 pull requests waiting is absolutely ridiculous. If your manager doesn’t have the time, he either has to make the time, or handle the job to you and your colleague, or leave and the company finds a replacement for him. What happens at the moment is just unacceptable.

I suggest that you not offer to help him, but offer that these reviews should be your job and your colleague’s job. If this isn’t accepted, then go one level higher and complain about him.

Improve this answer
5
  • 2
    Whenever I offer to do the merges, he says "we'll look at it tomorrow" and then does a few PRs but as we're always developing more things, the backload doesn't get smaller. – Frustrated Nov 21 '19 at 13:03
  • 8
    @Frustrated That's why I said you shouldn't just offer to help, but make it your and your colleague's permanent job. – gnasher729 Nov 21 '19 at 20:04
  • 12
    Agreed. This is, frankly, gross negligence. – Lightness Races in Orbit Nov 21 '19 at 22:28
  • 1
    If he says "we'll look at it tomorrow", make it concrete, suggest a time "How about 10am?" – Martijn Nov 22 '19 at 10:12
  • Another suggestion is having these done regurarily during a designated time. E.g. once a week, 2-3 times a week, once a day - depending on the amount of pull requests over time, size of project, size of team etc. – Koenigsberg Nov 22 '19 at 17:35
41

It could help if you step back from the immediate issue and consider the "meta" nature of this. Pull requests are just a symptom.

What you have there is a manager who wants to keep all the power (even such a petty one as reviewing/approving code changes), does not want to delegate, yet can't or won't do the job himself.

This can happen in any aspect of the job, whether it's pull requests, software design, discussion about features, deciding and approving use of 3rd party components...

And it can also happen in non-software companies, in any area of activity.

So, the essence of the problem is: The manager should either do the job he did not delegate, do it on time and properly - or empower someone else to do it. That's it.

Improve this answer
5
  • 2
    This is the true answer. All the other answers I've read are good, but they don't address the underlying issue which this one does. If it hadn't already been written, I would have written a similar answer. The manager's problem also goes into still wanting to do dev work instead of managerial work. In that case, the manager needs to step down to a dev position and let someone else be the manager. – computercarguy Nov 21 '19 at 23:47
  • Yes, it does not help to "look at the open PRs tomorrow" as one comment stated the manager is telling OP. The process is broken and needs change. They need to discuss the process. (Meaning it's not feasible for the manager to handle all the PRs and needs to delegate) – kutschkem Nov 22 '19 at 10:47
  • 1
    "So, the essence of the problem is: The manager should either do the job he did not delegate, do it on time and properly - or empower someone else to do it. That's it." the question is what should you do if they don't? should you just live with it? should you just quit? should you take it up with your boss's boss (if your boss indeed has a boss) – Peter Green Nov 22 '19 at 11:31
  • @PeterGreen Most often, this gets reduced to the question "What to do if the company I work for sucks". And the most common answer is "find another job". In a small number of cases you can make the management see the light, but it's rare. – Dragan Juric Nov 22 '19 at 15:45
  • This answer may be correct, but it's not very practical as it provides no suggestions for OP on how to deal with this. Could you include some suggestions on either how the OP can convince the boss to do any of the things highlighted in your final paragraph or another alternative (e.g. find another job)? – Llewellyn Nov 22 '19 at 18:00
14

The problem is bigger that you manager's availability / willingness to review and accept / reject the pull request - if a pull request can be in the queue for 6 months to 1 year, that indicates, the work is not really relevant / useful and the priorities are wrong.

No work, important enough and short enough to be contained in one pull request, will be having a deadline of a year or more. Someone needs to have the work properly planned and prioritized.

That said, make sure

  • You properly put the onus on the reviewer. Have them informed over email about a PR you made and follow up with reminders for atleast 2-3 times.
  • You try to identify any alternate person for reviewing the change.
Improve this answer
3
  • Let's say that it's not so urgent, but it's just feature enhancements, why am I doing it? This is what they're telling me to do. – Frustrated Nov 21 '19 at 12:10
  • There's no one else to review the work. I review my coworker's but it doesn't help him speed up. Almost every day I ask him to sit and review pull requests, but I have to remind him several times and maybe he'll go through 6-7 a week. – Frustrated Nov 21 '19 at 13:01
  • @Frustrated No, don;t do that. Reminders are reminders, let the manager face the consequences for unpublished code in unattended PRs. Reminding them once or twice is fine, but don;t make it your job. – Sourav Ghosh Nov 21 '19 at 13:59
5

Arrange an open and honest discussion between your team and your manager. Talk about the business concerns - the features that you develop are not getting put in front of customers.

That's a real cost - the business is paying for those features, but the customers are not getting them until months after they were complete. This costs the business both money and market share, as their (good) competitors will not be making the same mistake, and will be positioning themselves as leaders in the market. Not a good situation.

The goal of the meeting should be to get some actions that the attendees can take to get those features in front of the customers as early as possible. Everyone should make suggestions for improvement, and no one should blame a particular person for the current situation.

Your boss may be slow at reviewing the pull requests, but it sounds like the process is what needs to be changed. You won't get your boss on-side for improvements to the process if he feels like he is under attack.

Improve this answer
2

There is approving a PR and then there is merging a PR. The general rule is that no developer can approve his or her own PR.

Merging is another story. I think developers should be able to merge their own PRs. I know that in some shops, only managers or leads can merge, but this is just a bottleneck to me. Merge is always gated by approval, so if reviewers are taking their role seriously, then anyone should be able to do the merge.

It sounds like you are in a situation where only your boss can merge, and your boss doesn't get around to doing that very often. I suggest asking your manager if he or she can configure the repository so you can merge your own PRs. That should eliminate the problem. From what you describe, I can't imagine you are in a shop with a true CI/CD system where changes go all the way into production, so this isn't asking too much.

Improve this answer
5
  • This doesn't really seem to get at the problem - merging is a largely mechanical task, but approval requires making a decision - and that is the real obstacle, both as a demand on the boss's time and focus, and in that whole areas of the asker's work may not be able to be continued until that decision has been made, or will have to proceed on the shaky foundation of turning a guess at the outcome into an assumption. – Chris Stratton Nov 21 '19 at 21:14
  • @ChrisStratton Approval is an issue only if the manager is the only one who can do it, but that's not the way it's usually set up. That's not peer review. Developers should be reviewing each other's PRs and ANY developer should be able to do it. It sounds like the OP can approve some PRs, but not others, which is confusing to me. I don't know why that is, but it shouldn't be. The OP should be able to approve anybody's PR, and maybe that is the issue. – Mohair Nov 21 '19 at 21:22
  • You seem to be entirely ignoring the reality that deciding if a particular commit or set thereof is the appropriate way to solve a problem is the hardest job in software engineering. Sure - it passes the tests that have been thought of, the style issues are fine. But is it strategically a good idea? When that's been declared to be a boss-level decision, team input may support, but it's a boss-level decision unless or until the boss choses to delegate responsibility and approval authority to an area maintainer. – Chris Stratton Nov 21 '19 at 21:31
  • @ChrisStratton That's not a reality I am familiar with. Deciding if a PR is strategically good is a decision that should have been made long before any code was written. That's what the design process is for. What you are talking about seems to apply more to an open source project where PRs come from unknown developers rather than a small company that is following some project management methodology and where everyone knows each other. – Mohair Nov 21 '19 at 21:43
  • Pull requests that cleanly implement an already agreed upon strategy without spurious inclusions are indeed easier to approve. There may be aspects to the story we're not getting; festering disagreements of strategy, pull requests that tend to have repeating quality issues the asker and their boss have not been able to come into alignment on, or indeed simple task-queue irresponsibility by the boss as widely alleged here. In the end though, someone has to own responsibility for the decision to approve and the consequences of that decision; the boss or lead having final say is not unusual. – Chris Stratton Nov 21 '19 at 21:50
2

Assuming you are using Git and have master branch and several feature branches, just create another branch where you keep merged features into, maybe call it integration. Whenever your boss gets to merge a few pull requests into master, you merge the master into your all-features-branches. This way you can be sure they still work together and there are no conflicts.

Improve this answer
3
  • 1
    Innovating approach, turning it all on its head, effectively turning master into a feature branch... but I don't think it's a genuine solution to the problem, especially when you consider that much of it is a management issue and likely has other symptoms. – Lightness Races in Orbit Nov 21 '19 at 22:31
  • yeah, but it would be all readily prepared already when the boss finally gives up control. – MIke Nov 21 '19 at 23:54
  • @Frustrated are the pull requests independent or des he need to merge a lot? also is he changing a lot of the code before merging or just reading it? all important factors when you start a discussion about shifting responsibilites to you guys – MIke Nov 22 '19 at 0:13
1

How much effort is it to merge these when they do get approved? I appreciate something small with no conflicts will be minimal, but when you have merge conflicts? What's the opportunity cost? (what could you have been doing instead).

What is the probable cost to the business of finding the one feature they suddenly need in a hurry cannot be completed for weeks because of the backlog?

This is to say, you need to present this problem as something the manager can understand - cost in $$ and the loss of flexibility.

I agree with @sourav Ghosh that this is not your real problem.

For comparison, we have an (informal) SLA for reviewers to start the review of 60 mins. It takes the time it takes.

For non IT folks, a PR (pull request / peer review) is a list of all the changes which a developer (the OP) has made. There may be one or two, like just changing the name of something, or hundreds of changes in dozens of places. We have tools which explicitly highlight the changes, so as a reviewer they're really obvious. It's not like playing spot-the-difference between two editions of a book.

The purpose of this is to check for any mistakes which the developer has missed or to query how or why something has been done. It's a sanity check.

Usually the tools can easily merge the changes with the master version, but sometimes they can't, for example when 2 developers have made different changes to the same thing. This is called a "merge conflict" and has to be resolved manually (costs time and money).

Improve this answer
3
  • 2
    I am the one who handles the merge conflicts, but it costs money when I have to keep on doing it to the same branch because it's been so long since I wrote it! – Frustrated Nov 21 '19 at 13:05
  • 1
    Exactly my point - it costs money. What you need to do is quantify and report it. – Justin Nov 21 '19 at 13:09
  • The longer something sits unmerged, the more it creates a conflict between work which has preceded in the meantime on the assumption that it will or will not be approved, with the opposite possibility. Later work in the same subject area almost invariably requires making an assumption on the resolution of previous, and if that is wrong, then months of work could need deep revision. – Chris Stratton Nov 21 '19 at 21:12
1

Time to get another job, and not for the reason you might think.

The obvious reason is that clearly your work is not valued, which is demotivating. And that is certainly a legitimate cause for concern.

But the flip side of that coin is, and I hate to say it but, clearly your work is not valuable.

If it were, then your boss would be under enormous pressure to get features/fixes pushed out the door, and you wouldn't have a 50 PR backlog.

People are smart [citation needed]. If having a 50 PR backlog was causing career issues for your boss, your boss would be reviewing pull requests in a timely fashion. Since your boss is not doing so, the software must not be terribly important to the company. And transitively speaking, by implication neither are you.

Now certainly it's possible that the software is important, but your company is such a criminally mismanaged quagmire that no one understands that point. Which is also a major red flag in its own right, so no help there.

I would recommend finding a job where you are part of the system bringing in the proverbial bacon, because programmers are fairly highly-paid for individual contributors [citation needed] and not being part of (or at least adjacent to) the revenue-generating machine makes you somewhat expendable.

Either way, best of luck.

Improve this answer
0

As a lead engineer on a fairly large team, at least, getting towards the bounds of what I think one person can manage, I've allowed a portion of our team (3 engineers) to operate in a more autonomous fashion. Part of that is that I put trust in them to review their own PRs and not require too much involvement from me. The caveat is that it is incumbent on me to ensure the direction of that smaller team is valid, I do this by checking the odd PR, not necessarily commenting/rejecting/approving and larger "in-place" code base reviews.

I think you need to have an honest private chat with your manager (an ideal conversation for your 1 to 1 meeting with him/her, if you have them) starting by reviewing all the other potential causes for this behaviour. Be self-effacing as this often has the effect of bringing out more honest thoughts from people in my experience, you can often get an insight into what they're dealing with, for example, the conversation could go something like...

We have this large backlog of PRs and the rate at which we're able to merge and release them is harming the business and our user's productivity (elaborate as much as you want here). I want to understand why we can't move faster on them and I'm concerned that it's because my code [and some other developer's code] isn't good enough.

At this point your manager may just straight up confess to being overloaded.

Perhaps there are other areas that you manager could use some help in but for whatever reason hasn't asked or delegated. It may be worth asking if there is anything you or any of the rest of the team can take of his/her plate.

Maybe step back and take some time to improve the review-ability of the existing PRs, try and make it as easy as possible to review. Is the PR too big? Too many moving parts? Could it be broken down? Could I add more explanations? Diagrams? GIFs showing the functionality working under test? This might be useful: https://medium.com/arnoldclarkdpd/better-github-pull-requests-87b2ef4f06c8

This manager perhaps feels a sense of obligation to the business that they must check everything before it goes out of the door. If so, it would be worth stating that as a team, in order to grow and learn (the business case allowing) you must be allowed to make mistakes. As much as you will strive to keep that to an absolute minimum, the individuals in the team should be allowed to step up and take responsibility and learn from their mistakes.

Improve this answer

You must log in to answer this question.

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