-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
s3: bucketUrl and urlForObject(key) #370
Conversation
The `bucketUrl` returns the URL of the bucket and `urlForObject(key)` returns the URL of an object within the bucket. Furthermore: `iam.IIdentityResource` was soft-renamed to `iam.IPrincipal` (IIdentityResource is still supported).
@@ -18,6 +18,12 @@ export class AwsDomainSuffix extends PseudoParameter { | |||
} | |||
} | |||
|
|||
export class AwsURLSuffix extends PseudoParameter { | |||
constructor() { | |||
super('AWS::URLSuffix'); |
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.
Nice!
packages/@aws-cdk/s3/lib/bucket.ts
Outdated
@@ -1,5 +1,5 @@ | |||
import { applyRemovalPolicy, Arn, Construct, FnConcat, Output, PolicyStatement, RemovalPolicy, Token } from '@aws-cdk/core'; | |||
import { IIdentityResource } from '@aws-cdk/iam'; | |||
import { applyRemovalPolicy, Arn, AwsRegion, AwsURLSuffix, Construct, FnConcat, Output, PolicyStatement, RemovalPolicy, Token } from '@aws-cdk/core'; |
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.
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.
👍
* Similar to calling `urlForObject` with no object key. | ||
*/ | ||
public get bucketUrl() { | ||
return this.urlForObject(); |
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 would have expected the dependency to be in the reverse order of this...
Also, any reason to not use the DomainName
attribute of AWS::S3::Bucket
? Do we actually need to have a top-level S3 namespace rooted URL in some cases?
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.
Yes, in order for this to be available for BucketRef
s, which don't have the DomainName information on them.
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.
The BucketRef
could return the Fn::Concat
stuff, and the Bucket
could use the attribute directly. Isn't it?
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 would have expected the dependency to be in the reverse order of this...
This will result in FnConcat(FnConcat(...), key))
which is kind of ugly
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.
Only when using a BucketRef
. But whatever, that is not crazy important.
The
bucketUrl
returns the URL of the bucketand
urlForObject(key)
returns the URL of anobject within the bucket.
Furthermore:
iam.IIdentityResource
was soft-renamedto
iam.IPrincipal
(IIdentityResource isstill supported).
By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.