feat(s3-deployment): add deployedBucket attribute for sequencing#15384
feat(s3-deployment): add deployedBucket attribute for sequencing#15384mergify[bot] merged 14 commits intomasterfrom
deployedBucket attribute for sequencing#15384Conversation
This allows referencing the bucket in a way that ensures the deployment has happened before dependent resources are created. TODO: - Tests - Docstrings - README
|
|
|
@rix0rrr would we consider this a breaking change? bc the asset id's change as a result of this! So some people who run tests might have their systems break. |
No. Snapshot testing w/ asset hashes is something people shouldn't be doing. |
|
I can't approve this since I opened the PR. So we need @skinny85 in the game 🙂 |
skinny85
left a comment
There was a problem hiding this comment.
LGTM. Two minor comments.
| DistributionId: props.distribution?.distributionId, | ||
| DistributionPaths: props.distributionPaths, | ||
| // Passing through the ARN sequences dependencees on the deployment | ||
| PassThroughBucketArn: props.destinationBucket.bucketArn, |
There was a problem hiding this comment.
Can't we just use the existing DestinationBucketName to do this sequencing? Won't it have the same effect, without changing the customer's template?
There was a problem hiding this comment.
I'm not sure I follow. If it doesn't change the template, it won't enforce sequencing, right?
Right now, the dependencies look like this, which lead to a race condition:
┌────────────────────┐
{ Ref: Bucket } │ │
┌────────────│ Deployment │
│ │ │
┌────────────┐ │ └────────────────────┘
│ │ │
│ Bucket │◀───────────┤
│ │ │
└────────────┘ │ ┌────────────────────┐
│ │ │
└────────────│ BucketConsumer │
{ Ref: Bucket } │ │
└────────────────────┘
We want them to look like this:
{ GetAtt: [..., 'BucketArn'] }
┌────────────┐ ┌────────────────────┐ ┌────────────────────┐
│ │ { Ref: Bucket } │ │ │ │
│ Bucket │◀────────────────│ Deployment │◀───────────────│ BucketConsumer │
│ │ │ │ │ │
└────────────┘ └────────────────────┘ └────────────────────┘
There was a problem hiding this comment.
I meant template changes related to the new PassThroughBucketArn property that you added, regardless of whether your customers use the new deployedBucket property or not.
If you used the existing DestinationBucketName property for this purpose, I believe existing customers would not have their template affected.
There was a problem hiding this comment.
Oh I see. Is that good enough though? What if the bucket is in another account?
There was a problem hiding this comment.
If it's in another account, sequencing within the template with Fn::GetAtt is irrelevant anyway 🙂.
There was a problem hiding this comment.
That is true, using the bucket still shouldn't fail.
| }, | ||
| }); | ||
|
|
||
| this.deployedBucket = s3.Bucket.fromBucketArn(this, 'DestinationBucket', Token.asString(deployment.getAtt('DestinationBucketArn'))); |
There was a problem hiding this comment.
Pretty sure we're missing a unit test for actually using this property in another construct.
924c117 to
ebfd5f2
Compare
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…ws#15384) This allows referencing the bucket in a way that ensures the deployment has happened before dependent resources are created. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This allows referencing the bucket in a way that ensures the
deployment has happened before dependent resources are created.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license