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.

Sign up for GitHub

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

Jump to bottom

Revert amp-story-access validation. #26306

Merged
merged 1 commit into from
Jan 14, 2020
Merged

Revert amp-story-access validation. #26306

merged 1 commit into from
Jan 14, 2020

Conversation

gmajoulet
Copy link
Contributor

Revert amp-access and amp-story-access validation within AMP Stories. It was never documented or communicated on.
Recipe 534681 by @newmuis should tell us more about its usage before we merge this PR.

#12180

@amp-owners-bot
Copy link

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

  • extensions/amp-story/0.1/test/validator-amp-story-cta-layer-error.out
  • extensions/amp-story/1.0/test/validator-amp-story-access.html
  • extensions/amp-story/1.0/test/validator-amp-story-access.out
  • extensions/amp-story/1.0/test/validator-amp-story-amp-experiment-error.out
  • extensions/amp-story/1.0/test/validator-amp-story-cta-layer-error.out
  • extensions/amp-story/validator-amp-story.protoascii

@honeybadgerdontcare
Copy link
Contributor

Please follow the process of Intent-to-Remove (I2R) documented under I2D: https://github.com/ampproject/amphtml/blob/6d103b5603feab284d247bca132dc08064659b14/spec/amp-versioning-policy.md#deprecations

Have you verified that no site is using this?

honeybadgerdontcare
honeybadgerdontcare requested changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

We need to follow the I2D/I2R process for this and verify it does not make currently valid documents to become invalid.

extensions/amp-story/validator-amp-story.protoascii Show resolved Hide resolved
@gmajoulet
Copy link
Contributor Author

As long as no story is using this (will be confirmed by the recipe mentioned in the first comment), it should be safe to merge this without following the I2D/I2R since it hasn't been documented or announced to anyone.
Let's wait for the results.

@honeybadgerdontcare
Copy link
Contributor

@gmajoulet What recipe are you using? Are you sure that recipe covers all sites? Whenever we become more restrictive we must be sure about this which is why we created the I2D/I2R because we've had issues in the past.

@gmajoulet
Copy link
Contributor Author

Recipe 534681 shows 0 URL using amp-story-access, which we expected as we didn't announce or document the feature in any way. Please let me know if you want us to still go through the I2D/I2R process. We thought it might introduce confusion as it's never been announced, but are happy to follow the usual process if you think it's worth it.

@honeybadgerdontcare
Copy link
Contributor

I think you're right that it's not going to affect publishers. If you don't mind I'll run a sanity check to the recipe on a 10k set tonight.

@gmajoulet
Copy link
Contributor Author

Sounds good, thank you for your help!

honeybadgerdontcare
honeybadgerdontcare reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

lgtm for validation given test results

@gmajoulet
Copy link
Contributor Author

Re-requested a review, it was still marked as "changes requested" :)

honeybadgerdontcare
honeybadgerdontcare approved these changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

Approved for validation changes

@gmajoulet gmajoulet requested a review from Enriqe January 14, 2020 16:25
@gmajoulet gmajoulet merged commit ad1f1af into ampproject:master Jan 14, 2020
@gmajoulet gmajoulet deleted the revert_amp_story_access branch January 14, 2020 16:46
banaag pushed a commit to banaag/amphtml that referenced this pull request Jan 21, 2020
@amaltas @banaag
cl/289781117 Revision bump for ampproject#26306
1aa0b4f
@banaag banaag mentioned this pull request Jan 21, 2020
Merged
banaag added a commit that referenced this pull request Jan 21, 2020
@banaag @twifkak @amaltas
Validator rollup (#26420)
2134f78 
* cl/288597747 Revision bump for #25741

* cl/289781117 Revision bump for #26306

Co-authored-by: Devin Mullins <twifkak@google.com>
Co-authored-by: Amaltas <amaltas@amaltas.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare approved these changes

@Enriqe Enriqe Enriqe approved these changes

@newmuis newmuis Awaiting requested review from newmuis

@jackbsteinberg jackbsteinberg Awaiting requested review from jackbsteinberg

@Gregable Gregable Awaiting requested review from Gregable

Assignees
No one assigned
Labels
cla: yes PR use: In Beta / Experimental PR use: In Stable WG: caching
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gmajoulet @honeybadgerdontcare @Enriqe @googlebot @ampprojectbot