-
Notifications
You must be signed in to change notification settings - Fork 71
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
Include dynamic and additional URL formats #13
Conversation
Also log the subdomained version of the url
@amsross Looks good. Merging. This will be in the next versioned release. |
|
||
this.serverless.cli.log(`Uploading file ${fileKey} to bucket ${_this.bucketName}...`); | ||
this.serverless.cli.log(`If successful this should be deployed at: https://s3.amazonaws.com/${_this.bucketName}/${fileKey}`) | ||
this.serverless.cli.log('If successful this should be deployed at:') | ||
this.serverless.cli.log(`https://${urlRoot}/${_this.bucketName}/${fileKey}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This url isn't correct. I'll remove it when I push this version.
Compare:
http://serverless-finch-test.s3-website-us-east-1.amazonaws.com/ (what is output by line 274)
vs. what is output by this line:
https://s3-website-us-east-1.amazonaws.com/serverless-finch-test/index.html
The second link fails when deploying in us-east-1
A URL that does actually work (but isn't the official website endpoint url) is
https://s3.amazonaws.com/serverless-finch-test/index.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it makes sense to add back your original url format while you're at it.
this.serverless.cli.log(`https://s3.amazonaws.com/${_this.bucketName}/${fileKey}`)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amsross I could but that's also incorrect when you publish to another region. I prefer line 274. We'll stick with the correct format not the hacky "but it works" one I had before XD
Here's an example of where the line you just posted in that comment would fail. Let's say I deploy a static site to S3 in us-west-2. The working URL (and correct as far as AWS goes) is:
https://s3-us-west-2.amazonaws.com/serverless-finch-test-nonuseast1test/index.html
If I use the https://s3.amazonaws.com/
root all the time then the URL would be
https://s3.amazonaws.com/serverless-finch-test-nonuseast1test/index.html
Spoiler - this fails.
So TLDR there is a region issue in the previous version that was using s3.amazonaws.com
The newest version will fix it by changing to the correct output you put together on line 274. But I will trim out line 273 because it isn't correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
These changes were made according to the documentation on these pages:
This relates to, includes, and expands upon the changes in #6