feat: parameters property in StackSet construct #447
Conversation
pcholakov
changed the title
pcholakov
changed the title
There was a problem hiding this comment.
Hi @pcholakov! Thanks for the PR and I am very sorry about the extremely delayed review. If you're still interested in contributing this, please take a look at my comments. Nothing seems too controversial.
There was a problem hiding this comment.
i'd prefer you create a new test rather than modify an existing one. parameterOverrides is still a valid thing to do so we still need to test for it.
| parameters: { | ||
| Param1: 'Value1', | ||
| }, |
There was a problem hiding this comment.
in your new test, can you add multiple parameters please
| parameters: props.parameters ? Object.entries(props.parameters).map((entry) => { | ||
| return { | ||
| parameterKey: entry[0], | ||
| parameterValue: entry[1], | ||
| }; | ||
| }) : undefined, |
There was a problem hiding this comment.
this will add parameters: undefined to everyone's stackset, which isn't the end of the world, but people don't like snapshot diffs. instead we can do the following pattern:
...(props.parameters ? { parameters: {} } : {})this way no parameters means no parameter property at all and no one's diff gets needlessly updated.
| @@ -346,7 +347,8 @@ interface DeploymentTypeConfig { | |||
| readonly callAs?: CallAs; | |||
| } | |||
|
|
|||
| interface DeploymentTypeOptions {} | |||
| interface DeploymentTypeOptions { | |||
| } | |||
There was a problem hiding this comment.
please change back these stylistic choices.
kaizencc
changed the title
|
Hey @kaizencc, absolutely no worries - still interested in contributing this and will update with your comments addressed :-) |
Before this change, the StackSet construct supported only per-target parameter overrides. This is not sufficient to pass parameters to a stack template; parameters have to be set before they can be overridden per target -- see #115.