provider: Add AWSClient.StopContext #4179
Conversation
bflad
added
enhancement
| @@ -238,7 +240,7 @@ func (c *AWSClient) IsChinaCloud() bool { | |||
| } | |||
|
|
|||
| // Client configures and returns a fully initialized AWSClient | |||
| func (c *Config) Client() (interface{}, error) { | |||
| func (c *Config) Client() (*AWSClient, error) { | |||
There was a problem hiding this comment.
I guess this change locally makes sense, but I'm curious if there was any particular motivation for it aside from "cleanup"? I'd be potentially worried it will cause some troubles elsewhere where we expect interface{} and cast it to *AWSClient as casting a type which was already casted means crash. 🤔
There was a problem hiding this comment.
Maybe I'm just overly cautious...
There was a problem hiding this comment.
I can switch it back, but I do not see why we need to hide it and it introduces extra type setting. Explicit > Implicit, in my mind anyways. 😄
| if v, ok := d.GetOk("allowed_account_ids"); ok { | ||
| config.AllowedAccountIds = v.(*schema.Set).List() | ||
| } | ||
| // replaces the context between tests |
There was a problem hiding this comment.
Can you be a little bit more specific here about why do we need to do it?
There was a problem hiding this comment.
https://godoc.org/github.com/hashicorp/terraform/helper/schema#Provider
// MetaReset is called by TestReset to reset any state stored in the meta
// interface. This is especially important if the StopContext is stored by
// the provider.
MetaReset func() error
Duplicated from azurerm:
| } | ||
|
|
||
| p.ConfigureFunc = providerConfigure(p) |
There was a problem hiding this comment.
Certainly not a blocker, but this whole extraction makes me think we should perhaps make the StopContext available more easily via ResourceData, else we'll end up doing this in all providers. 😑
There was a problem hiding this comment.
This is how its implemented in azurerm:
There was a problem hiding this comment.
We have a precedent for this implementation pattern, but I second Radek's thoughts on this one, that the ctx should be surfaced in a cleaner manner if possible.
There was a problem hiding this comment.
I wouldn't hold up this PR personally, but I do think its worth looking at a PR in to core as well.
|
FWIW I'm certainly up for addressing the problem on the SDK integration side, but I think the root cause which users hit much more often lies in our retriers which are currently not cancellable, see https://github.com/hashicorp/terraform/issues/11350 |
|
In your retry func can you do something similar to: select {
case <-StopContext.Done():
//return non-retryable
default:
//keep trying...
} |
|
@radeksimko @paultyng where did we land with this PR? |
|
Closing this out as it seems it will not be handled this way. |
|
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! |
ghost
locked and limited conversation to collaborators
Add
?w=1to the PR files URL make it legible.Reference: #4177
Provider smoke tests okay in the test framework 😉