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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃彈 Updates existing OWNERS.yaml files with brace-set and modifier syntax1 #24599

Merged
merged 6 commits into from Sep 23, 2019

Conversation

rcebulko
Copy link
Contributor

Upgrades to the owners bot parser include support for brace-set expansion, ie *.{js,css}. This PR updates currently-duplicated rules to use this syntax.

@rcebulko rcebulko changed the title 馃彈 [DO NOT SUBMIT] Updates existing OWNERS.yaml files with brace-set and modifier syntax1 馃彈 Updates existing OWNERS.yaml files with brace-set and modifier syntax1 Sep 23, 2019
@rcebulko rcebulko requested review from rsimha and cramforce and removed request for rsimha September 23, 2019 14:26
- ampproject/wg-infra
- ampproject/wg-runtime
- ampproject/wg-performance
- "**/OWNERS.yaml": "#rcebulko"
Copy link
Member

Choose a reason for hiding this comment

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

Is this giving you the power to change all owners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea behind this was that it would be here only temporarily during this transition period where many owners files are being updated as contributors and teams notice which files are out of date. The # will tag me on any PR that touches an owners file, so I can verify syntax and semantics of all updates until I finish the syntax check API for Travis to use (which would ideally happen this week but since I'm build-cop this week it depends).

Technically speaking, it means I could theoretically make myself an owner of something (I will not). Practically speaking, I do not have merge access on this repository (so someone else would have to actually review and merge it anyway), the owners bot is non-blocking currently, and I run and maintain the actual owners bot itself, so from a security perspective it seemed moot. I will remove if you'd prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea behind this was that it would be here only temporarily during this transition period where many owners files are being updated as contributors and teams notice which files are out of date. The # will tag me on any PR that touches an owners file, so I can verify syntax and semantics of all updates until I finish the syntax check API for Travis to use (which would ideally happen this week but since I'm build-cop this week it depends).

Technically speaking, it means I could theoretically make myself an owner of something (I will not). Practically speaking, I do not have merge access on this repository (so someone else would have to actually review and merge it anyway), the owners bot is non-blocking currently, and I run and maintain the actual owners bot itself, so from a security perspective it seemed moot. I will remove if you'd prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Please paste a version of this comment as a comment into the yaml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 馃憤

Copy link
Member

Choose a reason for hiding this comment

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

One more thing (sorry), please also create and add here an issue for cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Created #24685 and added a TODO here.

Copy link
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

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

LGTM with comment

- ampproject/wg-infra
- ampproject/wg-runtime
- ampproject/wg-performance
- "**/OWNERS.yaml": "#rcebulko"
Copy link
Member

Choose a reason for hiding this comment

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

One more thing (sorry), please also create and add here an issue for cleanup.

@cathyxz cathyxz merged commit b8ab270 into ampproject:master Sep 23, 2019
@rcebulko rcebulko deleted the updates branch September 23, 2019 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants