Skip to content
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

Merged
merged 3 commits into from
Nov 25, 2021

Conversation

arcrank
Copy link
Contributor

@arcrank arcrank commented Nov 24, 2021

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 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]>
@gitpod-io
Copy link

gitpod-io bot commented Nov 24, 2021

@github-actions github-actions bot added the @aws-cdk/aws-servicecatalog Related to AWS Service Catalog label Nov 24, 2021
public static associateTagOptions(portfolio: IPortfolio, tagOptions: TagOptions): void {
const portfolioStack = cdk.Stack.of(portfolio);

public static associateTagOptions(resource: IResource, resourceId: string, tagOptions: TagOptions): void {
Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

@skinny85 skinny85 left a 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.

packages/@aws-cdk/aws-servicecatalog/README.md Outdated Show resolved Hide resolved
const tagOptions = new servicecatalog.TagOptions({
```ts fixture=portfolio-product
const tagOptionsForPortfolio = new servicecatalog.TagOptions({
costCenter: ['A', 'B'],
Copy link
Contributor

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?

Copy link
Contributor Author

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.

packages/@aws-cdk/aws-servicecatalog/README.md Outdated Show resolved Hide resolved
public static associateTagOptions(portfolio: IPortfolio, tagOptions: TagOptions): void {
const portfolioStack = cdk.Stack.of(portfolio);

public static associateTagOptions(resource: IResource, resourceId: string, tagOptions: TagOptions): void {
Copy link
Contributor

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.

@arcrank arcrank requested a review from skinny85 November 24, 2021 20:52
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mergify
Copy link
Contributor

mergify bot commented Nov 24, 2021

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 81fbb13
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 2d19e15 into aws:master Nov 25, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 25, 2021

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).

beezly pushed a commit to beezly/aws-cdk that referenced this pull request Nov 29, 2021
…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]>
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-servicecatalog Related to AWS Service Catalog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants