-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws Add support for updating SSM documents #13491
Conversation
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.
Hi @PeteGoo
Thanks for the work here - I left a couple of things inline but this is looking good and the tests pass as expected :)
% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSSSMDocument_' ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/10 14:06:34 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSSSMDocument_ -timeout 120m
=== RUN TestAccAWSSSMDocument_basic
--- PASS: TestAccAWSSSMDocument_basic (50.05s)
=== RUN TestAccAWSSSMDocument_update
--- PASS: TestAccAWSSSMDocument_update (49.99s)
=== RUN TestAccAWSSSMDocument_permission
--- PASS: TestAccAWSSSMDocument_permission (27.21s)
=== RUN TestAccAWSSSMDocument_params
--- PASS: TestAccAWSSSMDocument_params (28.00s)
=== RUN TestAccAWSSSMDocument_automation
--- PASS: TestAccAWSSSMDocument_automation (45.08s)
=== RUN TestAccAWSSSMDocument_documentTypeValidation
--- PASS: TestAccAWSSSMDocument_documentTypeValidation (0.00s)
PASS
ok github.com/hashicorp/terraform/builtin/providers/aws 200.352s
If you think any of the points need addressing then we can followup with a new PR / discussion
Thanks
P.
return errwrap.Wrapf("Error updating SSM document: {{err}}", err) | ||
} else { | ||
log.Printf("[INFO] Updating the default version to the new version %s: %s", newDefaultVersion, d.Id()) | ||
newDefaultVersion = *updated.DocumentDescription.DocumentVersion |
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.
Is this guaranteed to have a value? If not, then we will get a panic. We usually guard against raw dereferencing like this
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.
Good point. I think it'll be fine in this case but I'll keep an eye out in future
_, err = ssmconn.UpdateDocumentDefaultVersion(updateDefaultInput) | ||
|
||
if err != nil { | ||
return errwrap.Wrapf("Error updating the default document version to that of the updated document: {{err}}", err) |
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.
Nice use of ErrWrap
@@ -238,6 +248,23 @@ func resourceAwsSsmDocumentUpdate(d *schema.ResourceData, meta interface{}) erro | |||
log.Printf("[DEBUG] Not setting document permissions on %q", d.Id()) | |||
} | |||
|
|||
if !d.HasChange("content") { |
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.
This will be ok for now, but we need to remember that if there are any other params added to this resource , that updating them has to happen before this check or they will not work
@@ -10,6 +10,10 @@ description: |- | |||
|
|||
Provides an SSM Document resource | |||
|
|||
~> **NOTE on updating SSM documents:** Only documents with a schema version of 2.0 |
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.
Nice addition of a note for users!
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Fixes #12007
schema_version
attribute toaws_ssm_document
.Prune the versions to under 1000 (apparently the limit for number of versions of a single document)Looks like this would take 20 or so api calls to figure out so maybe not.