Skip to content

Lift frontend CODEOWNERS requirement #3155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Lift frontend CODEOWNERS requirement #3155

wants to merge 1 commit into from

Conversation

ammario
Copy link
Member

@ammario ammario commented Jul 24, 2022 •
edited
Loading

  • It's more difficult to get changes into the frontend during off hours (e.g weekend) due to the requirement. See ci: add skip directives for long tests #3151 for an example. That PR makes a minor, uncontroversial change to the frontend that may delay my merge by a day. I'm tempted to avoid improving the frontend in my free time, and perhaps other are too.
  • Lifting the gate will help encourage our traditional Go developers to make more changes to the frontend, which is good for cross functional empathy and overall velocity.
  • The backend doesn't have CODEOWNERS, and I don't get why we would want to be more protective about the frontend than the backend.

@ammario
Open up frontend PR reviews
3ea0f36 
It's more difficult to get changes into the frontend on off hours (e.g weekend) due to the requirement. Since the backend lacks a symmetrical requirement, I figured it would be OK to lift the requirement for frontend too. As an added benefit, I think lifting the gate steps us closer to a more cross functional team.
@ammario ammario requested a review from a team July 24, 2022 18:28
@ammario ammario changed the title Lift CODEOWNER requirement in frontend Lift frontend CODEOWNER requirement Jul 24, 2022
@ammario ammario changed the title Lift frontend CODEOWNER requirement Lift frontend CODEOWNERS requirement Jul 24, 2022
@presleyp
Copy link
Contributor

Arguments for frontend codeowners:

  • it makes for an efficient code review cycle in many cases. Someone makes a meaningful change to the frontend, so they want the people who know the codebase best to review it. Those people all get invited to review without anyone having to remember who all is on the team. Those people all get notifications via Slack if they've enabled that. Any one of them can review and fulfill the requirement, so whoever is free first unblocks the PR, rather than the implementer choosing someone who might not be free for a while.
  • FE and BE require different areas of expertise, and TS and Go have different coding conventions, so it's natural that code quality will suffer without review from those in the know. The v1 frontend has some misleading and difficult to refactor code and I think this is why. I don't think FE is special in this way; I would not merge a BE PR without a BE approval.

Proposal:

  1. Make the setting overridable if possible. Grey used to be under the impression that this was already the case. This way, we get the code quality and review efficiency benefits on a "meaty" FE PR, and we can move quickly on a PR that triggers the requirement in a trivial way.
  2. Maintain openness to adding to the "FE team." As we do more cross-functional work, more people may find that they have the expertise to function as a FE reviewer, and then we can add them to the list.

@Kira-Pilot
Copy link
Member

Seems like this would be good to have a quick discussion about in FE variety or office hours? I agree - FE review can be a bottleneck.

@ammario ammario closed this Jul 25, 2022
@github-actions github-actions bot deleted the codeowners branch February 4, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ammario @presleyp @Kira-Pilot