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

new withEnv function #779

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Conversation

tsahiduek
Copy link
Contributor

Fix #778.
Add cloneEnv method.
See description of 778 -->
AWS CDK has a basic type of cdk.Environment, which is an object holds the account and the region for a target environemtn for deployment.
Having a new clone function that accept a cdk.Evironment type can simplify code using the clone method

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

@tsahiduek I like your thinking and making code level improvements.
However, cloneEnv is not very idiomatic in typescript (clone is clone). The approach that I planned for such case is with, for example builder.withBlueprintProps that we have.

I propose a change to the method and renaming to withEnv similar to withBlueprintProps. Then you can achieve your scenario like:

blueprint.clone().withEnv(env);

Also please make env mandatory in the withEnv method. Implementation will change to just set this.env to the pass param.

@tsahiduek tsahiduek changed the title new cloneEnv function new withEnv function Jul 19, 2023
@tsahiduek
Copy link
Contributor Author

@tsahiduek I like your thinking and making code level improvements. However, cloneEnv is not very idiomatic in typescript (clone is clone). The approach that I planned for such case is with, for example builder.withBlueprintProps that we have.

I propose a change to the method and renaming to withEnv similar to withBlueprintProps. Then you can achieve your scenario like:

blueprint.clone().withEnv(env);

Also please make env mandatory in the withEnv method. Implementation will change to just set this.env to the pass param.

Thank you @shapirov103 . Just pushed an updated commit as requested.

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

LGTM

@shapirov103 shapirov103 merged commit d878d2b into aws-quickstart:main Jul 19, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack builder - add support to clone using cdk.Environment rather than strings of account and region
2 participants