-
Notifications
You must be signed in to change notification settings - Fork 190
refactor: use retry.StateChangeConf for encryption-at-rest resource. #1477
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
Conversation
I am also going to make sure I apply these proposed changes by Andrea. |
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
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
Refresh: resourceMongoDBAtlasEncryptionAtRestCreateRefreshFunc(ctx, projectID, conn, encryptionAtRestReq), | ||
Timeout: 1 * time.Minute, | ||
MinTimeout: 1 * time.Second, | ||
Delay: 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.
I believe it makes sense to add a delay here to wait before retrying the operation.
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.
Delay here means:
Delay time.Duration // Wait this time before starting checks
for reference:
// StateChangeConf is the configuration struct used for `WaitForState`.
type StateChangeConf struct {
Delay time.Duration // Wait this time before starting checks
Pending []string // States that are "allowed" and will continue trying
Refresh StateRefreshFunc // Refreshes the current state
Target []string // Target state
Timeout time.Duration // The amount of time to wait before timeout
MinTimeout time.Duration // Smallest time to wait before refreshes
PollInterval time.Duration // Override MinTimeout/backoff and only poll this often
NotFoundChecks int // Number of times to allow not found (nil result from Refresh)
// This is to work around inconsistent APIs
ContinuousTargetOccurence int // Number of times the Target state has to occur continuously
}
Description
Please include a summary of the fix/feature/change, including any relevant motivation and context.
Link to any related issue(s): https://jira.mongodb.org/browse/INTMDB-1039
In this change we are removing the
for-loop
used as retry mechanism for theencryption_at_rest
resource creation and using theretry strategy
provided by the terraform plugin sdk.Test
Note: I think we should unit test this the method used in the retry logic itself. But for now I've tested it manually with the following
and succeeded:
I've also tried to "fake" it and return "RETRY" instead of "COMPLETED"
and got (unrelated) error which proves it indeed successfuly created the resource but kept retrying.
Type of change:
Required Checklist:
Further comments