How to handle the situation as a CodeOwner and push back Large Pull Request from External Team?

Profile picture
Anonymous User at Taro Community6 months ago

How can I handle a situation where I am frequently asked to review Pull Requests for our large monolithic application that powers web, mobile, and platform services, but some of these requests involve over 2000 lines of code and 53 modified JavaScript files from teams outside my own, and I lack context on the new features being added?

Despite declining these requests and suggesting alternative solutions, such as recommending another engineer with more familiarity with the feature, the web team still approves these large Pull Requests. How can I effectively communicate my concerns about the code quality and potential issues or bad coding practices within these large PRs, and what steps can I take to ensure that the appropriate team members are reviewing and approving them?

Context about me & external team: I'm a senior engineer and one of the code owners. I'm responsible for backend changes for API, but I am often asked to review Pull Requests coming from different changes also for Web (React, TypeScript). I noticed a lot of Pull Request coming to me since our codebase is 1 large monolithic application that powers web, mobile, and backend platform services, and it is stored in 1 GitHub Repro.

The external team are people who I do not work directly with and they are new to our codebase, but their team is pushing out new features as new major initiative from another Team and the stake are high for their project to be roll out. Lately, there's multiple bugs introduced from the external team, and it causes Production issues. And I'm getting buy-in from my Directors that we need to lay down some rules for changes from the external team.

1 Like
85 Views
3 Comments
👑

Discussion

(3 comments)
  • Profile picture
    Senior Software Engineer and Career Coach
    6 months ago

    Hey there! This is a great question I'd love to weigh in on. I know this got partially answered during office hours so I'll reiterate for posterity + add some thoughts of my own.

    During office hours (and I 100% agree):

    • Set up a framework / process / policy for contributing to your area of the code. It's so much easier to defer to policy as a rule to follow rather than getting in a verbal or typing battle with someone
    • Make it easier to contribute in a way that doesn't lead to bugs. As Lee mentioned, depending on what's being added, you may be able to allow them to add code elsewhere that "plugs in" to your system without it affecting much of your code. This may be a win on both ends. You don't need to maintain it or review their code, and they don't need to view you as a blocker.

    Here's what I'll add regarding communication:

    • Be ok with being the gatekeeper. Just do so respectfully but sternly. Given you are a senior engineer, it's your responsibility to safeguard the code from bad patterns and bugs, especially from external teams. I can't imagine the external teams challenging that authority at all. So the only question then is how to approach that safeguarding mentality...
    • Be as collaborative and understanding as possible. If they are adding features or at least making an attempt to make your system better, they are making an active step to avoid putting that work on you as a feature request. This should be viewed as a positive thing and you can have the mindset that they are a good samaratan that just needs a bit of guidance. For example, thank them for their contribution and the time they spent on what they are trying to add. After thanking them, suggest having a chat over zoom so you can get a better understanding of the change and go through it together. In zoom, communication will be much easier than typing. In zoom, you can bring the topics up like you being looped in earlier so that you can have ample time to review and its not a surprise to you. Speak from your perspective on this and how it has affected you, and don't make it about them. Make it about the behavior / action that has occurred of a large review being sprung onto you. Speak about it in a way that they will empathize with. Doing that would hopefully create a more collaborative and understanding relationship. You can then ask them given that, what suggestions would they have for a process that would be beneficial for both of y'all. What is it that you can provide that they need? And you can offer up a suggestion or two.

    That last bullet spiraled out of control a little bit, but I just wanted to nail in the mindset of the collaborative relationship, and you are just looking for a process or way that you two can work together that will benefit both of you.

    Hope this helps! Best of luck.

    2 Likes
  • Profile picture
    Engineering Manager at Blend
    6 months ago

    Here's what I would do in your situation:

    1. Revisit the code owners file – to me, doesn't make sense for backend folks to be approving frontend PRs.
    2. Teach why 2000 LOC for PR is generally a bad idea – things aren't usually black/white situations where it's completely good or completely bad... there's nuance and discernment needed to be practiced. Maybe the external team is pushing huge PRs because they don't fully understand pros/cons of big PRs. (this can be done via email, presentation, team meeting, or any combination of these)
    3. Teach how to have smaller PRs – related to above, maybe the external team doesn't know good practices on how to split up a bigger feature/PR. Take some existing 2000 LOC PR, and give examples of how you would've broken it down into separate chunks.
    4. Repeat above this multiple times, have patience – it's hard to change behavior, unlikely to change behavior after you've said something once / taught it once... lower expectations to change people's behavior... generally takes multiple reminders over long periods of time. Recognize how difficult it is for you to change quickly so that you give people the same amount of patience to change as you would yourself.
    1 Like
  • Profile picture
    Mid-Level Software Engineer [SDE 2] at Amazon
    5 months ago

    I definitely 100% agree with the previous answers, however, I am also interested to find out why weren't the bugs caught in pre prod environments. Obviously, the huge PRs make it difficult to review code, but even then, I don't think the responsibility of catching bugs is on the code review process. If the code deploy pipeline does not have enough integration tests or the repo itself does not have enough unit tests, this is a good opportunity to invest time in writing those. Apart from enforcing a process in PRs, there should also be focus on improving the code deployment process. Practices like integration tests in pre prod, canaries in prod, and proper instrumentation for catching anomalies earlier on will also help in this scenario.

    And, though it won't help your situation immediately, I would strongly recommend to carry out a repo split into multiple more manageable repos. Since you are a senior engineer, you should try to convince your team and management to invest in such an effort, as this will simplify ownership and also promote the velocity of roll outs in the future.

    1 Like