feat(ssm): reference existing SSM list parameters #21880

corymhall · 2022-09-01T17:25:39Z

This PR fixes some issues with how SSM parameter types are implemented.
Currently this module models a single type of parameter (ParameterType
enum) and that type is used to represent both CloudFormation SSM
Parameter types
For example,

new cdk.CfnParameter(this, 'Param', {
  type: 'AWS::SSM::Parameter::Value<String>', // type
});

and the AWS::SSM::Parameter.type
For example,

new ssm.CfnParameter(this, 'Param', {
  type: 'String',
});

This overloading caused the issue in the referenced issue as well as
making it more confusing for the user. For example, You can specify a type when
creating a StringParameter, but you shouldn't need to since the only
valid values are String | StringList and these are modeled as two
separate classes StringParameter & StringListParameter.

To address this, the PR introduces a new enum ParameterValueType to
model the CloudFormation SSM Parameter Types. This enum is only used in
the valueForXXX and fromXXX methods since those return a CFN
parameter.

Deprecated ssm.StringParameter.valueForTypedStringParameter since it
uses the old overloaded ParameterType.
- Introduce a new ssm.StringParameter.valueForTypedStringParameterV2
  that uses the new ParameterValueType enum
Add ssm.StringListParameter.valueForTypedListParameter
Add ssm.StringListParameter.fromListParameterAttributes
Deprecated StringParameterProps.type since the value should only be
String.

fix #12477, #14364

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

This PR fixes some issues with how SSM parameter types are implemented. Currently this module models a single type of parameter (`ParameterType` enum) and that type is used to represent _both_ [CloudFormation SSM Parameter types](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/parameters-section-structure.html#aws-ssm-parameter-types) For example, ```ts new cdk.CfnParameter(this, 'Param', { type: 'AWS::SSM::Parameter::Value<String>', // type }); ``` _and_ the [AWS::SSM::Parameter.type](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ssm-parameter.html#cfn-ssm-parameter-type) For example, ```ts new ssm.CfnParameter(this, 'Param', { type: 'String', }); ``` This overloading caused the issue in the referenced issue as well as making it more confusing for the user. For example, You can specify a type when creating a `StringParameter`, but you shouldn't need to since the only valid values are `String | StringList` and these are modeled as two separate classes `StringParameter & StringListParameter`. To address this, the PR introduces a new enum `ParameterValueType` to model the CloudFormation SSM Parameter Types. This enum is only used in the `valueForXXX` and `fromXXX` methods since those return a CFN parameter. - Deprecated `ssm.StringParameter.valueForTypedStringParameter` since it uses the old overloaded `ParameterType`. - Introduce a new `ssm.StringParameter.valueForTypedStringParameterV2` that uses the new `ParameterValueType` enum - Add `ssm.StringListParameter.valueForTypedListParameter` - Add `ssm.StringListParameter.fromListParameterAttributes` - Deprecated `StringParameterProps.type` since the value should only be `String`. fix #12477

gitpod-io · 2022-09-01T17:25:48Z

Naumel · 2022-09-02T07:48:21Z

@Mergifyio update

mergify · 2022-09-02T07:49:00Z

update

✅ Branch has been successfully updated

Naumel · 2022-09-12T13:58:37Z

@Mergifyio update

mergify · 2022-09-12T13:59:59Z

update

✅ Branch has been successfully updated

Naumel · 2022-09-13T09:14:47Z

packages/@aws-cdk/aws-ssm/lib/parameter.ts

  readonly type?: ParameterType;
+
+  /**
+   * The type of the string parameter value
+   *
+   * @default ParameterValueType.STRING
+   */
+  readonly valueType?: ParameterValueType;
+}


The changes look fine, the meta-meta aspect for the names is making me do double-taken when reviewing them.

Could some of the other names you considered make it easier to read, even if it is more verbose?

hmm I'm not sure, I was having a hard time coming up with an alternative for type ha. Any suggestions?

It's definitely confusing because we are modeling two different types of SSM parameters in the same construct, which each have the concept of type.

rix0rrr · 2022-09-15T10:02:37Z

packages/@aws-cdk/aws-ssm/lib/parameter.ts

+   *
+   * @default ParameterValueType.STRING
+   */
+  readonly valueType?: ParameterValueType;


For lists, can we use a term like elementType ? (throughout, where necessary)

rix0rrr · 2022-09-15T10:05:31Z

packages/@aws-cdk/aws-ssm/lib/parameter.ts

+   * @param type the type of the SSM list parameter
+   * @param version The parameter version (recommended in order to ensure that the value won't change during deployment)
+   */
+  public static valueForTypedListParameter(scope: Construct, parameterName: string, type = ParameterValueType.STRING, version?: number): string[] {


Does typing this value even add value? (ha)

We return it as a string[] anyway... so does anybody does any checking on the types at all? If so, who?

CloudFormation does the type checking in this case. If you request a ParameterValueType.AWS_EC2_IMAGE_ID it expects the value to be a string in the AMI format, and if it is not it will throw an error.

Naumel · 2022-09-15T12:08:39Z

@Mergifyio update

mergify · 2022-09-15T12:09:12Z

update

✅ Branch has been successfully updated

mergify · 2022-09-15T12:45:39Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-09-15T13:23:22Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 145dd4e
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-09-15T13:23:50Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

This PR fixes some issues with how SSM parameter types are implemented. Currently this module models a single type of parameter (`ParameterType` enum) and that type is used to represent _both_ [CloudFormation SSM Parameter types](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/parameters-section-structure.html#aws-ssm-parameter-types) For example, ```ts new cdk.CfnParameter(this, 'Param', { type: 'AWS::SSM::Parameter::Value<String>', // type }); ``` _and_ the [AWS::SSM::Parameter.type](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ssm-parameter.html#cfn-ssm-parameter-type) For example, ```ts new ssm.CfnParameter(this, 'Param', { type: 'String', }); ``` This overloading caused the issue in the referenced issue as well as making it more confusing for the user. For example, You can specify a type when creating a `StringParameter`, but you shouldn't need to since the only valid values are `String | StringList` and these are modeled as two separate classes `StringParameter & StringListParameter`. To address this, the PR introduces a new enum `ParameterValueType` to model the CloudFormation SSM Parameter Types. This enum is only used in the `valueForXXX` and `fromXXX` methods since those return a CFN parameter. - Deprecated `ssm.StringParameter.valueForTypedStringParameter` since it uses the old overloaded `ParameterType`. - Introduce a new `ssm.StringParameter.valueForTypedStringParameterV2` that uses the new `ParameterValueType` enum - Add `ssm.StringListParameter.valueForTypedListParameter` - Add `ssm.StringListParameter.fromListParameterAttributes` - Deprecated `StringParameterProps.type` since the value should only be `String`. fix aws#12477, aws#14364 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Sep 1, 2022

aws-cdk-automation requested a review from a team September 1, 2022 17:26

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 1, 2022

corymhall added 3 commits September 1, 2022 14:01

Merge branch 'main' into corymhall/ssm/string-list

6fcb966

fixing test

f2224d7

updating usage of deprecated method

a8a69db

Merge branch 'main' into corymhall/ssm/string-list

e73e0fc

Merge branch 'main' into corymhall/ssm/string-list

c62c179

Naumel reviewed Sep 13, 2022

View reviewed changes

rix0rrr reviewed Sep 15, 2022

View reviewed changes

changing name and adding some additional docs

a15da33

Merge branch 'main' into corymhall/ssm/string-list

c72b708

Naumel approved these changes Sep 15, 2022

View reviewed changes

Merge branch 'main' into corymhall/ssm/string-list

145dd4e

mergify bot merged commit 8f7ee2b into aws:main Sep 15, 2022

rednes mentioned this pull request Feb 27, 2023

ssm.StringListParameter does not have version parameter #3586

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ssm): reference existing SSM list parameters #21880

feat(ssm): reference existing SSM list parameters #21880

corymhall commented Sep 1, 2022 •

edited

Loading

gitpod-io bot commented Sep 1, 2022

Naumel commented Sep 2, 2022

mergify bot commented Sep 2, 2022

Naumel commented Sep 12, 2022

mergify bot commented Sep 12, 2022

Naumel Sep 13, 2022

corymhall Sep 13, 2022

rix0rrr Sep 15, 2022 •

edited

Loading

rix0rrr Sep 15, 2022

corymhall Sep 15, 2022

Naumel commented Sep 15, 2022

mergify bot commented Sep 15, 2022

mergify bot commented Sep 15, 2022

aws-cdk-automation commented Sep 15, 2022

mergify bot commented Sep 15, 2022

feat(ssm): reference existing SSM list parameters #21880

feat(ssm): reference existing SSM list parameters #21880

Conversation

corymhall commented Sep 1, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Sep 1, 2022

Naumel commented Sep 2, 2022

mergify bot commented Sep 2, 2022

✅ Branch has been successfully updated

Naumel commented Sep 12, 2022

mergify bot commented Sep 12, 2022

✅ Branch has been successfully updated

Naumel Sep 13, 2022

Choose a reason for hiding this comment

corymhall Sep 13, 2022

Choose a reason for hiding this comment

rix0rrr Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

rix0rrr Sep 15, 2022

Choose a reason for hiding this comment

corymhall Sep 15, 2022

Choose a reason for hiding this comment

Naumel commented Sep 15, 2022

mergify bot commented Sep 15, 2022

✅ Branch has been successfully updated

mergify bot commented Sep 15, 2022

aws-cdk-automation commented Sep 15, 2022

AWS CodeBuild CI Report

mergify bot commented Sep 15, 2022

corymhall commented Sep 1, 2022 •

edited

Loading

rix0rrr Sep 15, 2022 •

edited

Loading