Skip to content
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

[owners] Support reviewerPool of users and/or teams in place of reviewerTeam property #1284

Merged
merged 7 commits into from
May 3, 2021

Conversation

rcebulko
Copy link
Contributor

@rcebulko rcebulko commented May 3, 2021

Closes #1283
Requires an update to amphtml's root OWNERS file, but this should not fail any PRs on that repo in the interim unless they trigger OWNERS build target. Will require pushing an updated deploy tag to trigger a build.

Before: reviewerTeam allowed only in repo root OWNERS file, must be a string representing a team
Now: reviewerPool allowed only in repo root `OWNERShi file, must be an array of strings representing teams or usernames

@rcebulko rcebulko requested a review from rsimha May 3, 2021 16:46
@rcebulko
Copy link
Contributor Author

rcebulko commented May 3, 2021

@rsimha Can you make the necessary changes on the amphtml side and I'll handle re-deploying this?

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix! LGTM with a few nits.

Happy to help coordinate this change with the one needed in amphtml.

owners/OWNERS.example Outdated Show resolved Hide resolved
owners/test/ownership/parser.test.js Outdated Show resolved Hide resolved
owners/OWNERS.example Outdated Show resolved Hide resolved
@rsimha
Copy link
Contributor

rsimha commented May 3, 2021

@rsimha Can you make the necessary changes on the amphtml side and I'll handle re-deploying this?

SG. I've sent out ampproject/amphtml#34176 for review. I can help coordinate the merging of it with this PR.

@rcebulko
Copy link
Contributor Author

rcebulko commented May 3, 2021

@rsimha FYI CircleCI migration may have introduced a bug in built target detection:
image

Locally I get the correct set of build targets, suggesting it may be due to the git merge-base main HEAD using the PR merge commit instead of the PR head commit.

@rcebulko rcebulko merged commit 9b5f51b into ampproject:main May 3, 2021
@rcebulko rcebulko deleted the reviewerPool branch May 3, 2021 17:17
@rsimha
Copy link
Contributor

rsimha commented May 3, 2021

Locally I get the correct set of build targets, suggesting it may be due to the git merge-base main HEAD using the PR merge commit instead of the PR head commit.

Could be due to GH actions. Thanks for the heads up. Will address this via a separate fix.

Edit: Addressed in #1307, confirmed in #1307 (comment).

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

Successfully merging this pull request may close these issues.

[owners] Add a mechanism to recognize specific bot accounts as legitimate reviewers
2 participants