fix(firehose): remove unused role during DeliveryStream creation#26930
fix(firehose): remove unused role during DeliveryStream creation#26930mergify[bot] merged 10 commits intoaws:mainfrom
Conversation
mrgrain
left a comment
There was a problem hiding this comment.
Hmm, this is kind of changing the contract for DeliveryStream.grantPrincipal. Yes technically it is still a principle, but now it might be unusable. (Although, grantPrincipal 🤨 why isn't this just called role - but that's a different story).
Can you change this.grantPrincipal to use a getter that will return the role if we already have one and only create the unused role when the property is accessed? That should give us the best of both worlds. What do you think?
@mrgrain Thanks for the review! I'm totally up for making that change but wanted to point out one line of code: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-autoscaling/lib/auto-scaling-group.ts#L1274. There seems to be some precedent for this? I'm good either way, let me know what you'd prefer. |
Yes there's precedent and if it were a new construct I'd probably lean that way. But because it is a change, I'd like us to comply with the previously established contract. |
18bdbf8 to
f6ac3fc
Compare
|
@mrgrain Updated per your feedback—thanks! |
|
This is expected as I'm removing the unused role: Stack: aws-cdk-firehose-delivery-stream-s3-all-properties - Resource: DeliveryStreamServiceRole964EEBCC - Impact: WILL_DESTROY |
|
@mrgrain Good to |
? Test failing because it's deleting a resource..|
Good to ship, but we'll need the build passing. Can you run that integration test and update the snapshot? |
8af926a to
ee8cbb2
Compare
Pull request has been modified.
|
@mrgrain I missed those other ones—apologies! |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main 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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
) When a DeliveryStream is created without `sourceStream` or `encryptionKey`, an extra role is being created that is unused. This PR removes creation of that role. I also learned that the role created for `encryptionKey` is used "indirectly" for a grant put on the KMS key...interesting. Closes #26927. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
When a DeliveryStream is created without
sourceStreamorencryptionKey,an extra role is being created that is unused. This PR removes creation of that role.
I also learned that the role created for
encryptionKeyis used "indirectly" for a grantput on the KMS key...interesting.
Closes #26927.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license