-
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
feat(servicecatalog): Add TagOptions to a CloudformationProduct #17672
Conversation
Users can now associate tag options to a cloudformation product through an association call or upon instantiation. Tag options addded to a portfolio are made available for any products within it, but you can also have separate, product level tag options. We only create unique tag option constructs in the template but we can have the same tag option associated with both a portfolio and a product in that portfolio, the logic that resolves this is handled by service catalog Co-authored-by: Dillon Ponzo <[email protected]>
public static associateTagOptions(portfolio: IPortfolio, tagOptions: TagOptions): void { | ||
const portfolioStack = cdk.Stack.of(portfolio); | ||
|
||
public static associateTagOptions(resource: IResource, resourceId: string, tagOptions: TagOptions): void { |
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.
By doing it this way it minimizes necessary code changes. The crux here was that we need the Id, which is a different field in both the portfolio and product. I see two ways of doing this. one way would create two entry points for the logic here, e.g. AssociationManager
has two different methods for associating tag options, and those would then reuse the same underlying functions., or doing it this way. I chose this way because it seemed more efficient, the code changes are just from renaming, and this method is only used internally so we control the types inputted.
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 or having just a type resolution method that can take some
type productOrPortfolio = IPortfolio | IProduct;
and then resolving which one it is based on if it had the portfolioId
field or not. I think this type of approach was shunned before for a similar issue.
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.
Honestly, I don't think it matters too much 😜. The current solution is fine.
This is all module-private, we can change it any time we want.
packages/@aws-cdk/aws-servicecatalog/lib/private/association-manager.ts
Outdated
Show resolved
Hide resolved
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.
Looks great @arcrank! Just minor documentation notes.
const tagOptions = new servicecatalog.TagOptions({ | ||
```ts fixture=portfolio-product | ||
const tagOptionsForPortfolio = new servicecatalog.TagOptions({ | ||
costCenter: ['A', 'B'], |
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.
Again, can we make it a real example when would you use these? Not just some dummy values?
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.
yeah I am not fully aware of how these are usually done since mostly they are like account numbers. I referenced our own public docs and used that example.
public static associateTagOptions(portfolio: IPortfolio, tagOptions: TagOptions): void { | ||
const portfolioStack = cdk.Stack.of(portfolio); | ||
|
||
public static associateTagOptions(resource: IResource, resourceId: string, tagOptions: TagOptions): void { |
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.
Honestly, I don't think it matters too much 😜. The current solution is fine.
This is all module-private, we can change it any time we want.
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.
👍
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). |
…17672) Users can now associate TagOptions to a cloudformation product through an association call or upon instantiation. TagOptions added to a portfolio are made available for any products within it, but you can also have separate, product level tag options. We only create unique TagOption constructs in the template but we can have the same Tag Option associated with both a portfolio and a product in that portfolio, the logic that resolves this is handled by service catalog. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Dillon Ponzo <[email protected]>
…17672) Users can now associate TagOptions to a cloudformation product through an association call or upon instantiation. TagOptions added to a portfolio are made available for any products within it, but you can also have separate, product level tag options. We only create unique TagOption constructs in the template but we can have the same Tag Option associated with both a portfolio and a product in that portfolio, the logic that resolves this is handled by service catalog. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Dillon Ponzo <[email protected]>
Users can now associate TagOptions to a cloudformation product through an association call
or upon instantiation. TagOptions added to a portfolio are made available for any products within it,
but you can also have separate, product level tag options. We only create unique TagOption constructs in the template
but we can have the same Tag Option associated with both a portfolio and a product in that portfolio, the logic that
resolves this is handled by service catalog.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Co-authored-by: Dillon Ponzo [email protected]