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: add a subset of properties for stackset stack manipulation #120

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

arrjay
Copy link

@arrjay arrjay commented May 19, 2023

Addresses #116

the current implementation of this provides some known properties that are ok to pass along, rather than everything except the synthesizer property.

@arrjay arrjay changed the title add a subset of properties for stackset stack manipulation feat: add a subset of properties for stackset stack manipulation May 19, 2023
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

@arrjay thanks for this! Just 1 minor comment and then we are good to go!

src/stackset-stack.ts Show resolved Hide resolved
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

I also forgot to mention tests. Can you add a test to assert that these settings take effect?

@@ -49,9 +75,10 @@ export class StackSetStack extends Stack {
public readonly templateFile: string;
private _templateUrl?: string;
private _parentStack: Stack;
constructor(scope: Construct, id: string) {
constructor(scope: Construct, id: string, props: StackSetStackProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constructor(scope: Construct, id: string, props: StackSetStackProps) {
constructor(scope: Construct, id: string, props: StackSetStackProps = {}) {

Sorry 1 more. Since all the props are optional this should be optional as well.

@dannysteenman
Copy link

+1 for this feature.

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.

3 participants