-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Add support for package SSM document type #11492
Conversation
a24cf2f
to
7c20fbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ryndaniels 👋 Thanks for working on this -- overall its shaping up good. Please reach out if you have any questions about the feedback items. 👍
aws/resource_aws_ssm_document.go
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
aws/resource_aws_ssm_document.go
Outdated
@@ -384,6 +420,31 @@ func resourceAwsSsmDocumentDelete(d *schema.ResourceData, meta interface{}) erro | |||
return nil | |||
} | |||
|
|||
func expandAttachments(a []interface{}) []*ssm.AttachmentsSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
func expandAttachments(a []interface{}) []*ssm.AttachmentsSource { | |
func expandSsmAttachmentsSources(a []interface{}) []*ssm.AttachmentsSource { |
Type: schema.TypeString, | ||
Required: true, | ||
ValidateFunc: validation.StringInSlice([]string{ | ||
ssm.AttachmentsSourceKeySourceUrl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional value is available:
ssm.AttachmentsSourceKeySourceUrl, | |
ssm.AttachmentsSourceKeyAttachmentReference, | |
ssm.AttachmentsSourceKeySourceUrl, |
aws/resource_aws_ssm_document.go
Outdated
@@ -39,6 +39,32 @@ func resourceAwsSsmDocument() *schema.Resource { | |||
Required: true, | |||
ValidateFunc: validateAwsSSMName, | |||
}, | |||
"attachments": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per comment in read function, recommend naming this attachments_source
(singular for configuration blocks)
"attachments": { | |
"attachments_source": { |
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
* `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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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! |
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! |
Community Note
Closes #10265
Release note for CHANGELOG:
Output from acceptance testing:
New test:
All tests:
Screenshots for docs: