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

Update generated tags file to 767 #1665

Merged
merged 3 commits into from Nov 29, 2018

Conversation

3 participants
@miina
Collaborator

miina commented Nov 28, 2018

PR for testing -- looks like updating the file makes the test fails.

@westonruter

This comment has been minimized.

Member

westonruter commented Nov 28, 2018

Updating the file from what state? The latest amphtml tag? I've identified an issue with the script where it is checking out the most recent tag and not necessarily the most recent stable release.

@miina

This comment has been minimized.

Collaborator

miina commented Nov 28, 2018

@westonruter I just checked out 1.0, run the amphtml-update.sh and committed the changes.

Was there anything else that should have been done?

@westonruter

This comment has been minimized.

Member

westonruter commented Nov 28, 2018

The last time I did it, I had to comment out these two lines:

https://github.com/Automattic/amp-wp/blob/b748977321cc3769d0476ceede56c39aa2fed267/bin/amphtml-update.sh#L35-L36

The reason being that the latest tag is not actually the latest stable release, as can be seen at https://github.com/ampproject/amphtml/releases

image

So I manually switched to the latest release tag when I did it. In practice this is probably overkill.

But as can be seen in ampproject/amphtml#19214, the amp-script component was commented-out a couple weeks ago. That is why amp-script is now failing in the unit tests. So we just need to remove the amp-script test.

@westonruter westonruter referenced this pull request Nov 28, 2018

Open

[amp-stories sub-task] Sanitize AMP Stories. #1614

1 of 1 task complete

kienstra added some commits Nov 29, 2018

Remove assertion for <amp-script>, as it's not allowed
As Weston mentioned, <amp-script> was made
invalid until its launch:
https://github.com/ampproject/amphtml/pull/19214/files#diff-7ceae6437161becf74c959b0e7bdaef2R17
So simply remove the assertion for it.

@kienstra kienstra changed the title from [WIP] Update generated tags file. to Update generated tags file to 767 Nov 29, 2018

@westonruter westonruter added this to the v1.0 milestone Nov 29, 2018

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Nov 29, 2018

Request For Review

Hi @westonruter,
Could you please review this? It applies your suggestion of removing the test for <amp-script>, and it adds a test for the newly-allowed element <amp-delight-player>.

Thanks, Weston 😄

@kienstra kienstra requested a review from westonruter Nov 29, 2018

@kienstra kienstra added this to Ready for review in v1.0 Nov 29, 2018

@westonruter westonruter merged commit 0e53679 into 1.0 Nov 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the update_generated_tags_file branch Nov 29, 2018

@kienstra kienstra moved this from Ready for review to Ready for Merging in v1.0 Nov 29, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

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