Skip to content

fix(ssm): AWS::EC2::Image::Id parameter type#4161

Merged
mergify[bot] merged 9 commits intoaws:masterfrom
logicgram:4013
Sep 23, 2019
Merged

fix(ssm): AWS::EC2::Image::Id parameter type#4161
mergify[bot] merged 9 commits intoaws:masterfrom
logicgram:4013

Conversation

@nmussy
Copy link
Copy Markdown
Contributor

@nmussy nmussy commented Sep 19, 2019 •
edited
Loading

  • use AWS::EC2::Image::Id SSM type for AMI resolved images
  • add valueForTypedStringParameter method to allow this override

Fixes #4013

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

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 19, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

eladb
eladb reviewed Sep 19, 2019
View reviewed changes
@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Sep 19, 2019

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Sep 19, 2019

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Sep 19, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@rix0rrr
Copy link
Copy Markdown
Contributor

rix0rrr commented Sep 20, 2019

The types are only used by and inside CloudFormation, right? And they would not apply if you specify a version, because we would then fall into a {{resolve}} primitive which doesn't take the type at all, correct?

What kind of behavior does the type cause? What are the advantages of specifying it?

@nmussy
Copy link
Copy Markdown
Contributor Author

nmussy commented Sep 20, 2019

I frankly have no idea, sorry 😄

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Sep 20, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

eladb
eladb reviewed Sep 22, 2019
View reviewed changes
packages/@aws-cdk/aws-ssm/lib/parameter.ts
const SECURE_STRING_PARAM_TYPE = 'SecureString';
const STRINGLIST_PARAM_TYPE = 'StringList';
export enum ParameterType {
STRING = 'String',
Copy link
Copy Markdown
Contributor

@eladb eladb Sep 22, 2019

Choose a reason for hiding this comment

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

add documentation to each one (paste from AWS docs)

Copy link
Copy Markdown
Contributor Author

@nmussy nmussy Sep 23, 2019

Choose a reason for hiding this comment

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

I couldn't find a proper documentation page describing the types, but I pieced something together

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Sep 22, 2019

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Sep 23, 2019

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Sep 23, 2019

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Sep 23, 2019

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 48c26c2 into aws:master Sep 23, 2019
@nmussy nmussy deleted the 4013 branch September 23, 2019 08:45
eladb pushed a commit that referenced this pull request Sep 23, 2019
fix(ssm): AWS::EC2::Image::Id parameter type (#4161)
8bb987c 
* fix(ssm): AWS::EC2::Image::Id parameter type

* fix: additional test fixes

* fix: EcsOptimizedImage type

* chore: refactor ParameterType

* chore: ParameterType documentation

* chore: fix new test
@rix0rrr
Copy link
Copy Markdown
Contributor

rix0rrr commented Sep 25, 2019

Sorry to complain about this, but I would like to get back to the following exchange:

What kind of behavior does the type cause? What are the advantages of specifying it?

I frankly have no idea, sorry 😄

Why are we doing this change then?

@nmussy
Copy link
Copy Markdown
Contributor Author

nmussy commented Sep 25, 2019

I looked around when I made the PR for explicit documentation about this use-case, but couldn't find it. What I got is:

If anyone has a better idea, feel free to share.

@kaizencc kaizencc mentioned this pull request Oct 8, 2021
Merged
mergify bot pushed a commit that referenced this pull request Oct 13, 2021
@kaizencc
fix(ssm): StringParameter accepts ParameterType.AWS_EC2_IMAGE_ID as t…
2b353be 
…ype (#16884)

Fixes #16806. Now, setting `type: ssm.ParameterType.AWS_EC2_IMAGE_ID` throws an error and instead, you can set `dataType: 'aws:ec2:image'`. Specifically, the `ssm.ParameterType.AWS_EC2_IMAGE_ID` value is used internally (original [PR](#4161)) in a few places and really shouldn't be exposed to the customer. But I'm not sure what else we can do, especially since this is a stable module.

Original code using `CfnParameter`: 
```ts
const parameter = ssm.CfnParameter(this, "ImageBuilderAMI", {
  type: "String",
  dataType: "aws:ec2:image",
  name: "/ec2-imagebuilder/latest",
  description: "Latest AMI Image",
  value: self.node.try_get_context(env_context)["LinuxAmi"] 
});
```

Can now use `StringParameter`: 
```ts
const parameter = ssm.StringParameter(this, 'ImageBuilderAMI', {
  dataType: 'aws:ec2:image',
  parameterName: '/ec2-imagebuilder/latest',
  description: "Latest AMI Image",
  stringValue: self.node.try_get_context(env_context)["LinuxAmi"]
});
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
@kaizencc @TikiTDO
fix(ssm): StringParameter accepts ParameterType.AWS_EC2_IMAGE_ID as t…
3c5bfe8 
…ype (aws#16884)

Fixes aws#16806. Now, setting `type: ssm.ParameterType.AWS_EC2_IMAGE_ID` throws an error and instead, you can set `dataType: 'aws:ec2:image'`. Specifically, the `ssm.ParameterType.AWS_EC2_IMAGE_ID` value is used internally (original [PR](aws#4161)) in a few places and really shouldn't be exposed to the customer. But I'm not sure what else we can do, especially since this is a stable module.

Original code using `CfnParameter`: 
```ts
const parameter = ssm.CfnParameter(this, "ImageBuilderAMI", {
  type: "String",
  dataType: "aws:ec2:image",
  name: "/ec2-imagebuilder/latest",
  description: "Latest AMI Image",
  value: self.node.try_get_context(env_context)["LinuxAmi"] 
});
```

Can now use `StringParameter`: 
```ts
const parameter = ssm.StringParameter(this, 'ImageBuilderAMI', {
  dataType: 'aws:ec2:image',
  parameterName: '/ec2-imagebuilder/latest',
  description: "Latest AMI Image",
  stringValue: self.node.try_get_context(env_context)["LinuxAmi"]
});
```

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rix0rrr rix0rrr Awaiting requested review from rix0rrr

@skinny85 skinny85 Awaiting requested review from skinny85

@SoManyHs SoManyHs Awaiting requested review from SoManyHs

1 more reviewer

@eladb eladb eladb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

4 participants

@nmussy @aws-cdk-automation @rix0rrr @eladb