Add timeoutSeconds to cloud run #3622
Conversation
|
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @danawillow, please review this PR or find an appropriate assignee. |
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 45 insertions(+), 4 deletions(-)) |
|
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=119260" |
products/cloudrun/api.yaml
Outdated
| - !ruby/object:Api::Type::Integer | ||
| name: timeoutSeconds | ||
| description: |- | ||
| TimeoutSeconds holds the max duration the instance is allowed for responding to a request. Not currently used by Cloud Run. |
There was a problem hiding this comment.
I think the sentence "Not currently used by Cloud Run." is more confusing than helpful in this context, and should probably be removed.
There was a problem hiding this comment.
fixed, but it's take from https://cloud.google.com/run/docs/reference/rest/v1/RevisionSpec?hl=ro
There was a problem hiding this comment.
Yeah, it's totally fine to modify descriptions for TF docs so that they make more sense. Using the ones from the GCP docs is just handy as a starting point.
| @@ -152,6 +152,8 @@ overrides: !ruby/object:Overrides::ResourceOverrides | |||
| default_from_api: true | |||
| spec.template.spec.containerConcurrency: !ruby/object:Overrides::Terraform::PropertyOverride | |||
| default_from_api: true | |||
| spec.template.spec.timeoutSeconds: !ruby/object:Overrides::Terraform::PropertyOverride | |||
| default_from_api: true | |||
There was a problem hiding this comment.
Is the default a known value, or might it change depending on how a user configures their service? If we know what it's going to be, it makes more sense to hardcode the default (https://github.com/terraform-providers/terraform-provider-google/wiki/Developer-Best-Practices#default-values)
There was a problem hiding this comment.
it's depends for fully managed or Cloud Run for Anthos.
From documentations https://cloud.google.com/run/docs/configuring/request-timeout#command-line it's 5 minutes for fully managed(but I think it's 15 minutes from my last test).
I think better use default from API, because it's related to user billing because fully managed cloud run users pay for time of request.
There was a problem hiding this comment.
Makes sense, thanks for the explanation!
| @@ -56,6 +56,7 @@ resource "google_cloud_run_service" "default" { | |||
| args = ["arrgs"] | |||
| } | |||
| container_concurrency = %s | |||
| timeoutSeconds = %s | |||
There was a problem hiding this comment.
| timeoutSeconds = %s | |
| timeout_seconds = %s |
sergeylanzman
force-pushed
the
cloud-run-add-timeout
branch
from
caaefde to
a82de46
Compare
|
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @danawillow, please review this PR or find an appropriate assignee. |
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 45 insertions(+), 4 deletions(-)) |
|
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=119371" |
Fixes hashicorp/terraform-provider-google#5924
Release Note Template for Downstream PRs (will be copied)