Skip to content

add a way to override hostname#6972

Merged
erwinmombay merged 3 commits intoampproject:masterfrom
erwinmombay:allow-hostname-override
Jan 18, 2017
Merged

add a way to override hostname#6972
erwinmombay merged 3 commits intoampproject:masterfrom
erwinmombay:allow-hostname-override

Conversation

@erwinmombay
Copy link
Copy Markdown
Member

ideally we would actually read this from a providers "config" and generate that config with the binary but theres some refactoring needed to do that. providing these flags for now to unblock #4293

Fixes #4293

/to @dknecht @jrf0110 for additional reviewers

@erwinmombay erwinmombay requested a review from cramforce January 10, 2017 22:45
gulpfile.js Outdated
var integrationJs = argv.fortesting
? './f.js'
: `https://3p.ampproject.net/${internalRuntimeVersion}/f.js`;
: `https://${hostname3p}/${internalRuntimeVersion}/f.js`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't f.js (3p/integration) already use urls defined config.js?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i think this is actually the path thats embedded to remote.html, we'll need that updated too right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remote.html goes to the publisher. So for now we are just broken in those cases.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok i'll revert this line for now

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

rolled back

@jrf0110
Copy link
Copy Markdown
Contributor

jrf0110 commented Jan 10, 2017

I think that takes care of just about everything except for remote.html. Any publisher who copies that file will likely copy it straight from the ampproject/amphtml repo. Actually, it doesn't matter where it was copied from. Even though f.js would be configurable through AMP_CONFIG, the file itself will always be loaded from a non-configurable 3p domain.

@erwinmombay erwinmombay force-pushed the allow-hostname-override branch from 9e8cb3f to 6a9acf2 Compare January 17, 2017 19:47
@erwinmombay
Copy link
Copy Markdown
Member Author

@cramforce PTAL

gulpfile.js Outdated
var integrationJs = argv.fortesting
? './f.js'
: `https://3p.ampproject.net/${internalRuntimeVersion}/f.js`;
: `https://3p.ampproject.org/${internalRuntimeVersion}/f.js`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

huh?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ick, bad revert

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

@erwinmombay erwinmombay merged commit 2a7cfaa into ampproject:master Jan 18, 2017
jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017
* add a way to override hostname

* rollback 3p url change

* org -> net
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* add a way to override hostname

* rollback 3p url change

* org -> net
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants