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

resource/aws_ssm_parameter added data_type as option for possible validation #13326

Merged
merged 8 commits into from
Jul 31, 2020

Conversation

jayolmos
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #13172

Release note for CHANGELOG:

resource/aws_ssm_parameter: added data_type argument for validation (text or aws:ec2:image) 

Output from acceptance testing:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSSMParameter_dataType'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSSMParameter_dataType -timeout 120m
=== RUN   TestAccAWSSSMParameter_dataType
=== PAUSE TestAccAWSSSMParameter_dataType
=== CONT  TestAccAWSSSMParameter_dataType
--- PASS: TestAccAWSSSMParameter_dataType (81.78s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       81.832s

----

Also tested with a compilation and aws:ec2:image data_type:

terraform code:
resource "aws_ssm_parameter" "foo" {
  name = "foo"
  type = "String"
  data_type = "aws:ec2:image"
  value = "ami-06ce3edf0cff21f07"

terraform apply:
data.aws_subnet_ids.private: Refreshing state...

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # aws_ssm_parameter.foo will be created
  + resource "aws_ssm_parameter" "foo" {
      + arn       = (known after apply)
      + data_type = "aws:ec2:image"
      + id        = (known after apply)
      + key_id    = (known after apply)
      + name      = "foo"
      + tier      = "Standard"
      + type      = "String"
      + value     = (sensitive value)
      + version   = (known after apply)
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

aws_ssm_parameter.foo: Creating...
aws_ssm_parameter.foo: Creation complete after 2s [id=foo]

@jayolmos jayolmos requested a review from a team May 14, 2020 18:01
@ghost ghost added size/S Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. needs-triage Waiting for first response or review from a maintainer. service/ssm Issues and PRs that pertain to the ssm service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels May 14, 2020
go.mod Outdated Show resolved Hide resolved
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels May 15, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @jayolmos 👋 Thank you for contributing this! I found some issues while running the other existing resource tests. Can you please see the below feedback and let us know if you have any questions?

aws/resource_aws_ssm_parameter.go Outdated Show resolved Hide resolved
aws/resource_aws_ssm_parameter.go Show resolved Hide resolved
aws/resource_aws_ssm_parameter_test.go Outdated Show resolved Hide resolved
aws/resource_aws_ssm_parameter.go Outdated Show resolved Hide resolved
@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 1, 2020
@jayolmos
Copy link
Contributor Author

Hello @bflad sorry for the late response. I added all your suggestions to the code and it passed the tests. Thanks for the comments, I was having doubts when started the code specially with the paramInput part, not it's very clear how to proceed. Thanks a lot!

=== RUN TestAccAWSSSMParameter_dataType === PAUSE TestAccAWSSSMParameter_dataType === CONT TestAccAWSSSMParameter_dataType --- PASS: TestAccAWSSSMParameter_dataType (42.92s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 42.953s

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 24, 2020
@bflad bflad self-assigned this Jul 24, 2020
@bflad
Copy link
Contributor

bflad commented Jul 24, 2020

@jayolmos thank you for those updates and no worries! Do you mind quickly double checking all the resource tests? You can use TestAccAWSSSMParameter_ as the test pattern. I thought when I ran the other tests without Computed: true added to the data_type attribute (to tell Terraform to ignore the API added default only for String type parameters), they were failing, but if those tests aren't failing, great!

@jayolmos
Copy link
Contributor Author

Indeed they were failing, I think because the default was missing. I don't know if the SDK should provide this default but I added it to the code and now all tests are OK.

Reference from SDK:
// The data type of the parameter, such as text or aws:ec2:image. The default
// is text.
DataType *string type:"string"

Tests results:
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSSMParameter_ -timeout 120m
=== RUN TestAccAWSSSMParameter_basic
=== PAUSE TestAccAWSSSMParameter_basic
=== RUN TestAccAWSSSMParameter_Tier
=== PAUSE TestAccAWSSSMParameter_Tier
=== RUN TestAccAWSSSMParameter_disappears
=== PAUSE TestAccAWSSSMParameter_disappears
=== RUN TestAccAWSSSMParameter_overwrite
=== PAUSE TestAccAWSSSMParameter_overwrite
=== RUN TestAccAWSSSMParameter_tags
=== PAUSE TestAccAWSSSMParameter_tags
=== RUN TestAccAWSSSMParameter_updateType
=== PAUSE TestAccAWSSSMParameter_updateType
=== RUN TestAccAWSSSMParameter_updateDescription
=== PAUSE TestAccAWSSSMParameter_updateDescription
=== RUN TestAccAWSSSMParameter_changeNameForcesNew
=== PAUSE TestAccAWSSSMParameter_changeNameForcesNew
=== RUN TestAccAWSSSMParameter_fullPath
=== PAUSE TestAccAWSSSMParameter_fullPath
=== RUN TestAccAWSSSMParameter_secure
=== PAUSE TestAccAWSSSMParameter_secure
=== RUN TestAccAWSSSMParameter_dataType
=== PAUSE TestAccAWSSSMParameter_dataType
=== RUN TestAccAWSSSMParameter_secure_with_key
=== PAUSE TestAccAWSSSMParameter_secure_with_key
=== RUN TestAccAWSSSMParameter_secure_keyUpdate
=== PAUSE TestAccAWSSSMParameter_secure_keyUpdate
=== CONT TestAccAWSSSMParameter_basic
=== CONT TestAccAWSSSMParameter_changeNameForcesNew
=== CONT TestAccAWSSSMParameter_dataType
=== CONT TestAccAWSSSMParameter_secure_with_key
=== CONT TestAccAWSSSMParameter_tags
=== CONT TestAccAWSSSMParameter_updateDescription
=== CONT TestAccAWSSSMParameter_updateType
=== CONT TestAccAWSSSMParameter_fullPath
=== CONT TestAccAWSSSMParameter_disappears
=== CONT TestAccAWSSSMParameter_overwrite
=== CONT TestAccAWSSSMParameter_secure_keyUpdate
=== CONT TestAccAWSSSMParameter_Tier
=== CONT TestAccAWSSSMParameter_secure
--- PASS: TestAccAWSSSMParameter_disappears (30.45s)
--- PASS: TestAccAWSSSMParameter_secure (45.05s)
--- PASS: TestAccAWSSSMParameter_fullPath (46.14s)
--- PASS: TestAccAWSSSMParameter_dataType (46.33s)
--- PASS: TestAccAWSSSMParameter_basic (46.43s)
--- PASS: TestAccAWSSSMParameter_secure_with_key (65.15s)
--- PASS: TestAccAWSSSMParameter_updateType (70.45s)
--- PASS: TestAccAWSSSMParameter_updateDescription (70.45s)
--- PASS: TestAccAWSSSMParameter_changeNameForcesNew (72.43s)
--- PASS: TestAccAWSSSMParameter_overwrite (72.66s)
--- PASS: TestAccAWSSSMParameter_secure_keyUpdate (88.79s)
--- PASS: TestAccAWSSSMParameter_tags (100.50s)
--- PASS: TestAccAWSSSMParameter_Tier (100.71s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 100.773s

@bflad bflad added this to the v3.1.0 milestone Jul 29, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thanks, @jayolmos 🚀 Please note I updated the test to verify the aws:ec2:image data type, which required a valid AMI ID (e.g. an Amazon Linux AMI ID).

Output from acceptance testing:

--- PASS: TestAccAWSSSMParameter_disappears (8.61s)
--- PASS: TestAccAWSSSMParameter_basic (12.28s)
--- PASS: TestAccAWSSSMParameter_fullPath (13.42s)
--- PASS: TestAccAWSSSMParameter_secure (16.02s)
--- PASS: TestAccAWSSSMParameter_DataType_AwsEc2Image (18.95s)
--- PASS: TestAccAWSSSMParameter_changeNameForcesNew (21.68s)
--- PASS: TestAccAWSSSMParameter_secure_with_key (23.16s)
--- PASS: TestAccAWSSSMParameter_updateDescription (24.50s)
--- PASS: TestAccAWSSSMParameter_updateType (26.40s)
--- PASS: TestAccAWSSSMParameter_Tier (28.93s)
--- PASS: TestAccAWSSSMParameter_tags (29.42s)
--- PASS: TestAccAWSSSMParameter_secure_keyUpdate (31.56s)
--- PASS: TestAccAWSSSMParameter_overwrite (32.82s)

bflad added a commit that referenced this pull request Jul 31, 2020
…c2:image error handling during read, and acceptance test aws:ec2:image data_type

Reference: #13326

Output from acceptance testing:

```
--- PASS: TestAccAWSSSMParameter_disappears (8.61s)
--- PASS: TestAccAWSSSMParameter_basic (12.28s)
--- PASS: TestAccAWSSSMParameter_fullPath (13.42s)
--- PASS: TestAccAWSSSMParameter_secure (16.02s)
--- PASS: TestAccAWSSSMParameter_DataType_AwsEc2Image (18.95s)
--- PASS: TestAccAWSSSMParameter_changeNameForcesNew (21.68s)
--- PASS: TestAccAWSSSMParameter_secure_with_key (23.16s)
--- PASS: TestAccAWSSSMParameter_updateDescription (24.50s)
--- PASS: TestAccAWSSSMParameter_updateType (26.40s)
--- PASS: TestAccAWSSSMParameter_Tier (28.93s)
--- PASS: TestAccAWSSSMParameter_tags (29.42s)
--- PASS: TestAccAWSSSMParameter_secure_keyUpdate (31.56s)
--- PASS: TestAccAWSSSMParameter_overwrite (32.82s)
```
@bflad bflad merged commit e65a895 into hashicorp:master Jul 31, 2020
bflad added a commit that referenced this pull request Jul 31, 2020
@ghost
Copy link

ghost commented Aug 7, 2020

This has been released in version 3.1.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Aug 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Aug 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ssm Issues and PRs that pertain to the ssm service. size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

resource/aws_ssm_parameter: Add data_type argument
2 participants