Amphtml: Require OWNERS approval from all formats that a component is used in

[caroqliu]

Describe the new feature or change to an existing feature you'd like to see

Most components have a primary owner, i.e. amp-story by @ampproject/wg-stories or amp-consent by @ampproject/wg-analytics, with most non-specialized components defaulting to @ampproject/wg-ui-and-a11y as the OWNER.

There are some components that are used by multiple formats:

• amp-social-share by @ampproject/wg-stories
• amp-list by @ampproject/wg-amp4email

We should modify all extensions/ OWNERS files to reflect: if amp-foo is used by a format, any changes to it should require approval by a member of _each_ corresponding WG. This is because ultimately, any changes to a component concern all of its enabled formats.

Additional context

27826 involved a breakage in Stories for a critical user journey that relies on amp-social-share. The offending PR #27533 modified the component without knowledge of this dependency. The ensuing P0 could have been avoided entirely if any member of the stories WG had been notified to sanity check the change within their format.

Note: This recommendation solves a symptom not a root cause, and does not replace the need for robust integration/e2e testing for all these dependencies.

[choumx]

I think we already have a notify feature e.g. to inform the conduit team of validation changes.

[caroqliu]

Yes, either notify or required will work for this. No need for any additional work from the infra side to support this. The issue is mainly to discuss between current and potential component OWNERS and reach consensus on a change. i.e. If we go with required we can have higher confidence with a potential trade off of elongating the code review period for a given component that's enabled in many formats.

[rcebulko]

Something in the past month resulted in Owners Bot team @ mentions not working properly, so unfortunately notify isn't WAI for teams at the moment. I'm working on debugging why it stopped, hope to have it figured out by end-of-week.

[rsimha]

I'm working on debugging why it stopped, hope to have it figured out by end-of-week.

Whatever you did seems to be working, because I can confirm that owners-bot team mentions are working as of this evening 😃

Yes, either notify or required will work for this.

This will have to be required to guarantee a review from each of the working groups involved.

[caroqliu]

@ampproject/wg-ads-reviewers Thoughts? Y'all would get many more PR review requests for your enabled components.

[calebcordry]

Sorry I don't fully understand, we would get notified for every change to a component that might be used in amphtml ads?

[powerivq]

That seems like too many PRs for ads-wg to be able to handle, so I do not know how much it would actually help. Maybe we should have a new Github group and look them up once a week. That could be the right balance. @calebcordry ?

[caroqliu]

@calebcordry Yes that's right, the idea is that while UI changes to components have a high threshold for quality web experiences, we cannot exhaustively account for quality integrations. i.e. If one format relies on amp-foo to do something implicitly, UI does not necessarily know to keep track of this, much less that we should uphold this guarantee.

It does sound like you prefer notify to required so that the review is non-blocking. The main difference is notify means silence from the notified group => approval. Still the intention of having either of these designations is to put explicit onus on notified formats to sanity check that the change does not interfere with any implicit reliance that the UI WG may not be aware of.

Another solution to this is to train UI members to become experts in all formats, but I'm not aware of this being feasible at this time. 😅

[calebcordry]

Yeah that sounds like a reasonable tradeoff to me. I could see this being basically every PR.

[choumx]

IIUC, I don't think the proposal is tenable at scale. The same problem would apply to wg-amp4email for components used in AMP4EMAIL.

I'm also wary of extrapolating a new bureaucratic process from a single data point.

One practice I'd recommend (which I think would've been possible in the amp-social-share bug) is for API authors to survey for internal usages via code search. Creating UI components internally should be pretty rare in practice, so this would be much less costly than "review all PRs".

@ampproject/wg-runtime actually does this constantly to avoid breaking callers of internal APIs. We'd be happy to give some pointers there.

[caroqliu]

@choumx Thanks for weighing in here. Since this is getting mixed reviews I'd recommend @ampproject/wg-stories folks add notify or required as they see fit on their format components since this is the only group that expressed interest in the change.

Closing the issue, feel free to re-open if folks want to discuss further.