Add support for package SSM document type #11492

ryndaniels · 2020-01-06T18:16:38Z

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

Release note for CHANGELOG:

ENHANCEMENTS:
* resource/aws_ssm_document: Add support for "Package" document type

Output from acceptance testing:

New test:

$ make testacc TESTARGS="-run=TestAccAWSSSMDocument_package"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSSSMDocument_package -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSSSMDocument_package
=== PAUSE TestAccAWSSSMDocument_package
=== CONT  TestAccAWSSSMDocument_package
--- PASS: TestAccAWSSSMDocument_package (146.95s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       147.861s

All tests:

$ make testacc TESTARGS="-run=TestAccAWSSSMDocument_"       
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSSSMDocument_ -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSSSMDocument_basic
=== PAUSE TestAccAWSSSMDocument_basic
=== RUN   TestAccAWSSSMDocument_update
=== PAUSE TestAccAWSSSMDocument_update
=== RUN   TestAccAWSSSMDocument_permission_public
=== PAUSE TestAccAWSSSMDocument_permission_public
=== RUN   TestAccAWSSSMDocument_permission_private
=== PAUSE TestAccAWSSSMDocument_permission_private
=== RUN   TestAccAWSSSMDocument_permission_batching
=== PAUSE TestAccAWSSSMDocument_permission_batching
=== RUN   TestAccAWSSSMDocument_permission_change
=== PAUSE TestAccAWSSSMDocument_permission_change
=== RUN   TestAccAWSSSMDocument_params
=== PAUSE TestAccAWSSSMDocument_params
=== RUN   TestAccAWSSSMDocument_automation
=== PAUSE TestAccAWSSSMDocument_automation
=== RUN   TestAccAWSSSMDocument_package
=== PAUSE TestAccAWSSSMDocument_package
=== RUN   TestAccAWSSSMDocument_SchemaVersion_1
=== PAUSE TestAccAWSSSMDocument_SchemaVersion_1
=== RUN   TestAccAWSSSMDocument_session
=== PAUSE TestAccAWSSSMDocument_session
=== RUN   TestAccAWSSSMDocument_DocumentFormat_YAML
=== PAUSE TestAccAWSSSMDocument_DocumentFormat_YAML
=== RUN   TestAccAWSSSMDocument_Tags
=== PAUSE TestAccAWSSSMDocument_Tags
=== CONT  TestAccAWSSSMDocument_basic
=== CONT  TestAccAWSSSMDocument_automation
=== CONT  TestAccAWSSSMDocument_Tags
=== CONT  TestAccAWSSSMDocument_DocumentFormat_YAML
=== CONT  TestAccAWSSSMDocument_SchemaVersion_1
=== CONT  TestAccAWSSSMDocument_package
=== CONT  TestAccAWSSSMDocument_permission_private
=== CONT  TestAccAWSSSMDocument_permission_batching
=== CONT  TestAccAWSSSMDocument_params
=== CONT  TestAccAWSSSMDocument_permission_public
=== CONT  TestAccAWSSSMDocument_permission_change
=== CONT  TestAccAWSSSMDocument_update
=== CONT  TestAccAWSSSMDocument_session
--- PASS: TestAccAWSSSMDocument_params (44.54s)
--- PASS: TestAccAWSSSMDocument_session (44.60s)
--- PASS: TestAccAWSSSMDocument_basic (45.38s)
--- PASS: TestAccAWSSSMDocument_permission_batching (47.24s)
--- PASS: TestAccAWSSSMDocument_permission_public (47.78s)
--- PASS: TestAccAWSSSMDocument_permission_private (47.96s)
--- PASS: TestAccAWSSSMDocument_automation (58.91s)
--- PASS: TestAccAWSSSMDocument_update (69.47s)
--- PASS: TestAccAWSSSMDocument_SchemaVersion_1 (69.77s)
--- PASS: TestAccAWSSSMDocument_DocumentFormat_YAML (69.79s)
--- PASS: TestAccAWSSSMDocument_Tags (97.47s)
--- PASS: TestAccAWSSSMDocument_permission_change (98.74s)
--- PASS: TestAccAWSSSMDocument_package (147.51s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       149.687s

Screenshots for docs:

bflad

Hey @ryndaniels 👋 Thanks for working on this -- overall its shaping up good. Please reach out if you have any questions about the feedback items. 👍

bflad · 2020-01-09T03:10:38Z

aws/resource_aws_ssm_document.go

@@ -266,6 +297,11 @@ func resourceAwsSsmDocumentRead(d *schema.ResourceData, meta interface{}) error

 	d.Set("status", doc.Status)

+	if v, ok := d.GetOk("attachments"); ok {
+		// The API doesn't currently return attachment information so it has to be set this way


It appears that the SSM GetDocument API does returns attachment information, but in a different form AttachmentContent structure. It is generally better to perform the drift detection and support full resource import, but the mismatched structures will make the implementation harder, if not impossible. 🙁

Since the Create/Update and Read APIs do not share a common API structure, this would have potentially been a case where we could smooth over the user experience in Terraform and support a configuration like the following:

attachment { name = "" url = "" }

Where on Create/Update the Terraform logic converted those to the requisite AttachmentSource objects by automatically inferring the Key value from the URL (e.g. S3FileUrl if url prefix is s3://, SourceUrl if prefix is http(s):// etc). On Read, it would be a simple "passthrough" of the API structure.

However, it appears AttachmentsSource SourceUrl may support providing the URL to a whole S3 "folder" of files. This concept does not map well into Terraform if the API expands all those into individual AttachmentContents objects on Read. Bummer.

It may be best in this case then to just rename this attribute directly attachments_source (prefer singular for configuration block names) to match the Create/Update API and denote to operators its exact usage.

We can also skip this d.GetOk() and d.Set() logic in the Read function since it is a root level attribute. Terraform will automatically "passthrough" a given configuration into the Terraform state if it does not have d.Set() called on it to overwrite the value during refresh.

bflad · 2020-01-09T03:14:23Z

aws/resource_aws_ssm_document.go

@@ -384,6 +420,31 @@ func resourceAwsSsmDocumentDelete(d *schema.ResourceData, meta interface{}) erro
 	return nil
 }

+func expandAttachments(a []interface{}) []*ssm.AttachmentsSource {


Nit: Resources and their functions currently live in the shared aws Go package across all services and resources. We should prefer to name functions with their service until this shared package is split up.

Suggested change

func expandAttachments(a []interface{}) []*ssm.AttachmentsSource {

func expandSsmAttachmentsSources(a []interface{}) []*ssm.AttachmentsSource {

bflad · 2020-01-09T03:16:55Z

aws/resource_aws_ssm_document.go

+							Type:     schema.TypeString,
+							Required: true,
+							ValidateFunc: validation.StringInSlice([]string{
+								ssm.AttachmentsSourceKeySourceUrl,


Additional value is available:

Suggested change

ssm.AttachmentsSourceKeySourceUrl,

ssm.AttachmentsSourceKeyAttachmentReference,

ssm.AttachmentsSourceKeySourceUrl,

bflad · 2020-01-09T03:18:03Z

aws/resource_aws_ssm_document.go

@@ -39,6 +39,32 @@ func resourceAwsSsmDocument() *schema.Resource {
 				Required:     true,
 				ValidateFunc: validateAwsSSMName,
 			},
+			"attachments": {


Per comment in read function, recommend naming this attachments_source (singular for configuration blocks)

Suggested change

"attachments": {

"attachments_source": {

bflad · 2020-01-09T03:20:41Z

website/docs/r/ssm_document.html.markdown

@@ -48,12 +48,21 @@ DOC
 The following arguments are supported:

 * `name` - (Required) The name of the document.
+* `attachments` - (Optional) A list of key/value pairs describing attachments to a version of a document. Defined below.


Nit: Recommend shying away from "list" and "key/value pairs" when referring to a configuration block attributes, since in Terraform 0.12+ they all mean semantically different things (and Terraform 0.12 removed the old attachments_source = [{}] syntax you could do in some undocumented cases).

Suggested change

* `attachments` - (Optional) A list of key/value pairs describing attachments to a version of a document. Defined below.

* `attachments_source` - (Optional) One or more configuration blocks describing attachments sources to a version of a document. Defined below.

bflad

LGTM 🚀

Output from acceptance testing:

--- PASS: TestAccAWSSSMDocument_permission_public (21.91s)
--- PASS: TestAccAWSSSMDocument_session (24.29s)
--- PASS: TestAccAWSSSMDocument_permission_batching (24.94s)
--- PASS: TestAccAWSSSMDocument_params (38.59s)
--- PASS: TestAccAWSSSMDocument_permission_private (38.60s)
--- PASS: TestAccAWSSSMDocument_automation (39.07s)
--- PASS: TestAccAWSSSMDocument_DocumentFormat_YAML (43.03s)
--- PASS: TestAccAWSSSMDocument_basic (43.31s)
--- PASS: TestAccAWSSSMDocument_Tags (43.35s)
--- PASS: TestAccAWSSSMDocument_SchemaVersion_1 (47.21s)
--- PASS: TestAccAWSSSMDocument_update (55.54s)
--- PASS: TestAccAWSSSMDocument_permission_change (78.47s)
--- PASS: TestAccAWSSSMDocument_package (82.12s)

ghost · 2020-01-17T05:04:15Z

This has been released in version 2.45.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 · 2020-03-27T17:43:16Z

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 support for package SSM document type

c2c7826

ryndaniels requested review from bflad and a team January 6, 2020 18:16

ryndaniels removed the needs-triage Waiting for first response or review from a maintainer. label Jan 7, 2020

Merge commits

7c20fbb

ryndaniels force-pushed the f-ssm-documenttype-package branch from a24cf2f to 7c20fbb Compare January 7, 2020 11:46

ghost added size/XXL 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 Jan 7, 2020

Merge branch 'master' into f-ssm-documenttype-package

f4acc06

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Jan 7, 2020

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Jan 9, 2020

bflad suggested changes Jan 9, 2020

View reviewed changes

Rename attachments to attachments_source

8900800

bflad self-assigned this Jan 10, 2020

bflad approved these changes Jan 10, 2020

View reviewed changes

ryndaniels added this to the v2.45.0 milestone Jan 13, 2020

ryndaniels merged commit 639f197 into master Jan 13, 2020

bflad added a commit that referenced this pull request Jan 17, 2020

Update CHANGELOG for #11492

a5df4c9

ghost locked and limited conversation to collaborators Mar 27, 2020

bflad deleted the f-ssm-documenttype-package branch April 23, 2020 03:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for package SSM document type #11492

Add support for package SSM document type #11492

ryndaniels commented Jan 6, 2020

bflad left a comment

bflad Jan 9, 2020

bflad Jan 9, 2020

bflad Jan 9, 2020

bflad Jan 9, 2020

bflad Jan 9, 2020

bflad left a comment

ghost commented Jan 17, 2020

ghost commented Mar 27, 2020

	func expandAttachments(a []interface{}) []*ssm.AttachmentsSource {
	func expandSsmAttachmentsSources(a []interface{}) []*ssm.AttachmentsSource {

	ssm.AttachmentsSourceKeySourceUrl,
	ssm.AttachmentsSourceKeyAttachmentReference,
	ssm.AttachmentsSourceKeySourceUrl,

	* `attachments` - (Optional) A list of key/value pairs describing attachments to a version of a document. Defined below.
	* `attachments_source` - (Optional) One or more configuration blocks describing attachments sources to a version of a document. Defined below.

Add support for package SSM document type #11492

Add support for package SSM document type #11492

Conversation

ryndaniels commented Jan 6, 2020

Community Note

bflad left a comment

Choose a reason for hiding this comment

bflad Jan 9, 2020

Choose a reason for hiding this comment

bflad Jan 9, 2020

Choose a reason for hiding this comment

bflad Jan 9, 2020

Choose a reason for hiding this comment

bflad Jan 9, 2020

Choose a reason for hiding this comment

bflad Jan 9, 2020

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

ghost commented Jan 17, 2020

ghost commented Mar 27, 2020