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.

Sign up for GitHub

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

Jump to bottom

feat(s3): allow consumers to specify bucket website URL format #1550

Merged
merged 3 commits into from
Jan 15, 2019
Merged

feat(s3): allow consumers to specify bucket website URL format #1550

merged 3 commits into from
Jan 15, 2019

Conversation

AlexChesters
Copy link
Contributor

Opened after a discussion with @rix0rrr after #1544 was merged.

Pull Request Checklist

  • [ not needed ] Testing
    • Unit test added
    • CLI change?: manually run integration tests and paste output as a PR comment
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • [ not needed ] Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@AlexChesters AlexChesters requested a review from a team as a code owner January 15, 2019 13:40
@AlexChesters
Copy link
Contributor Author

@rix0rrr if I understand what I've done correctly, I think this PR may only change ImportedBucket rather than the "normal" Bucket construct. Am I correct? If so, I couldn't quite figure out how to implement this for the "normal" Bucket so hopefully you or someone else can chime in with some advice 😄

@AlexChesters AlexChesters mentioned this pull request Jan 15, 2019
Merged
3 tasks
@AlexChesters
Copy link
Contributor Author

I've marked the Testing section on the PR template as "not needed" as this change is already covered by existing test cases. Happy to be convinced otherwise though 😄

Indeed depending on any replies to my comment above there may be some additional changes to this PR that do require new tests added.

rix0rrr
rix0rrr reviewed Jan 15, 2019
View reviewed changes
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I would like to see a test on the new format as well, just to make sure.

packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated
@@ -1000,6 +1012,8 @@ class ImportedBucket extends BucketBase {
}

private generateBucketWebsiteUrl() {
return `${this.bucketName}.s3-website-${new cdk.Aws().region}.amazonaws.com`;
return this.bucketWebsiteNewUrlFormat
? `${this.bucketName}.s3-website-${cdk.Stack.find(this).region}.${cdk.Stack.find(this).urlSuffix}`
Copy link
Contributor

@rix0rrr rix0rrr Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It is just the other way around; new format is with ., old format is with -.

I support the default being false, so that the behavior is the same.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 15, 2019

And thank you for submitting this PR!

@AlexChesters
Copy link
Contributor Author

@rix0rrr not a problem 😄

I've pushed two changes, one to actually make sure the new format is correct (i.e. with a ., not a -) and one to default the prop to false as per your suggestion - how do they look?

I agree with you that it feels like there should be a test here, have you got any ideas? Currently it's a private function so I'm not sure how this test would look. I don't see how it would be possible by asserting against the generated stack as it just eventually outputs a Fn::GetAtt snippet rather than the actual value.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 15, 2019

You're right. It's only for imported buckets, so is not bound to come up often. Thanks!

@rix0rrr rix0rrr merged commit 28a423d into aws:master Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rix0rrr rix0rrr rix0rrr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AlexChesters @rix0rrr