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

🐛 Set latestVersion for amp-story-audio-sticker #39913

Merged
merged 6 commits into from
May 2, 2024
Merged

🐛 Set latestVersion for amp-story-audio-sticker #39913

merged 6 commits into from
May 2, 2024

Conversation

swissspidy
Copy link
Contributor

Tools like the AMP Optimizer require latestVersion to correctly import extensions with the correct version.

I thought ampproject/amp-toolbox#1357 was enough to support amp-story-audio-sticker there, but turns out it wasn't.

All other extensions already have latestVersion set, so it makes sense for amp-story-audio-sticker to set it too.

Once this is merged, I'll submit a follow-up to ampproject/amp-toolbox#1357 on the toolbox repo.

Tools like the AMP Optimizer require `latestVersion` to correctly import extensions with the correct version.

I thought ampproject/amp-toolbox#1357 was enough to support `amp-story-audio-sticker` there, but turns out it wasn't.

All other extensions already have `latestVersion` set, so it makes sense for `amp-story-audio-sticker` to set it too.
@swissspidy
Copy link
Contributor Author

/cc @erwinmombay

@erwinmombay
Copy link
Member

sorry missed this. approved

@swissspidy
Copy link
Contributor Author

Awesome, thanks. I just updated the branch so this should be ready for merge now.

Note that the failing Circle CI tests seem unrelated as they happen on main as well.

@swissspidy
Copy link
Contributor Author

@erwinmombay @ychsieh Mind merging this one? :-)

@ychsieh
Copy link
Contributor

ychsieh commented Apr 15, 2024

Seems like Percy visual test is failing. Please re-run.

@swissspidy
Copy link
Contributor Author

Seems like Percy visual test is failing. Please re-run.

I just did, but to no avail. There's a snapshot that's missing on main but is not missing here 🤷

Screenshot 2024-04-15 at 16 03 29

Maybe just approve the build on Percy?

@swissspidy
Copy link
Contributor Author

@ychsieh OK, Percy is happy now 🤷 THis should be ready.

@ychsieh ychsieh removed the request for review from processprocess May 2, 2024 16:35
@ychsieh
Copy link
Contributor

ychsieh commented May 2, 2024

Could you sync and push to rerun the owners check(which also rerun everything tho)?

@swissspidy
Copy link
Contributor Author

That doesn't help with the owners check it seems

@danielrozenberg
Copy link
Member

Looks like someone with OWNERS approval had to re-approve this, which I just did. Go ahead team and submit this when ready!

@ychsieh ychsieh merged commit cf284c8 into ampproject:main May 2, 2024
52 checks passed
@swissspidy swissspidy deleted the patch-1 branch May 2, 2024 22:31
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.

5 participants