[owners] Support no-notify and always-notify owners about amp-github-apps HOT 9 CLOSED

Support for # and ? has been added.

from amp-github-apps.

I like the ? syntax - this was a problem we had with the bundle-size bot where we wanted some people to be able to review PRs, but not have them auto-added as reviewers.

The ! syntax is also useful, but I'm not sure whether the !! syntax is a little over-engineery. I can't imagine a reasonable scenario where a ! user is notified, says nothing, and then regrets it after the PR has been merged by other OWNERS

from amp-github-apps.

@danielrozenberg Suppose languageperson should have veto power over any *.protoascii file changes, and I'm an owner of extensions/amp-foo. I could create extensions/amp-foo/stuff.protoascii, get a review from anyone, and submit (since I own it, I can approve it). In that case, it could be submitted before languageperson ever gets the chance to see it or approve/reject.

I agree it's a corner-case, and probably worth holding off on implementing !! until we see a need arise.

from amp-github-apps.

For *.protoascii files I believe !someuser would be sufficient. Being aware of an incoming change to validation rules is helpful but we don't always need to be gatekeepers of it as some changes are fairly straight-forward.

from amp-github-apps.

@honeybadgerdontcare I'm a little unclear now; your comment here suggests you would not need a "require-approval" gatekeeping syntax; whereas on this PR you suggest you'd look forward to/need the ability to require approval. Would you mind clarifying how important each is to you/your subteam?

from amp-github-apps.

Sorry for the confusion.

At this point in time I do not believe we need to be gatekeepers of extensions/validator-*.protoascii but only be informed of them. If in time we find it necessary to be gatekeepers of extensions/validator-*.protoascii then we would want to use !!someuser syntax.

Typically validator rules review take less time than the javascript review so we have ample time to review it. Further these rules don't compile into the production javascript until they've been imported internally and compiled to validator.js. That allows us to have a second opportunity to review them then.

Any changes to validator/... we would be gatekeepers of because that could involve changes to the validator engine.

from amp-github-apps.

Excellent, thanks!

from amp-github-apps.

Since there is a need for an "always-require" modifier, meaning a file requires approval from a particular owner even when they have approval from another owner, the planned syntax will be updated as follows:
! - require review
# - always notify/tag
? - never auto-add reviewer

from amp-github-apps.

@rcebulko Thanks for working on this!

from amp-github-apps.