feat(s3): export bucket websiteURL (#1521) #1544
Conversation
|
This was originally implemented in #1522, however I made the diff a bit of a mess when trying to pull into some changes from master, so I've opened this PR to make it easier to review. It should have all the requested changes from #1522 implemented. cc: @eladb, @rix0rrr - apologies for messing you guys around! |
| @@ -986,4 +998,8 @@ class ImportedBucket extends BucketBase { | |||
| private generateDomainName() { | |||
| return `${this.bucketName}.s3.amazonaws.com`; | |||
| } | |||
|
|
|||
| private generateBucketWebsiteUrl() { | |||
| return `${this.bucketName}.s3-website-${new cdk.Aws().region}.amazonaws.com`; | |||
There was a problem hiding this comment.
The region should be ${cdk.Stack.find(this).region}
And I think we need ${cdk.Stack.find(this).urlSuffix} instead of amazonaws.com.
Just found out is no generalized form of this. It's either:
bucket-name.s3-website-region.amazonaws.com
OR
bucket-name.s3-website.region.amazonaws.com
depending on the region :/ Grrr
There was a problem hiding this comment.
Region change seems like a simple change 😄
The URL suffix seems less so. Do you know which one it is depending on which region? It's a bit ugly but we could have something like:
const suffix = cdk.Stack.find(this).urlSuffix
const region = cdk.Stack.find(this).region
const regionToSuffix = {
'eu-west-1': `s3-website-eu-west-1.${suffix}`,
'us-west-1': `s3-website.us-west-1.${suffix}`
}
return `${this.bucketName}.${regionToSuffix[region]}`;There was a problem hiding this comment.
In fact, this problem is intractable for you right now (since we may be deploying without the region being available at all), so don't worry about it. This ties into #1282, we need some kind of region information to solve this.
Leave it as is and ignore it, I was just making a mental note of this issue.
There was a problem hiding this comment.
Could you add a switch on the Props to pick between the two formats?
Looking at the list: https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_website_region_endpoints I'm tempted to call it something like bucketWebsiteNewUrlFormat?: boolean, and in the documentation mention it should be true for regions launched since 2014.
| @@ -972,6 +983,7 @@ class ImportedBucket extends BucketBase { | |||
| this.bucketArn = parseBucketArn(this, props); | |||
| this.bucketName = bucketName; | |||
| this.domainName = props.bucketDomainName || this.generateDomainName(); | |||
| this.bucketWebsiteUrl = props.bucketWebsiteUrl || this.generateBucketWebsiteUrl(); | |||
There was a problem hiding this comment.
Do we ALWAYS want to do this? Is there a case in which you wouldn't want the bucketWebsiteUrl set?
There was a problem hiding this comment.
I think we do. I was basing this off the current CloudFormation functionality; it will always allow you to output (and therefore read) the website URL even if web hosting is not configured.
|
Oh oops. I had some comments on this that I forgot to publish. |
|
This PR is subtly wrong now. |
|
Hi @rix0rrr, Thanks for those comments, they look sensible 😄 I notice this PR has been merged, would you like me to open a new PR with those comments applied? |
|
Oh if you wouldn't mind, that would be fantastic! Thanks! |
Fixes #1521.
This exposes the WebsiteURL property (documented in the relevant CloudFormation template).
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.