Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(servicecatalog): Add Product Stack Asset Support #22143
feat(servicecatalog): Add Product Stack Asset Support #22143
Changes from 1 commit
86b14f6
e245852
37d12c3
0b84503
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 does
None
mean? If this is not provided are assets not supported?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.
This class is the result of binding a Artifact Template into a Product.
In the event that it is not binding to a Product Stack (URL or local asset template instead), there is no asset bucket and no asset support.
In the event that they are binding a Product Stack, if they are not using assets, then the asset bucket will not be generated and will be undefined and there will be no asset bucket.
In any other case there is an asset bucket.
I will update to
None - no assets are used in this product
as well.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 are you granting access on the other side? The IAM role that gets from these buckets needs
access to be granted access to the bucket in its princial policy.
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.
Addressed in my other comment, but there is not much more we can do. We made it as easy as possible but we don't have access to the accounts it is being shared with, it is up to the Admin to configure these spoke accounts.
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.
this message will reach the user. Do you mean to tell your user to call
bindStack()
?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.
This is the error we throw in our
synthesize
method as well. I'm not sure about this, I don't think this is an error the user can act on regardless. I don't think we can ever reach this state, so maybe this error message is just for our own sanity.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 I feel about the way we are creating the asset bucket here. What if we just created
the asset bucket by default as part of the product stack?
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.
Do you mean we create a ProductStackAssetBucket regardless of if the user is using an asset or not.
I don't that is a good idea since we would be creating resources that will not be needed (S3-deployment also deploys a lambda). Furthermore, a user could be unintentionally create a bunch of S3 Buckets (since it is one per product-stack) and fill up their S3 Bucket limit capacity.
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 think this is where we should handle the asset deployment, rather than in the bucket construct.
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 default is not none... a bucket name is generated if this property is not supplied.
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 think you can be more descriptive here since you know what the generated name will be (or atleast
the format).
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 think this should extends
s3.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.
Is
DESTROY
the correct removalPolicy? If the bucket is removed for any reason all consuming stackswill break. I think the safer default will be to
RETAIN
.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 is the reasoning behind using
cdk.Aspects
to run deploy assets? not questioning, just wondering.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.
It can be done without Aspects I believe, but Aspects fits the use case and allows us to cleanly implement our use case as well as abstract the additional code to the new
product-stack-asset-bucket
construct rather than modifying and adding additional code toproduct-stack
.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.
this is doing more than fetch right? adds an asset to
this.assets
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.
Updated to be a bit more descriptive.
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 don't think the asset management should be done in this bucket construct. These assets should be
handled as normal stack assets.