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

🐛Require src attribute in amp-ad-custom #27624

Merged
merged 1 commit into from
Apr 8, 2020

Conversation

powerivq
Copy link
Contributor

@powerivq powerivq commented Apr 7, 2020

#27614

This requirement should be mandatory but is missing from validator spec. We do not expect to see a large audience encounting problems with it since it would never work.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Apr 7, 2020

Hey @ampproject/wg-caching, these files were changed:

extensions/amp-ad-custom/validator-amp-ad-custom.protoascii

@powerivq powerivq force-pushed the amp-ad-custom-validator branch from e8a1edc to 7268b3e Compare April 7, 2020 20:32
@calebcordry
Copy link
Member

to/ @ampproject/wg-caching for final approval

@rcebulko the amp-owners-bot @ comment is not bolded, is it still working?

@rcebulko
Copy link
Contributor

rcebulko commented Apr 7, 2020

@calebcordry That's very odd; I'm certain it has worked for groups in the past (though there are TONS of GitHub issues out there trying to address this exact issue of team mentions like probot/probot#337).

I'm not sure why this changed, but based on behaviorbot/welcome#17, it looks like maybe this permission can be re-enabled at the repository level.

@rsimha When you have a few minutes to spare, can you see if there is an Organization members setting at https://github.com/organizations/ampproject/settings/apps/amp-owners-bot/permissions ? I found that URL at the link above. I've checked all app-level settings that I can access through ampprojectbot, and it seems to me like the app should have Org Member read permissions, but maybe something in the installation settings is blocking that

image

If so, can you set it to read-only please?

@rsimha
Copy link
Contributor

rsimha commented Apr 7, 2020

@rsimha When you have a few minutes to spare, can you see if there is an Organization members setting at github.com/organizations/ampproject/settings/apps/amp-owners-bot/permissions?

I am no longer admin for the ampproject organization (I was at one point). Looks like @mrjoro or @cramforce are the only people with this permission.

@cramforce
Copy link
Member

cramforce commented Apr 7, 2020 via email

@powerivq powerivq requested a review from Gregable April 8, 2020 17:58
@powerivq powerivq merged commit 78dbabd into ampproject:master Apr 8, 2020
@powerivq powerivq deleted the amp-ad-custom-validator branch April 8, 2020 18:09
zhouyx pushed a commit to zhouyx/amphtml that referenced this pull request Apr 9, 2020
twifkak added a commit to twifkak/amphtml that referenced this pull request Apr 15, 2020
@twifkak twifkak mentioned this pull request Apr 15, 2020
twifkak added a commit that referenced this pull request Apr 16, 2020
* cl/305561778 Revision bump for #27624

* cl/305733898 CSS validation changes.

* cl/306335254 Revision bump for #27709

* cl/306343181 Fix javascript validator symbol export.

* cl/306685743 Revision bump for #27768

Co-authored-by: Greg Grothaus <[email protected]>
@rsimha
Copy link
Contributor

rsimha commented Apr 25, 2020

@rsimha When you have a few minutes to spare, can you see if there is an Organization members setting at github.com/organizations/ampproject/settings/apps/amp-owners-bot/permissions ? I found that URL at the link above. I've checked all app-level settings that I can access through ampprojectbot, and it seems to me like the app should have Org Member read permissions, but maybe something in the installation settings is blocking that

To close the loop on this, I noticed that owners-bot @mentions are working again as of this evening.

@rcebulko
Copy link
Contributor

Yip-yip! Fixed in ampproject/amp-github-apps#796

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

Successfully merging this pull request may close these issues.

8 participants