Skip to content
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

provider timeouts #19222

Merged
merged 4 commits into from
Oct 30, 2018
Merged

provider timeouts #19222

merged 4 commits into from
Oct 30, 2018

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Oct 30, 2018

Resource timeouts were a separate config block, but did not exist in the
resource schema. Insert any defined timeouts when generating the
configshema.Block so that the fields can be accepted and validated by
core.

helper/schema will remove "timeouts" from the config, and stash them in
the diff.Meta map. Terraform sees "timeouts" as a regular config block,
so needs them to be present in the state in order to not show a diff.
Have the GRPCProviderServer shim copy all timeout values into any state
it returns to provide consistent diffs in core.

@jbardin jbardin requested a review from a team October 30, 2018 17:13
Resource timeouts were a separate config block, but did not exist in the
resource schema. Insert any defined timeouts when generating the
configshema.Block so that the fields can be accepted and validated by
core.
helper/schema will remove "timeouts" from the config, and stash them in
the diff.Meta map. Terraform sees "timeouts" as a regular config block,
so needs them to be present in the state in order to not show a diff.

Have the GRPCProviderServer shim copy all timeout values into any state
it returns to provide consistent diffs in core.
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I left a question inline, but I don't think it's a blocker for merging this in.

@@ -172,6 +172,53 @@ func (r *Resource) CoreConfigSchema() *configschema.Block {
}
}

// insert configured timeout values into the schema
if r.Timeouts != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I like that it's possible to leave this out of the schema in the common case where it's not used. I was worried initially that we'd have a redundant extra timeouts thing in every schema!

}
}

block.BlockTypes["timeouts"] = &configschema.NestedBlock{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there already an InternalValidate step somewhere that prohibits a resource schema from containing a normal attribute called timeouts? Not absolutely required right now of course, since any existing one like that would already be broken in some sense, but given that this feature is not well-known I worry that provider developers might try it in future and get confused about the weird behavior that will inevitably result, particularly if they define timeouts as something that ends up in block.Attributes and so it'd actually be in both maps. 😱

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you mention it, I think I'll add a check that this isn't overriding a timeouts block or attribute. This would probably leave it closer to the original behavior.

Don't overwrite anything the provider defined, in order to maintain
existing behavior.

Change strings to pre-defined constants
@ghost
Copy link

ghost commented Mar 31, 2020

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 ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants