provider/aws: add support for group name and path changes with group update function #3237
Conversation
| if err != nil { | ||
| if iamerr, ok := err.(awserr.Error); ok && iamerr.Code() == "NoSuchEntity" { | ||
| d.SetId("") | ||
| return nil |
There was a problem hiding this comment.
This would be a fine thing to do in the Read function, but I don't think this is the best behavior here: if (for some reason) the group gets deleted between the creation of the plan and the application of the plan then this will result in a successful exit but will leave the user with no group resource, even though the config says that the resource should exist. This is confusing, since users will expect the resource to match their config if terraform apply exits without error.
Instead I think it's better to actually fail in this case so we tell the user that it wasn't possible to converge on the settings in the config. If the user then makes a new plan the Read function will detect the resource is gone and the diff will include a create action as expected, allowing Terraform to create a fresh resource.
|
Thanks for doing this, @graycoder! I left one inline comment which is relatively minor. Once that's resolved we can get this merged. 😄 |
|
@apparentlymart I believe I've resolved the issue about swallowing NoSuchEntity errors on the group update. Please let me know if my change isn't quite what you had in mind. |
|
|
||
| request := &iam.GetGroupInput{ | ||
| GroupName: aws.String(d.Id()), | ||
| GroupName: aws.String(name), |
There was a problem hiding this comment.
This change seems superfluous; name isn't used outside of this iam.GetGroupInput struct.
Does it serve some purpose I don't see?
There was a problem hiding this comment.
I thought it made the code clearer.
|
I left some questions inline, otherwise looks good. Let me know when you get a chance to reply/update, and thanks! |
catsby
added
the
waiting-response
|
@catsby I believe this should be up to date and ready to merge. Please let me know if you have any further questions. |
|
This looks good, thanks! |
provider/aws: add support for group name and path changes with group update function
|
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. |
ghost
locked and limited conversation to collaborators
This provides support for updating groups. It removes the need to delete and then recreate the resource.