add name and description to SSM Maintenance Window Tasks #5762

Guimove · 2018-09-04T10:46:04Z

Changes proposed in this pull request:

Add the name and description fields in terraform when creating SSM Maintenance window tasks

Output from acceptance testing:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSSMMaintenanceWindowTarget_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSSMMaintenanceWindowTarget_basic -timeout 120m
=== RUN   TestAccAWSSSMMaintenanceWindowTarget_basic
--- PASS: TestAccAWSSSMMaintenanceWindowTarget_basic (20.48s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	20.497s

...

@Ninir : Please be kind, this is my first contrib :)

Ninir

Hey @Guimove!

Thank you so much for your contribution! Nice first shot 😄

Just left a few comments prior to testing. Let me know if you need any help in this!

aws/resource_aws_ssm_maintenance_window_task.go

aws/resource_aws_ssm_maintenance_window_task_test.go

Guimove · 2018-09-10T14:43:14Z

Hello @Ninir, as discussed with you, I've added the two new validators and updated the test to check the name and description.
Thanks

bflad

Looking good, @Guimove! A few minor bits and this should be ready. 👍

bflad · 2018-09-11T18:16:29Z

aws/resource_aws_ssm_maintenance_window_task.go

+				Type:         schema.TypeString,
+				Optional:     true,
+				ForceNew:     true,
+				ValidateFunc: validateAwsSSMMaintenanceWindowTaskDescription,


We can use an upstream validation function here instead of our own custom one: https://godoc.org/github.com/hashicorp/terraform/helper/validation#StringLenBetween

ValidateFunc: validation.StringLenBetween(0, 128),

Ok for the description but for the name I can keep my validator that check the regex and the length ?

bflad · 2018-09-11T18:18:08Z

aws/resource_aws_ssm_maintenance_window_task.go

@@ -241,6 +257,14 @@ func resourceAwsSsmMaintenanceWindowTaskRead(d *schema.ResourceData, meta interf
 			d.Set("task_arn", t.TaskArn)
 			d.Set("priority", t.Priority)

+			if t.Name != nil {


d.Set() automatically handles conversions of types like *string and we prefer to always call d.Set() for drift detection. (Same with description below)

Ok I'll remove that if

bflad · 2018-09-11T18:19:41Z

aws/resource_aws_ssm_maintenance_window_task_test.go

@@ -141,6 +143,8 @@ resource "aws_ssm_maintenance_window_task" "target" {
  task_type = "RUN_COMMAND"
  task_arn = "AWS-RunShellScript"
  priority = 1
+  name = "TestMaintenanceWindowTask"
+  description = "This ressource is for test purpose only"


Nitpick: Typo ressource should be resource

This is the french touch hehe

bflad · 2018-09-11T18:24:31Z

FYI the documentation also lives in website/docs/r/ssm_maintenance_window_task.html.md for updating that as well. 😄

Guimove · 2018-09-14T09:17:12Z

Updates are done 🤞

Ninir · 2018-09-26T15:12:51Z

aws/resource_aws_ssm_maintenance_window_task.go

+			"name": {
+				Type:         schema.TypeString,
+				Optional:     true,
+				ForceNew:     true,


Is this really forcing a new resource? The API seems to tell the opposite (i.e. it's updatable): https://docs.aws.amazon.com/systems-manager/latest/APIReference/API_UpdateMaintenanceWindowTask.html

Ninir · 2018-09-26T15:12:58Z

aws/resource_aws_ssm_maintenance_window_task.go

+			"description": {
+				Type:         schema.TypeString,
+				Optional:     true,
+				ForceNew:     true,


Ninir · 2018-09-26T15:22:46Z

aws/resource_aws_ssm_maintenance_window_task_test.go

@@ -213,6 +217,8 @@ resource "aws_ssm_maintenance_window_task" "target" {
  task_type = "RUN_COMMAND"
  task_arn = "AWS-RunShellScript"
  priority = 1
+  name = "TestMaintenanceWindowTask"
+  description = "This resource is for test purpose only"


Since they seem to be updatable, we would then be able to make a change to these 2 lines to ensure the update for name and description is OK, as in:

name = "TestMaintenanceWindowTaskUpdate" description = "This resource is for update purpose only"

Thus, we could update TestAccAWSSSMMaintenanceWindowTask_basic for instance to add another TestStep section, as in:

func TestAccAWSSSMMaintenanceWindowTask_basic(t *testing.T) { var task ssm.MaintenanceWindowTask name := acctest.RandString(10) resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, CheckDestroy: testAccCheckAWSSSMMaintenanceWindowTaskDestroy, Steps: []resource.TestStep{ { Config: testAccAWSSSMMaintenanceWindowTaskBasicConfig(name), Check: resource.ComposeTestCheckFunc( testAccCheckAWSSSMMaintenanceWindowTaskExists("aws_ssm_maintenance_window_task.target", &task), resource.TestCheckResourceAttr("aws_ssm_maintenance_window_task.target", "name", "TestMaintenanceWindowTask"), resource.TestCheckResourceAttr("aws_ssm_maintenance_window_task.target", "description", "This ressource is for test purpose only"), ), }, { Config: testAccAWSSSMMaintenanceWindowTaskBasicConfigUpdated(name), Check: resource.ComposeTestCheckFunc( testAccCheckAWSSSMMaintenanceWindowTaskExists("aws_ssm_maintenance_window_task.target", &task), resource.TestCheckResourceAttr("aws_ssm_maintenance_window_task.target", "name", "TestMaintenanceWindowTaskUpdate"), resource.TestCheckResourceAttr("aws_ssm_maintenance_window_task.target", "description", "This ressource is for update purpose only"), ), }, }, }) }

Tell me if you need any help on that!

bflad

Thanks so much @Guimove! Since we haven't heard back in awhile and to not hold up further development, I've gone ahead and added a commit which fixes the merge conflict and typo in the acceptance test. We can treat the lack of SSM Maintenance Window Task updates as a separate enhancement. 👍

--- PASS: TestAccAWSSSMMaintenanceWindowTask_updateForcesNewResource (130.55s)
--- PASS: TestAccAWSSSMMaintenanceWindowTask_basic (228.94s)

bflad · 2018-11-15T00:51:34Z

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

lorengordon · 2018-12-14T21:25:02Z

@Guimove @bflad @Ninir just a heads up that the validation on the name and desciption parameters appears to be running even when the parameters are not passed. The parameters are marked as optional, so this seems counter-intuitive. That's what is causing this issue, which we also just encountered trying to use the community module terraform-aws-ssm-patch-management.

YakDriver · 2019-01-17T19:47:00Z

Please see #7186 which fixes this PR.

ghost · 2020-04-01T17:24:44Z

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!

add name and description to SSM Maintenance Window Tasks

2728e9d

Ninir reviewed Sep 4, 2018

View reviewed changes

aws/resource_aws_ssm_maintenance_window_task.go Outdated Show resolved Hide resolved

aws/resource_aws_ssm_maintenance_window_task.go Outdated Show resolved Hide resolved

aws/resource_aws_ssm_maintenance_window_task_test.go Show resolved Hide resolved

Ninir added enhancement Requests to existing resources that expand the functionality or scope. service/ssm Issues and PRs that pertain to the ssm service. labels Sep 4, 2018

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 4, 2018

Guimove added 3 commits September 7, 2018 18:03

add SSM Window Task validator for name and desc

4004ffb

fmt checked passed

465112b

update acctests to check name and desc exist

d63ff82

bflad reviewed Sep 11, 2018

View reviewed changes

use upstream validation and typo correction

f418a43

ghost added the size/M Managed by automation to categorize the size of a PR. label Sep 14, 2018

update doc with name and description for maintenance window task

2d41b1a

Merge remote-tracking branch 'upstream/master'

dbb2e31

Ninir reviewed Sep 26, 2018

View reviewed changes

sunilkumarmohanty mentioned this pull request Nov 13, 2018

extend aws_ssm_maintenance_window_task resource to accept name, description, task-parameters and Document version. #6431

Closed

bflad added this to the v1.44.0 milestone Nov 14, 2018

bflad approved these changes Nov 14, 2018

View reviewed changes

bflad merged commit dbb2e31 into hashicorp:master Nov 14, 2018

bflad added a commit that referenced this pull request Nov 14, 2018

Update CHANGELOG for #5762

6e3f377

mdlavin mentioned this pull request Nov 21, 2018

V1.46.0 patched lifeomic mdlavin/terraform-provider-aws#3

Closed

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 14, 2018

userhas404d mentioned this pull request Dec 14, 2018

Patch for terraform AWS provider issue #6715 claranet/terraform-aws-ssm-patch-management#3

Open

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add name and description to SSM Maintenance Window Tasks #5762

add name and description to SSM Maintenance Window Tasks #5762

Guimove commented Sep 4, 2018

Ninir left a comment

Guimove commented Sep 10, 2018

bflad left a comment

bflad Sep 11, 2018

Guimove Sep 14, 2018

bflad Sep 11, 2018

Guimove Sep 14, 2018

bflad Sep 11, 2018

Guimove Sep 14, 2018

bflad commented Sep 11, 2018

Guimove commented Sep 14, 2018

Ninir Sep 26, 2018 •

edited

Loading

Ninir Sep 26, 2018

Ninir Sep 26, 2018

bflad left a comment

bflad commented Nov 15, 2018

lorengordon commented Dec 14, 2018

YakDriver commented Jan 17, 2019

ghost commented Apr 1, 2020

add name and description to SSM Maintenance Window Tasks #5762

add name and description to SSM Maintenance Window Tasks #5762

Conversation

Guimove commented Sep 4, 2018

Ninir left a comment

Choose a reason for hiding this comment

Guimove commented Sep 10, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Sep 11, 2018

Guimove commented Sep 14, 2018

Ninir Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad commented Nov 15, 2018

lorengordon commented Dec 14, 2018

YakDriver commented Jan 17, 2019

ghost commented Apr 1, 2020

Ninir Sep 26, 2018 •

edited

Loading