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

Add ampproject/wg-ads as OWNERS of ads related code. #24714

Merged
merged 7 commits into from Sep 30, 2019

Conversation

lannka
Copy link
Contributor

@lannka lannka commented Sep 24, 2019

No description provided.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Sep 24, 2019

Hey @rcebulko, these files were changed:

  • 3p/OWNERS.yaml
  • ads/OWNERS.yaml
  • extensions/amp-ad-custom/OWNERS.yaml
  • extensions/amp-ad-exit/OWNERS.yaml
  • extensions/amp-ad-network-adsense-impl/OWNERS.yaml
  • extensions/amp-ad-network-adzerk-impl/OWNERS.yaml
  • extensions/amp-ad-network-doubleclick-impl/OWNERS.yaml
  • extensions/amp-ad-network-fake-impl/OWNERS.yaml
  • extensions/amp-ad-network-gmossp-impl/OWNERS.yaml
  • extensions/amp-ad/OWNERS.yaml
  • src/inabox/OWNERS.yaml

ads/alp/OWNERS.yaml Outdated Show resolved Hide resolved
@rcebulko rcebulko requested a review from oliy September 24, 2019 21:00
@rcebulko
Copy link
Contributor

Added @oliy to verify the addition to extensions/amp-ad-network-cloudflare/OWNERS.yaml
Since 3p/OWNERS.yaml is new, it falls under the top-level OWNERS file. I can give coverage, but if you could get a thumbs-up from Joey or one of the top-level OWNERS, that would be preferred.

@lannka
Copy link
Contributor Author

lannka commented Sep 24, 2019

@rcebulko regarding extensions/amp-ad-network-cloudflare/, what's our general OWNER policy for 3rd party integration in AMP? we could by default have wg-ads as owner of all ads 3P integration.

@rcebulko
Copy link
Contributor

@lannka Can you give me a bit more context on your question? I'm not fully familiar with 3rd party integrations (or what 3P is, or the structure of ads integrations). Some extra info would help me either provide an answer or loop in someone who knows :)

@lannka
Copy link
Contributor Author

lannka commented Sep 26, 2019

sure. It's very common for Ads & Analytics vendors to implement their tags in AMP. We provided generic extensions such as amp-ad, amp-analytics, or extendable extensions such as amp-ad-network-* for these kind of integration. Those contributions are not completely owned by individual vendors, ampproject/wg-ads should be free to make changes such as refactoring or internal API changes.

@rcebulko
Copy link
Contributor

For the time being, ensuring ampproject/wg-ads is in the OWNERS.yaml file for each of these integration directories will serve your purpose. If your intent is to preemptively establish ownership of any existing or new directory following the amp-ad-network-* pattern, I'd suggest filing a feature request for adding support for directory globs. They were not implemented in the first pass as it was unclear if they would be necessary or would add additional/undesirable complexity to owners rules. Discussion over use cases, tradeoffs, and priorities can then take place on that feature request tracking issue. Thanks!

@lannka lannka requested a review from szach September 26, 2019 17:22
@lannka
Copy link
Contributor Author

lannka commented Sep 26, 2019

@oliy @szach for approval

@lannka
Copy link
Contributor Author

lannka commented Sep 26, 2019

will do the changes for these 2 networks in separate PRs.

@lannka
Copy link
Contributor Author

lannka commented Sep 26, 2019

This all looks good to me! I don't have authority to approve the changes in 3p, but all other files you already own 👍

@lannka
Copy link
Contributor Author

lannka commented Sep 27, 2019

@rcebulko I guess you accidentally changed my comment. And I still need your approve to be able to merge it.

@rcebulko
Copy link
Contributor

@rcebulko I guess you accidentally changed my comment. And I still need your approve to be able to merge it.

Odd, I'm not sure how that happened! Regardless, while I'm technically an owner of owners files (at least, during the transition to fully using them), I don't actually have authority to approve them (just reject them in the case of syntax errors). You are an owner of all other files, but 3p/OWNERS.yaml needs approval from one of dvoytenko, cramforce, or jridgewell.

@lannka
Copy link
Contributor Author

lannka commented Sep 27, 2019

@dvoytenko for review

@lannka lannka merged commit d280242 into ampproject:master Sep 30, 2019
@lannka lannka deleted the OWNERS-ads branch September 30, 2019 22:34
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