-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
resource/aws_ram_resource_share_accepter: Fix Read operation since invitations are purged after 7 days #11562
Conversation
Please advise. I will keep it as a WIP until 7 days have passed, the invitations for my existing shares have being purged by AWS, so that I can confirm the code works as intended (so far I can only confirm that there is no regression with the existing acceptance test). |
I did some local tests faking the fact that no invitation was still present. It showed me a significant remaining bug in the "import" behavior. I made necessary changes and from my tests everything seems to be fine now. |
* `status` - The status of the invitation (e.g., ACCEPTED, REJECTED). | ||
* `status` - The status of the resource share (ACTIVE, PENDING, FAILED, DELETING, DELETED). |
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.
Should we document such a breaking change somewhere else? I assume almost nobody uses this attribute for something else, but ...
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.
I doubt anyone is really using it, but it wouldn't hurt to mention it in the changelog or somewhere. Not sure where breaking changes are normally logged for the providers.
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.
Changes like these can be noted in the CHANGELOG.md
file, which the maintainers will handle on merge. Always good to call this out in PRs though. 👍
if invitation != nil { | ||
d.Set("invitation_arn", invitation.ResourceShareInvitationArn) | ||
d.Set("receiver_account_id", invitation.ReceiverAccountId) | ||
} else { | ||
d.Set("receiver_account_id", meta.(*AWSClient).accountid) |
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.
If invitation is still present, we source both attributes from the invitation.
If the invitation is not present anymore, we source only the receiver_account_id from the current client accountid.
if *invitation.ReceiverAccountId != client.accountid { | ||
// Report an error, this won't work on the long term since once the invitation has disappeared | ||
// the receiver_account_id is populated with the account_id (and same during import) | ||
return nil, fmt.Errorf("Unexpected behavior while reading RAM resource share invitation %s: receiver_account_id is %s while current account_id is %s", resourceShareARN, *invitation.ReceiverAccountId, client.accountid) | ||
} | ||
|
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.
I see no reason why this could happen, but I prefer to have the check in place to be notified if this happens under a corner-case condition, since this would cause the bugs fixed by this change to reappear under this corner-case condition.
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.
We may also want to remove this since its not guaranteed client.accountid
is populated in the case it cannot be detected automatically via other means and the provider skip_requesting_account_id
argument is set.
Testing is now complete and sucessful (I do not have any more invitations is my calls to AWS but terraform does not tries anymore to re-accept invitations that have already been processed) @bflad : could you please have a look at this PR now that testing is complete? Looking at the associated issue, it seems that I'm not the only one waiting for this to be merged ;) Thanks in advance ! |
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 @benoit74 👋 Thank you so much for this fix and for this note:
Testing is now complete and sucessful (I do not have any more invitations is my calls to AWS but terraform does not tries anymore to re-accept invitations that have already been processed)
Overall this looks like a preferred solution for this situation. Pulling this in with very minor changes (for reasons mentioned below). 🚀
Output from acceptance testing (while the invitation is still active of course 😅):
--- PASS: TestAccAwsRamResourceShareAccepter_basic (39.67s)
@@ -217,13 +237,13 @@ func resourceAwsRamResourceShareAccepterDelete(d *schema.ResourceData, meta inte | |||
return nil | |||
} | |||
|
|||
func resourceAwsRamResourceShareGetInvitation(conn *ram.RAM, resourceShareARN, status string) (*ram.ResourceShareInvitation, error) { |
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: For consistency across the provider codebase, we prefer these functions to take the service-specific clients.
if *invitation.ReceiverAccountId != client.accountid { | ||
// Report an error, this won't work on the long term since once the invitation has disappeared | ||
// the receiver_account_id is populated with the account_id (and same during import) | ||
return nil, fmt.Errorf("Unexpected behavior while reading RAM resource share invitation %s: receiver_account_id is %s while current account_id is %s", resourceShareARN, *invitation.ReceiverAccountId, client.accountid) | ||
} | ||
|
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.
We may also want to remove this since its not guaranteed client.accountid
is populated in the case it cannot be detected automatically via other means and the provider skip_requesting_account_id
argument is set.
* `status` - The status of the invitation (e.g., ACCEPTED, REJECTED). | ||
* `status` - The status of the resource share (ACTIVE, PENDING, FAILED, DELETING, DELETED). |
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.
Changes like these can be noted in the CHANGELOG.md
file, which the maintainers will handle on merge. Always good to call this out in PRs though. 👍
…anges Reference: #11562 (review) Output from acceptance testing: ``` --- PASS: TestAccAwsRamResourceShareAccepter_basic (39.67s) ```
This has been released in version 2.50.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 #10064
Closes #10186
Closes #11785
Release note for CHANGELOG:
Output from acceptance testing: