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 parameters getter on GuStack #1313

Merged
merged 1 commit into from
Jun 1, 2022
Merged

feat: Add parameters getter on GuStack #1313

merged 1 commit into from
Jun 1, 2022

Conversation

akash1810
Copy link
Member

What does this change?

This is more correct than getParam as it return ALL parameters, not just those of type GuParameter. As a result the getParam and setParam functions on GuStack are also removed. (There's an argument that this getter could be internal/private to the library - see below.)

This change also somewhat reduces the utility of the GuParameter construct. We might be able to simplify (or even remove!) it.

How to test

CI should continue to pass.

How can we measure success?

Less code, and a more complete API.

Have we considered potential risks?

Note
I propose we ship this as a minor update. The risk is this isn't inline with semver.

Although this change removes the getParam and setParam functions on GuStack:

Given the above, there might be an argument to make the new parameters getter internal to the repository.

Checklist

  • I have listed any breaking changes, along with a migration path 1
  • I have updated the documentation as required for the described changes 2

Footnotes

  1. Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs.

  2. If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid?

This is more correct than `getParam` as it return ALL parameters,
not just those of type `GuParameter`.

As a result the `getParam` and `setParam` functions on `GuStack` are removed.

This change also somewhat reduces the utility of `GuParameter`.
We might be able to simplify (or even remove!) it.
@akash1810 akash1810 requested a review from a team as a code owner May 27, 2022 07:12
Comment on lines +145 to 150
const parameterKeys = Object.keys(props.stackSetInstance.parameters);

const undefinedStackSetParams = parameterKeys.filter((_) => !params.includes(_));

if (undefinedStackSetParams.length !== 0) {
throw new Error(`There are undefined stack set parameters: ${undefinedStackSetParams.join(", ")}`);
Copy link
Member Author

Choose a reason for hiding this comment

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

Our current model is to feed stack set instance's parameters from the parent stack, rather than have the stack set instance read from parameter store, as the orchestration of this is tricky.

Here we check that all parameters on the stack set instance also exist on the parent stack, throwing if not.

*
* @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/parameters-section-structure.html
*/
get parameters(): Record<string, CfnParameter> {
Copy link
Member Author

@akash1810 akash1810 May 27, 2022

Choose a reason for hiding this comment

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

Today, this is only used in the GuStackSet construct below.

@jacobwinch
Copy link
Contributor

jacobwinch commented May 30, 2022

there might be an argument to make the new parameters getter internal to the repository

I would support this as:

  • We are at ~19% adoption and nobody is using the old getters/setters; this suggests that we don't really need them to be part of the public API.
  • We want to discourage usage of CFN Parameters where possible (they complicate the deployment story and are often used to obscure values that could easily be version controlled under cdk e.g. instance size). Including a public getter on GuStack could be seen to contradict this recommendation.

@akash1810
Copy link
Member Author

there might be an argument to make the new parameters getter internal to the repository

Attempts at this have failed! Initially I thought protected would be enough, but it's still available to any subclass of GuStack. The best I can think of is to make it it's own function on an obscure import path, but that'll still be available, albeit annoying.

Any objections to leaving as is?

@jacobwinch
Copy link
Contributor

jacobwinch commented Jun 1, 2022

The best I can think of is to make it it's own function on an obscure import path, but that'll still be available, albeit annoying.

Sorry if I'm missing something obvious, but could we just move the code into stack-set.ts, since that's the only place that it's used at the moment anyway?

Any objections to leaving as is?

If that's not possible for some reason (or if you don't like the idea!) feel free to leave it as is 👍

@akash1810
Copy link
Member Author

Sorry if I'm missing something obvious, but could we just move the code into stack-set.ts, since that's the only place that it's used at the moment anyway?

I'm sure we can do this, I'm just getting a bit confused about how we've named things, which suggests our API for creating stack sets isn't that clear 😬 .

I'll merge as is and look to refactor afterwards to simplify.

@akash1810 akash1810 merged commit 6442f71 into main Jun 1, 2022
@akash1810 akash1810 deleted the aa-parameters branch June 1, 2022 07:34
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.

2 participants