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 amp-story-subscriptions experiment and turn it to 100% #38284

Merged
merged 3 commits into from
Jul 18, 2022

Conversation

ychsieh
Copy link
Contributor

@ychsieh ychsieh commented Jun 9, 2022

This will be pushed only when all the final approvals from all stakeholders are granted.

@amp-owners-bot amp-owners-bot bot requested a review from newmuis June 9, 2022 18:32
@amp-owners-bot
Copy link

amp-owners-bot bot commented Jun 9, 2022

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story.js

@ychsieh ychsieh requested review from gmajoulet and removed request for newmuis June 9, 2022 18:33
@gmajoulet
Copy link
Contributor

cc @mszylkowski

Should we turn on the experiment instead?

@mszylkowski
Copy link
Contributor

mszylkowski commented Jun 10, 2022

I'd keep the experiment if we at some point might need to turn it off (eg: adding the component breaks all stories with the component and we have to disable the feature through client-side experiments). So for safety, sounds good to have the experiment.

For reference the experiment can be turned on with canary-config.json and prod-config.json (you can reference #38285, 0 means it's disabled and 1 means it's enabled). The client-side experiments are overriden in another repo but that's not necessary now.

@ychsieh
Copy link
Contributor Author

ychsieh commented Jun 10, 2022

Cool thanks for the suggestion! Done.

@newmuis
Copy link
Contributor

newmuis commented Jun 11, 2022

Let's be sure to coordinate with @raovs before launching this experiment

@ychsieh
Copy link
Contributor Author

ychsieh commented Jun 11, 2022

Definitely, this and other launch PRs #38179 and #38194 would only be merged once we got all the internal approvals from all the stakeholders.

Copy link
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

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

Approving the changes, feel free to merge when the other conditions are met.

@mszylkowski
Copy link
Contributor

@gmajoulet has OWNERS over the config files so you'll need his approval as well

@ychsieh ychsieh changed the title ✨ Remove the flag to launch amp-story-subscriptions fully ✨ Add amp-story-subscriptions experiment and turn it to 100% Jun 13, 2022
Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

Shouldn't we launch the experiment from the CDN configuration for instant propagation, vs 10+ days with these config changes?

@ychsieh
Copy link
Contributor Author

ychsieh commented Jun 16, 2022

Good question. I guess the other two PRs would be merged the same time as this one after all approvals are granted, so even though we can make this config in almost instantly, the developers would still need to wait for the other two PRs to be able to play with this new feature?

@gmajoulet
Copy link
Contributor

Sounds good! Documentation and validation rollout to production faster. We'll be able to enable the experiment through the CDN configuration a few days after merging validation/doc if we want to save a few days : ))

@ychsieh ychsieh merged commit 78802d8 into ampproject:main Jul 18, 2022
@ampprojectbot
Copy link
Member

Warning: disparity between this PR Percy build and its main build

The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the main branch that this PR was merged into, and there appears to be a mismatch between the two.

This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build / main commit's Percy build > and determine further action:

  • If the disparity appears to be caused by this PR, please create an bug report or send out a new PR to fix
  • If the disparity appears to be a flake, please @-mention ampproject/wg-approvers in a comment
  • If there is no disparity and this comment was created by mistake, please @-mention ampproject/wg-infra
  • If unsure, @-mention ampproject/wg-approvers

jpettitt pushed a commit to jpettitt/amphtml that referenced this pull request Jul 20, 2022
…ect#38284)

* Remove the flag to launch amp-story-subscriptions fully.

* Make amp-story-subscriptions a controllable experiment.
jpettitt pushed a commit to jpettitt/amphtml that referenced this pull request Jul 21, 2022
…ect#38284)

* Remove the flag to launch amp-story-subscriptions fully.

* Make amp-story-subscriptions a controllable experiment.
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.

None yet

5 participants