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

Force upgrade sticky ad to 1.0 #9042

Merged
merged 4 commits into from
May 4, 2017
Merged

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Apr 28, 2017

For #6169
cc @jasti
I will remove the actually file once the upgrade is stable in prod.

@dvoytenko
Copy link
Contributor

Why are we updating only tests? How would the actual upgrade happen? Are you saying that you are deploying 1.0 to 0.1 instead?

@zhouyx
Copy link
Contributor Author

zhouyx commented Apr 28, 2017

Yes. Use the file amp-sticky-ad-1.0.js for sticky-ad 0.1 in build stage.

@dvoytenko
Copy link
Contributor

LGTM, but please wait for @erwinmombay

@zhouyx
Copy link
Contributor Author

zhouyx commented May 1, 2017

ping @erwinmombay

gulpfile.js Outdated
@@ -378,6 +378,10 @@ function buildExtension(name, version, hasCss, options, opt_extraGlobs) {
options = options || {};
options.extraGlobs = opt_extraGlobs;
var path = 'extensions/' + name + '/' + version;
if (name == 'amp-sticky-ad' && version == '0.1') {
// sticky-ad upgrade, 0.1 is deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

please add explanation why we do this

Copy link
Member

@erwinmombay erwinmombay left a comment

Choose a reason for hiding this comment

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

one request to add explanation

gulpfile.js Outdated
if (name == 'amp-sticky-ad' && version == '0.1') {
// Special case for sticky-ad upgrade.
// To deprecate 0.1, replace the build path so that amp-sticky-ad-0.1.js
// is built from extensions/amp-sticky-ad/1.0/amp-sticky-ad.js
Copy link
Member

Choose a reason for hiding this comment

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

can you explain further "why" we want this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how do you like the explanation now : )

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.

4 participants