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

aws_ssm_maintenance_window_task: allow service_role_arn to be optional #12200

Merged

Conversation

0x91
Copy link
Contributor

@0x91 0x91 commented Feb 27, 2020

The AWS API documentation states:

ServiceRoleArn
The ARN of the IAM service role for Systems Manager to assume when running a maintenance window task. If you do not specify a service role ARN, Systems Manager uses your account's service-linked role. If no service-linked role for Systems Manager exists in your account, it is created when you run RegisterTaskWithMaintenanceWindow.

Therefore, this attribute isn't required. Allowing users to rely on the AWS behaviour simplifies terraform configuration as you do not need to specify and manage an IAM role to use this resource.

I also simplified the acceptance tests for this resource to better re-use some configs. I'm happy to break that into a separate PR if you want.

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 #0000

Release note for CHANGELOG:

* aws_ssm_maintenance_window_task: allow service_role_arn to be optional

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSSMMaintenanceWindowTask'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSSMMaintenanceWindowTask -timeout 120m
=== RUN   TestAccAWSSSMMaintenanceWindowTask_basic
=== PAUSE TestAccAWSSSMMaintenanceWindowTask_basic
=== RUN   TestAccAWSSSMMaintenanceWindowTask_noRole
=== PAUSE TestAccAWSSSMMaintenanceWindowTask_noRole
=== RUN   TestAccAWSSSMMaintenanceWindowTask_updateForcesNewResource
=== PAUSE TestAccAWSSSMMaintenanceWindowTask_updateForcesNewResource
=== RUN   TestAccAWSSSMMaintenanceWindowTask_TaskInvocationAutomationParameters
=== PAUSE TestAccAWSSSMMaintenanceWindowTask_TaskInvocationAutomationParameters
=== RUN   TestAccAWSSSMMaintenanceWindowTask_TaskInvocationLambdaParameters
=== PAUSE TestAccAWSSSMMaintenanceWindowTask_TaskInvocationLambdaParameters
=== RUN   TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParameters
=== PAUSE TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParameters
=== RUN   TestAccAWSSSMMaintenanceWindowTask_TaskInvocationStepFunctionParameters
=== PAUSE TestAccAWSSSMMaintenanceWindowTask_TaskInvocationStepFunctionParameters
=== RUN   TestAccAWSSSMMaintenanceWindowTask_TaskParameters
=== PAUSE TestAccAWSSSMMaintenanceWindowTask_TaskParameters
=== RUN   TestAccAWSSSMMaintenanceWindowTask_emptyNotificationConfig
=== PAUSE TestAccAWSSSMMaintenanceWindowTask_emptyNotificationConfig
=== CONT  TestAccAWSSSMMaintenanceWindowTask_basic
=== CONT  TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParameters
=== CONT  TestAccAWSSSMMaintenanceWindowTask_TaskInvocationAutomationParameters
=== CONT  TestAccAWSSSMMaintenanceWindowTask_noRole
=== CONT  TestAccAWSSSMMaintenanceWindowTask_TaskParameters
=== CONT  TestAccAWSSSMMaintenanceWindowTask_emptyNotificationConfig
=== CONT  TestAccAWSSSMMaintenanceWindowTask_TaskInvocationStepFunctionParameters
=== CONT  TestAccAWSSSMMaintenanceWindowTask_updateForcesNewResource
=== CONT  TestAccAWSSSMMaintenanceWindowTask_TaskInvocationLambdaParameters
--- PASS: TestAccAWSSSMMaintenanceWindowTask_noRole (30.92s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_emptyNotificationConfig (32.38s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskParameters (33.07s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationStepFunctionParameters (33.08s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_updateForcesNewResource (52.86s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_basic (53.34s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationLambdaParameters (57.78s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParameters (74.49s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationAutomationParameters (131.37s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	131.426s...

@0x91 0x91 requested a review from a team February 27, 2020 19:02
@ghost ghost added size/L Managed by automation to categorize the size of a PR. 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. documentation Introduces or discusses updates to documentation. labels Feb 27, 2020
@0x91
Copy link
Contributor Author

0x91 commented Mar 16, 2020

bump?

Base automatically changed from master to main January 23, 2021 00:57
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:57
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Feb 12, 2021
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. size/L Managed by automation to categorize the size of a PR. and removed needs-triage Waiting for first response or review from a maintainer. size/M Managed by automation to categorize the size of a PR. labels Feb 12, 2021
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Feb 12, 2021
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.

Quick rebase fixes

aws/resource_aws_ssm_maintenance_window_task_test.go Outdated Show resolved Hide resolved
aws/resource_aws_ssm_maintenance_window_task_test.go Outdated Show resolved Hide resolved
aws/resource_aws_ssm_maintenance_window_task_test.go Outdated Show resolved Hide resolved
aws/resource_aws_ssm_maintenance_window_task_test.go Outdated Show resolved Hide resolved
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.

Thank you for submitting this, @0x91 👍 After rebase and fixing the Computed issue, this was good to go.

Output from acceptance testing in AWS Commercial:

--- PASS: TestAccAWSSSMMaintenanceWindowTask_disappears (21.36s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_emptyNotificationConfig (22.49s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_noRole (23.79s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationStepFunctionParameters (25.18s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_updateForcesNewResource (37.93s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_basic (38.21s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationAutomationParameters (46.55s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParameters (48.17s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationLambdaParameters (48.58s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParametersCloudWatch (49.26s)

Output from acceptance testing in AWS GovCloud (US):

--- PASS: TestAccAWSSSMMaintenanceWindowTask_noRole (26.04s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_disappears (26.62s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_emptyNotificationConfig (27.01s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationStepFunctionParameters (29.35s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_updateForcesNewResource (43.07s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_basic (43.41s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationLambdaParameters (46.46s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationAutomationParameters (51.63s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParameters (51.98s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParametersCloudWatch (59.12s)

Check: resource.ComposeTestCheckFunc(
testAccCheckAWSSSMMaintenanceWindowTaskExists(resourceName, &task),
),
ExpectNonEmptyPlan: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this type of handling in a test is indicative of a problem that practitioners would face in real world configurations:

=== CONT  TestAccAWSSSMMaintenanceWindowTask_noRole
    resource_aws_ssm_maintenance_window_task_test.go:59: Step 1/1 error: After applying this test step, the plan was not empty.
        stdout:


        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
          ~ update in-place

        Terraform will perform the following actions:

          # aws_ssm_maintenance_window_task.test will be updated in-place
          ~ resource "aws_ssm_maintenance_window_task" "test" {
                id               = "b9865b4e-8afd-40da-89ec-4c90009f40c6"
                name             = "TestMaintenanceWindowTask"
              - service_role_arn = "arn:aws:iam::123456789012:role/aws-service-role/ssm.amazonaws.com/AWSServiceRoleForAmazonSSM" -> null
                # (7 unchanged attributes hidden)


                # (2 unchanged blocks hidden)
            }

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

The solution in this case is to mark service_role_arn with Computed: true to allow Terraform to ignore the value being filled in when it is not configured. 👍

bflad added a commit that referenced this pull request Feb 12, 2021
…mputed, add CHANGELOG for #12200

Output from acceptance testing in AWS Commercial:

```
--- PASS: TestAccAWSSSMMaintenanceWindowTask_disappears (21.36s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_emptyNotificationConfig (22.49s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_noRole (23.79s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationStepFunctionParameters (25.18s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_updateForcesNewResource (37.93s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_basic (38.21s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationAutomationParameters (46.55s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParameters (48.17s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationLambdaParameters (48.58s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParametersCloudWatch (49.26s)
```

Output from acceptance testing in AWS GovCloud (US):

```
--- PASS: TestAccAWSSSMMaintenanceWindowTask_noRole (26.04s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_disappears (26.62s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_emptyNotificationConfig (27.01s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationStepFunctionParameters (29.35s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_updateForcesNewResource (43.07s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_basic (43.41s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationLambdaParameters (46.46s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationAutomationParameters (51.63s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParameters (51.98s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_TaskInvocationRunCommandParametersCloudWatch (59.12s)
```
@bflad bflad merged commit 72cf9cc into hashicorp:main Feb 12, 2021
@github-actions github-actions bot added this to the v3.28.0 milestone Feb 12, 2021
@ghost
Copy link

ghost commented Feb 12, 2021

This has been released in version 3.28.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!

@0x91
Copy link
Contributor Author

0x91 commented Feb 15, 2021

Thanks @bflad apologies I didn't come back to this.

@0x91 0x91 deleted the feat/aws_ssm_maintenance_window_task_optional_role branch February 15, 2021 19:55
@ghost
Copy link

ghost commented Mar 14, 2021

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 as resolved and limited conversation to collaborators Mar 14, 2021
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/M 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.

2 participants