-
Notifications
You must be signed in to change notification settings - Fork 722
fix(cloudflare_list_item) fix bulk operation polling and update #5854
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
722632a
to
91c4fcc
Compare
91c4fcc
to
c3f1dec
Compare
@@ -97,6 +98,12 @@ func (r *ListItemResource) Create(ctx context.Context, req resource.CreateReques | |||
return | |||
} | |||
|
|||
err = pollBulkOperation(ctx, data.AccountID.ValueString(), createEnv.Result.OperationID.ValueString(), r.client) |
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 don't know if we own the ctx
instance, so I'm unclear what timeout exists already. Given that, should we have a new context WithTimeout
?
(Ditto other calls to pollBulkOperation
)
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 don't see a context.WithTimeout
used anywhere currently in the provider.
I'm adding a RequestOption
onto the client calls. option.WithRequestTimeout(time.Second*3)
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 looks like in general Terraform has a 20min timeout set for each of the resource operations.
https://developer.hashicorp.com/terraform/plugin/sdkv2/resources/retries-and-customizable-timeouts#default-timeouts-and-deadline-exceeded-errors
@mgirouard @vaishakdinesh Could we get a review on this please, or merge it. If there are any changes I'll have to make a new PR (can't push to this branch). |
@mgirouard @vaishakdinesh This is superseded by #5881 which has more acceptance tests and fixes the behaviour implemented here. The problem was that the search was not reliable at returning the expected item at the first index, and we don't really have a way of updating items so they should just be recreated. |
This can be closed now that the other PR #5881 has been merged. |
Changes being requested
Creating, updating, or deleting list items results in an asynchronous bulk job that must be polled to know when it has completed and the new list item is visible.
It is not possible to update an existing list item by ID, so we must create a new list item and delete the old one.
If the "key" value is the same, they will be merged into the same list item. So we can optimize updates by only requiring replacement if the "key" value has changed.
Additional context & links
Bulk operation status
https://developers.cloudflare.com/api/resources/rules/subresources/lists/subresources/bulk_operations/methods/get/