-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(s3): export bucket websiteURL (#1521) #1522
feat(s3): export bucket websiteURL (#1521) #1522
Conversation
@@ -199,6 +199,13 @@ export interface BucketImportProps { | |||
* @default Inferred from bucket name | |||
*/ | |||
bucketDomainName?: string; | |||
|
|||
/** | |||
* The website URL of the bucket (if static web hosting is enabled). |
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.
I'm not sure how useful this description is, I tried a couple of different variations but couldn't come up with anything better than this.
Suggestions welcome!
@@ -465,6 +467,17 @@ export = { | |||
"Export": { | |||
"Name": "S1:MyBucketDomainNameF76B9A7A" | |||
} | |||
}, | |||
"MyBucketWebsiteURL9C222788": { |
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.
I found the logical ID here (i.e. MyBucketWebsiteURL9C222788
) from running the tests, watching them fail and observing the output.
I'm not sure if I was meant to have determined the output from somewhere else?
* | ||
* @default Inferred from bucket name | ||
*/ | ||
bucketWebsiteURL?: string; |
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.
Please rename to bucketWebsiteUrl
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.
What's the difference between this and domainName? I also can't seem to find the actual implementation in Bucket
.
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.
Thanks for the review @eladb 😄
I've pushed the change to rename to bucketWebsiteUrl
now.
Ah, I've just spotted that this actually will be identical to domainName
. Good spot, I'll push a fix for that.
based on PR feedback from @eladb
rather than the DomainName value
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.
Still can't find the implementation in "normal" Bucket
(not ImportedBucket
)
@@ -620,6 +629,7 @@ export class Bucket extends BucketBase { | |||
bucketArn: new cdk.Output(this, 'BucketArn', { value: this.bucketArn }).makeImportValue().toString(), | |||
bucketName: new cdk.Output(this, 'BucketName', { value: this.bucketName }).makeImportValue().toString(), | |||
bucketDomainName: new cdk.Output(this, 'DomainName', { value: this.domainName }).makeImportValue().toString(), | |||
bucketWebsiteUrl: new cdk.Output(this, 'WebsiteURL', { value: this.bucketWebsiteUrl }).makeImportValue().toString() |
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.
What happens if bucketWebsiteUrl is undefined?
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.
As in from a consumer's point of view? Currently it would just result in the property being undefined
as you said.
I've had a go at creating separate test cases (for cases where a bucket does and doesn't have a website configuration) but I'm not 100% sure how to proceed. Looking at the other cases it seems the usual way of testing this stuff is to generate the CloudFormation and then assert against that. In this case, however, the value is just an Fn::GetAtt
snippet:
'exports the WebsiteURL is website configuration is enabled'(test: Test) {
const stack = new cdk.Stack();
const bucket = new s3.Bucket(stack, 'Website', {
bucketName: 'my-bucket',
websiteIndexDocument: 'index.html'
});
test.deepEqual(cdk.resolve(bucket.bucketWebsiteUrl), 'my-bucket-<REGION>.s3-website.amazonaws.com');
test.done();
}
The are two problems I've ran into when attempting to create test cases like the above:
AssertionError: { 'Fn::GetAtt': [ 'Website32962D0B', 'WebsiteURL' ] } deepEqual 'my-bucket-<REGION>.s3-website.amazonaws.com'
- How can I compare against the actual value as opposed to an
Fn::GetAtt
snippet? (I assumed callingcdk.resolve
would have solved this - How can I determine the region correctly? I can't see an example of how to do this in any of the other test cases
- How can I compare against the actual value as opposed to an
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.
Currently it would just result in the property being undefined as you said.
why? if the user sets up website hosting for the bucket, I'd expect this to return the URL, no?
How can I compare against the actual value as opposed to an Fn::GetAtt snippet? (I assumed calling cdk.resolve would have solved this
You can't. The "end result" of a CDK synth is a CloudFormation template. I'd expect the test to assert that the resulting template (or template part) is the right one (i.e. includes the GetAtt).
How can I determine the region correctly? I can't see an example of how to do this in any of the other test cases
What I usually do is print out the output of cdk.resolve
, inspect it to make sure it's what I expect, and then use it as my test expectation:
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.
why? if the user sets up website hosting for the bucket, I'd expect this to return the URL, no?
Ah, I thought you were asking what would happen if the user hadn't set up website hosting for the bucket. In the case where they have set up website hosting for the bucket, bucketWebsiteUrl
should never be undefined
. Is there something you've spotted that might result it in being undefined
?
You can't. The "end result" of a CDK synth is a CloudFormation template. I'd expect the test to assert that the resulting template (or template part) is the right one (i.e. includes the GetAtt).
What I usually do is print out the output of cdk.resolve, inspect it to make sure it's what I expect, and then use it as my test expectation:
Gotcha, I'll give this a go. Thanks!
@@ -985,4 +997,8 @@ class ImportedBucket extends BucketBase { | |||
private generateDomainName() { | |||
return `${this.bucketName}.s3.amazonaws.com`; | |||
} | |||
|
|||
private generateBucketWebsiteUrl() { | |||
return `${this.bucketName}.s3-website-${new cdk.AwsRegion()}.amazonaws.com`; |
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.
how does this look in a different partition? (i.e. china)
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.
That's a.. good question. I wasn't aware (until I found these docs) that the domain name was different in regions such as China.
Happy to take a look at making sure this works in regions such as China, would you want me to take a look at doing the same for the generateDomainName()
function above (as that appears to have the same problem), or leave that for a separate issue and PR?
@eladb I think I see what you mean, but I'm struggling to see exactly where I need to define this in normal Bucket. Are you able to point me in the general direction? Perhaps where some of the other properties (e.g. |
Sure, something around here |
Oh right. sorry, my bad. I missed this. Looks good. |
Not a problem 😄 I've added a unit test, let me know your thoughts on that? |
The automatic test failed. |
@rix0rrr are you able to shed any light on that test failure? I've ran |
I think this is due to some refactorings we've done on
Sorry for the hassle. |
Ah, of course. Don't apologise, my fault entirely! I'll merge master into my branch and resolve the issues. Apologies! |
Small change to enable tagging KMS keys.
The `cdk diff` command intentionally returns non-zero when a difference is found, similar to how the POSIX `diff` command behaves (returning 0 when no difference is found, 1 when some difference is found, and >1 when an error occurred). This behavior was however undocumented, possibly leading to user confusion. This change adds a note in `cdk --help` that mentions the exit code 1 will be used when a difference is found. Fixes #1440
Since #973, the renames of CFN properties from "name" to "xxxName" were removed. But the ELBv2 library still used `loadBalancerName`. The reason this was not discovered was because we are splatting `additionalProps` of type `any`, and this caused the type checker to stop verifying property names. Fixes #1481
Use a unified diff format to render differences in arbitrary values, making it easier to understand what is changing in possibly large JSON structures, for example. The number of context lines used when rendering the JSON differences can be customized using the `--context-lines` option of `cdk diff`, which has a default value of `3`.
Adds support for CNAME records via the `CnameRecord` construct. Misc: - `TXTRecord` was renamed to `TxtRecord`. - `hostedZoneNameServers` attribute added to IHostedZone - Made `HostedZone` a concrete (non-abstract) class so it will be compatible with how CloudFormation represents this resource, but left PublicHostedZone and PrivateHostedZone to allow a more strongly-typed experience if you like. Credits for original PR (closes #1420): @MikeBild BREAKING CHANGE: The `route53.TXTRecord` class was renamed to `route53.TxtRecord`.
Now that we can represent attributes as native strings and string lists, this covers the majority of resource attributes in CloudFormation. This change: * String attributes are represented as `string` (like before). * String list attribute are now represented as `string[]`. * Any other attribute types are represented as `cdk.Token`. Attributes that are represented as Tokens as of this change: * amazonmq has a bunch of `Integer` attributes (will be solved by #1455) * iot1click has a bunch of `Boolean` attributes * cloudformation has a JSON attribute * That's all. A few improvements to cfn2ts: * Auto-detect cfn2ts scope from package.json so it is more self-contained and doesn't rely on cdk-build-tools to run. * Added a "cfn2ts" npm script to all modules so it is now possible to regenerate all L1 via "lerna run cfn2ts". * Removed the premature optimization for avoiding code regeneration (it saved about 0.5ms). Fixes #1406 BREAKING CHANGE: any `CfnXxx` resource attributes that represented a list of strings are now typed as `string[]`s (via #1144). Attributes that represent strings, are still typed as `string` (#712) and all other attribute types are represented as `cdk.Token`.
BREAKING CHANGE: the deprecated `cloudformation.XxxResource` classes have been removed. Use the `CfnXxx` classes instead.
Change `conditionXx` methods to accept an interface `IConditionExpression` instead of concrete class and implement this interface by the `Condition` construct. This enables chaining conditions like so: const cond1, cond2, cond, cond4 = new cdk.Condition(...) Fn.conditionOr(cond1, cond2, Fn.conditionAnd(cond3, cond4)) Fixes #1457
AWS Step Functions released support for 'Parameters' option in the input definition. Closes #1480
It is now no longer necessary to use `export()` and `import()` when sharing constructs between two `Stacks` inside the same CDK app. Instead, objects defined in one stack can be used directly in another stack. The CDK will detect when an attribute (such as an ARN, ID or URL) of such an object is used in a different stack, and will automatically create the required `Export` in the producing stack and insert the corresponding `Fn::ImportValue` in the consuming stack. BREAKING CHANGE: if you are using `export()` and `import()` to share constructs between stacks, you can stop doing that, instead of `FooImportProps` accept an `IFoo` directly on the consuming stack, and use that object as usual. `ArnUtils.fromComponents()` and `ArnUtils.parse()` have been moved onto `Stack`. All CloudFormation pseudo-parameter (such as `AWS::AccountId` etc) are now also accessible via `Stack`, as `stack.accountId` etc. `resolve()` has been moved to `this.node.resolve()`. `CloudFormationJSON.stringify()` has been moved to `this.node.stringifyJson()`. `validate()` now should be `protected`. Fixes #1324.
In order to allow passing in an imported role to a Lambda/Alias, we must use `IRole` instead of `Role`.
The "toCloudFormation" method of generated CFN resources invoke "resolve" so that any lazy tokens are evaluated. This escaped the global settings set by `findTokens` which collect tokens so they can be reported as references by `StackElement.prepare`. To solve this, findTokens now accepts a function instead of an object and basically just wraps it's invocation with settings that will collect all tokens resolved within that scope. Not an ideal solution but simple enough. This was not discovered because we didn't have any tests that validated the behavior of the generated CFN resources (they are not accessible from the @aws-cdk/cdk library). We add a unit test in the IAM library to cover this use case.
Two improvements to stack dependency handling in the toolkit: * Destroy stacks in reverse order. This is necessary to properly dispose of stacks that use exports and `Fn::ImportValue`. Fixes #1508. * Automatically include stacks that have a dependency relationship with the requested stacks, unless `--exclusively` (`-e`) is passed on the command line. Fixes #1505.
add packages.txt to github pages
Jetbrains IDEs will index the files in a project to provide intelligent suggestions to users. However, this project uses lerna and has cyclical symlinks inside node_module directories. The IDE will index until it runs out of memory following these links. Given the size of this project, doing the exclusions by hand is infeasable every time one does a git clean. This script will modify the .iml file directly to add the appropriate exclusions. * The JetBrains IDEs exclusion list assumed your iml file was named aws-cdk. It now uses the root directory's name instead. * IntelliJ stores the iml file at root. The script now accounts for that difference. * For uncommon setups, the script can take an optional path arguement to opt-out of conventional file discovery, and be explicit.
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.