-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 default_route_action to compute_url_map #3587
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 8291 insertions(+), 3822 deletions(-)) |
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.
added @slevenick and @rambleraptor for inspec and ansible
products/compute/api.yaml
Outdated
exactly_one_of: | ||
- path_matchers.0.default_service | ||
- path_matchers.0.default_url_redirect | ||
# TODO: (mbang) won't work for array path matchers yet, uncomment here once they are supported. |
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.
do you mind linking to an issue number or issue here?
@@ -2500,6 +2500,40 @@ overrides: !ruby/object:Overrides::ResourceOverrides | |||
pathMatchers.pathRules.urlRedirect.stripQuery: !ruby/object:Overrides::Terraform::PropertyOverride | |||
required: true | |||
description: '{{description}} This field is required to ensure an empty block is not set. The normal default value is false.' | |||
pathMatchers.defaultRouteAction.weightedBackendServices.weight: !ruby/object:Overrides::Terraform::PropertyOverride | |||
validation: !ruby/object:Provider::Terraform::Validation | |||
function: 'validation.IntBetween(0, 1000)' |
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 add send_empty values? I'm guessing that if we do add it, it will always send zero which is not really what we want, but I don't think we can unweigh things then. Might just be a limitation we have to live with, backendService is the same way.
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 adding them in here, but i get an error when defaultService
is set because they're conflicting, so we might just have to work with it like this.
If not specified, will use the largest timeout among all backend services associated with the route. | ||
properties: | ||
- !ruby/object:Api::Type::String | ||
name: 'seconds' |
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 this be an int like nanos?
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 api lists it as string (int64 format)
and i saw another field similar use string for that. I assumed TypeInt
is just int
and not int64
, but I may have assumed wrong. And now I'm curious if I can use TypeInt
for that?
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.
weird - well, if it works!
- default_route_action.0.fault_injection_policy | ||
properties: | ||
- !ruby/object:Api::Type::Array | ||
name: 'retryConditions' |
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.
does this field need to be a set?
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 think it should be fine as an array - i ran a few tests on it and it seemed to be fine, and also the same retryConditions
is on pathMatchers.routeRules.routeAction.retryPolicy.retryConditions
as an array, so I feel pretty comfortable that it's been working fine.
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 be fine from the InSpec side as its only additions
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 8292 insertions(+), 3822 deletions(-)) |
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.
LGTM if the tests look good
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 8292 insertions(+), 3822 deletions(-)) |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=118742" |
Updated VCR tests - seeds have been recorded: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloud_ProviderGoogleCloudMmUpstreamVcr/118745? |
Fixes hashicorp/terraform-provider-google#6205
And adds the extra fields in default_route_action as well.
Release Note Template for Downstream PRs (will be copied)