-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adds startup_probe
field to google_cloud_run_service
resource for beta
#6532
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @rileykarson, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudRunService_probes|TestAccCGCSnippet_eventarcWorkflowsExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
startup_probe
field to google_cloud_run_service
resource.startup_probe
field to google_cloud_run_service
resource for beta
I don't think the failed test is relative to my changes. |
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.
Made a first pass! I have a few more thoughts about what we want to test, but those are contingent on how we handle default values for the integer fields.
mmv1/templates/terraform/examples/cloud_run_service_probes.tf.erb
Outdated
Show resolved
Hide resolved
mmv1/templates/terraform/examples/cloud_run_service_probes.tf.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_cloud_run_service_test.go.erb
Outdated
Show resolved
Hide resolved
Yep, you're all good. Sorry for the noise! |
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.
Looks like generation got blocked, that's a bug on our end. Details on how to resolve inline (along with some more implementation questions I was hoping to answer myself from the test logs post-generation. Sorry for the extra qs!)
default_from_api: true | ||
spec.template.spec.containers.startupProbe.tcpSocket.port: !ruby/object:Overrides::Terraform::PropertyOverride | ||
default_from_api: true | ||
spec.template.spec.containers.startupProbe.httpGet.path: !ruby/object:Overrides::Terraform::PropertyOverride |
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.
This indicates that the server will pick a non-fixed default for path when not provided. Is that correct?
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.
The rules are:
- Not specified -> "/"
- Empty string -> "/"
- Other string -> keep the input
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.
In that case, should we consider a client-side default_value
or "/"
? That means that if a user doesn't specify anything, they'll get "/"
which is the same behaviour as the API.
default_from_api
preserves values when the field is unset rather than resets to default, so it's generally undesirable if the default value is fixed.
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 tried default_value = "/"
.
If I don't specify path
, the test passed.
If I specify path=""
, the test failed due to
~ http_get {
- path = "/" -> null
# (3 unchanged blocks hidden)
}
I think Terraform treats "" as an input, not unset.
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudRunService_cloudRunServiceProbesExample|TestAccCloudRunService_probes|TestAccComputeForwardingRule_update|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccFirebaserulesRelease_BasicRelease |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudRunService_cloudRunServiceProbesExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
For the failed run of TestAccCloudRunService_cloudRunServiceProbesExample, it is expected that I'm not sure why other examples tests succeeded. Shall I update |
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.
Looks fine overall, had some follow-up questions on the meaning of empty name&value in the headers fields
mmv1/products/cloudrun/api.yaml
Outdated
description: |- | ||
The header field name. | ||
default_value: "" | ||
send_empty_value: true |
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.
What does empty mean here? What I want to understand is why a user would want to specify empty rather than some value. Sending an empty header field doesn't make sense, so does empty have a special meaning to the API?
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.
You're right, empty header name doesn't make sense. I checked what GKE does and GKE doesn't allow empty. Our API needs a change.
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.
Should we wait on that change before releasing this then?
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 have similar questions about the other value field next to this
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 think we need to wait since in Terraform side this field is required.
For header value, IIUC, empty is a valid http header value. Just verified GKE also supports empty value.
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccCloudRunService_probes|TestAccBigQueryDataTable_bigtable |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
Just one typo change, then should be good to go
2 quick questions from an interested party:
|
Yeah, if that's true they should either have conflicts_with or exactly_one_of on both. @yanweiguo can you add that if it is appropriate? You can find examples of both in the code base, one thing to note is that you need to hard-code index 0 for nested objects due to how TF represents objects as lists in the underlying structure. |
Co-authored-by: Sam Levenick <slevenick@google.com>
Yes. Coming soon.
Added. PTAL |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Part of hashicorp/terraform-provider-google#12532.
Adds
startup_probe
field togoogle_cloud_run_service
resource for google beta provider.If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)