Skip to content

[WIP - no merge] Update add label based on code owners #1772

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RissyRan
Copy link
Collaborator

@RissyRan RissyRan commented May 23, 2025

Description

Update the add label file based on recent code owner change: PR

# out of date
**Please note**: the current change that includes all approvers from code owner file. If any approver from other directories approves the PR, I think the `pull ready` label will also be added. Will this trigger the merge if LGTM to the G3 cl afterwards?

Tests

N/A

Checklist

Before submitting this PR, please make sure (put X in square brackets):

  • I have performed a self-review of my code.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have run end-to-end tests tests and provided workload links above if applicable.
  • I have made or will make corresponding changes to the doc if needed.

@gobbleturk
Copy link
Collaborator

gobbleturk commented May 23, 2025

Just a note to myself/readers, took some time for me to figure out - This PR relies on the per - file owners being added to required owners via your per-owner PR - #1690?

We have this key conditional

if (review.state === "APPROVED" && review.user.login in requiredReviewer)

This isn't perfect - for instance if someone writes a PR that touches both moe.py and pipeline.py and I approve it, it will get pull ready since I own pipeline.py despite the fact that no moe.py owners reviewed.

@RissyRan RissyRan changed the title Update add label based on code owners [WIP] Update add label based on code owners May 23, 2025
@RissyRan
Copy link
Collaborator Author

RissyRan commented May 23, 2025

Just a note to myself/readers, took some time for me to figure out - This PR relies on the per - file owners being added to required owners via your per-owner PR - #1690?

We have this key conditional

if (review.state === "APPROVED" && review.user.login in requiredReviewer)

This isn't perfect - for instance if someone writes a PR that touches both moe.py and pipeline.py and I approve it, it will get pull ready since I own pipeline.py despite the fact that no moe.py owners reviewed.

Yes. That's my concern in my PR description note. Running some manual tests with my current change.

@RissyRan RissyRan changed the title [WIP] Update add label based on code owners [WIP - no merge] Update add label based on code owners May 23, 2025
Copy link
Collaborator

@bvandermoon bvandermoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RissyRan asked me to add my approval to test the workflow. I am not the owner of this file, so we want to see if the Pull Ready label is added. I can take another look once things are ready if needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants