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

Include dynamic and additional URL formats #13

Merged
merged 2 commits into from
Nov 25, 2017
Merged

Include dynamic and additional URL formats #13

merged 2 commits into from
Nov 25, 2017

Conversation

amsross
Copy link
Contributor

@amsross amsross commented Nov 23, 2017

  • Correctly display s3 url based on the formats laid out for specific regions in the documentation.
  • Add a log the subdomained version of the url

These changes were made according to the documentation on these pages:

This relates to, includes, and expands upon the changes in #6

Julian Kleinhans and others added 2 commits October 13, 2017 17:14
Also log the subdomained version of the url
@amsross amsross changed the title Patch 1 Include dynamic and additional URL formats Nov 23, 2017
@fernando-mc fernando-mc mentioned this pull request Nov 25, 2017
@fernando-mc
Copy link
Owner

@amsross Looks good. Merging. This will be in the next versioned release.

@fernando-mc fernando-mc merged commit a90e2a7 into fernando-mc:master Nov 25, 2017

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}`)
Copy link
Owner

@fernando-mc fernando-mc Nov 25, 2017

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

Copy link
Contributor Author

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}`)

Copy link
Owner

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LGTM!

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.

2 participants