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

Add the ability to set synchronous timeout. #2761

Merged

Conversation

nat-henderson
Copy link
Contributor

@nat-henderson nat-henderson commented Nov 27, 2019

This is useful for old operations like DNS, which are synchronous but also long-running.
This fixes hashicorp/terraform-provider-google#5008.

Release Note Template for Downstream PRs (will be copied)

`all`: Added `synchronous_timeout` to provider block to allow setting higher per-operation-poll timeouts.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, f3d0ce2.

Pull request statuses

No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1449
depends: GoogleCloudPlatform/terraform-google-conversion#274
depends: hashicorp/terraform-provider-google#5013

Copy link
Contributor

@chrisst chrisst left a comment

Choose a reason for hiding this comment

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

I have mild feelings about naming.

Also you should also be updating relevant docs, I think primarily https://www.terraform.io/docs/providers/google/guides/provider_reference.html .

Overall I think it's ok, but there are sharp edges that we should definitely document in a public facing location. EG: many requests are killed by googleapis server side at 30s, so a customer may bump this timeout and end up just seeing a different error message.

If the linked issue suffers from the server side timeout then this is not worth adding until we see an example API that would benefit from it.

@@ -70,6 +70,7 @@ type Config struct {
Scopes []string
BatchingConfig *batchingConfig
UserProjectOverride bool
SynchronousTimeout time.Duration
Copy link
Contributor

Choose a reason for hiding this comment

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

I would lean towards "RequestTimeout" or "HTTPTimeout" as this is going to affect every request that Terraform makes to the API including polling of async resources.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 8410a9f.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, b6417b0.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

@nat-henderson
Copy link
Contributor Author

Lots of passing integration tests - not going to wait for the full suite a second time, results are looking good.

nat-henderson and others added 4 commits December 6, 2019 19:21
This is useful for old operations like DNS, which are synchronous but also long-running.
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
@modular-magician modular-magician merged commit 29b1ba3 into GoogleCloudPlatform:master Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error creating ManagedZone due to timeout
4 participants