Add back requireds on stripquery #3443

slevenick · 2020-04-29T23:19:51Z

Reverts changes from #3378 to prevent empty blocks from being valid.

Adds required to the same field that was added in a duplicate object in #3379

This should be cherry-picked into 3.20.0

Release Note Template for Downstream PRs (will be copied)

modular-magician · 2020-04-29T23:25:08Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 54 insertions(+), 60 deletions(-))
Terraform Beta: Diff ( 4 files changed, 54 insertions(+), 60 deletions(-))
Ansible: Diff ( 4 files changed, 41 insertions(+), 42 deletions(-))

ctavan · 2020-04-30T09:14:39Z

products/compute/api.yaml

@@ -9452,11 +9452,11 @@ objects:
                          - :TEMPORARY_REDIRECT
                      - !ruby/object:Api::Type::Boolean
                        name: 'stripQuery'
-                        default_value: false
+                        required: true


There should be 8 occurrences of stripQuery in total:

pathMatchers.routeRules.urlRedirect.stripQuery

pathMatchers.pathRules.urlRedirect.stripQuery

pathMatchers.pathRules.defaultUrlRedirect.stripQuery

defaultUrlRedirect.stripQuery

And these for both for RegionUrlMap and UrlMap.

There are, but two of them were not marked as required before. Marking them as required would be a breaking change that can't happen on a minor version

OK. But then it should be noted somewhere, that these two blocks where the non-required stripQuery properties appear exhibit the "empty block" problem that @danawillow pointed out.

modular-magician · 2020-04-30T16:20:59Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 54 insertions(+), 60 deletions(-))
Terraform Beta: Diff ( 4 files changed, 54 insertions(+), 60 deletions(-))
Ansible: Diff ( 4 files changed, 41 insertions(+), 42 deletions(-))

danawillow · 2020-04-30T16:57:14Z

products/compute/api.yaml

                        description: |
                          If set to true, any accompanying query portion of the original URL is removed
                          prior to redirecting the request. If set to false, the query portion of the
-                          original URL is retained.
+                          original URL is retained. The default value is false.


If this is required, it can't have a default

(here and elsewhere)

I was going for saying the default so that people who don't want to specify this field but are required to have an idea of what the default normally is. But can remove

I don't have a strong opinion, but my guess is it would be more confusing to specify it. @ctavan, what do you think?

I just played a bit with the API explorer at https://cloud.google.com/compute/docs/reference/rest/v1/urlMaps/insert

The API does not accept empty defaultUrlRedirect objects, I assume the same is true for the urlRedirect blocks although I didn't double check.

If I try to insert a new urlMap with these contents:

{ "name": "empty-url-map", "defaultUrlRedirect": {} }

I get an error:

{ "error": { "errors": [ { "domain": "global", "reason": "invalid", "message": "Invalid value for field 'resource.defaultUrlRedirect': ''. No fields set for UrlRedirect" } ], "code": 400, "message": "Invalid value for field 'resource.defaultUrlRedirect': ''. No fields set for UrlRedirect" } }

As soon as I set any field the insert works, e.g.:

{ "name": "empty-url-map", "defaultUrlRedirect": { "hostRedirect": "example.com" } }

I believe instead of making an arbitrary field mandatory I suggest that the correct way of handling this would be to specify

at_least_one_of: - host_redirect - path_redirect - prefix_redirect - redirect_response_code - https_redirect - strip_query

for all urlRedirect/defaultUrlRedirect blocks.

This should also be backwards-compatible for people who did specify strip_query and it should also be "forward compatible" with those blocks that currently don't specify mandatory stripQuery fields because empty blocks are not accepted by the compute API anyways.

WDYT?

Yeah, that sounds reasonable.

Ah, unfortunately because these fields are nested within lists we cannot enforce at_least_one_of on them at this point.

We would definitely like to be able to enforce at_least_one_of on nested objects within lists, but the current implementation does not support this

danawillow · 2020-04-30T16:58:17Z

products/compute/api.yaml

@@ -9871,11 +9871,11 @@ objects:
                          - :TEMPORARY_REDIRECT
                      - !ruby/object:Api::Type::Boolean
                        name: 'stripQuery'
-                        default_value: false
+                        required: true


Idea: does it make sense to do these as overrides in terraform.yaml instead? I don't think inspec/ansible have the same constraints around non-empty blocks, and so putting it there would make it easier to call out why we're doing this (we could group them all together more easily)

That makes sense. Do you think it is a good idea to explicitly say in the description "This is only required to ensure we do not allow an empty block in a terraform config"?

modular-magician · 2020-04-30T18:45:50Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 74 insertions(+), 62 deletions(-))
Terraform Beta: Diff ( 4 files changed, 74 insertions(+), 62 deletions(-))
Ansible: Diff ( 4 files changed, 26 insertions(+), 29 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2020-04-30T19:39:20Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 74 insertions(+), 62 deletions(-))
Terraform Beta: Diff ( 4 files changed, 74 insertions(+), 62 deletions(-))
Ansible: Diff ( 4 files changed, 26 insertions(+), 29 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2020-04-30T22:49:18Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 67 insertions(+), 61 deletions(-))
Terraform Beta: Diff ( 4 files changed, 67 insertions(+), 61 deletions(-))
Ansible: Diff ( 4 files changed, 23 insertions(+), 31 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

modular-magician · 2020-04-30T23:02:21Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 66 insertions(+), 60 deletions(-))
Terraform Beta: Diff ( 4 files changed, 66 insertions(+), 60 deletions(-))
Ansible: Diff ( 4 files changed, 17 insertions(+), 28 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

danawillow · 2020-04-30T23:11:28Z

products/compute/api.yaml

@@ -14437,11 +14434,10 @@ objects:
                          - :TEMPORARY_REDIRECT
                      - !ruby/object:Api::Type::Boolean
                        name: 'stripQuery'
-                        default_value: false


Can you add these back so that ansible still has them?

Unfortunately not?

If we have a default value specified in api.yaml we cannot override it with a nil value that I know of

Oh, but I can add them to ansible.yaml

modular-magician · 2020-04-30T23:37:55Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 66 insertions(+), 60 deletions(-))
Terraform Beta: Diff ( 4 files changed, 66 insertions(+), 60 deletions(-))
Ansible: Diff ( 4 files changed, 17 insertions(+), 22 deletions(-))
Inspec: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))

* Add back requireds on stripquery * Move required to terraform.yaml, add description on empty blocks' * Fixed to preserve backwards compatibility * Remove last default in description * Default back to false in ansible urlmaps

googlebot added the cla: yes label Apr 29, 2020

ctavan suggested changes Apr 30, 2020

View reviewed changes

slevenick requested a review from danawillow April 30, 2020 15:27

slevenick force-pushed the stripQuery-required branch from 375fb90 to bcc53f2 Compare April 30, 2020 16:14

danawillow reviewed Apr 30, 2020

View reviewed changes

slevenick force-pushed the stripQuery-required branch from 180880f to 8cb10e7 Compare April 30, 2020 19:34

slevenick added 3 commits April 30, 2020 13:23

Add back requireds on stripquery

bc25e34

Move required to terraform.yaml, add description on empty blocks'

2828e63

Fixed to preserve backwards compatibility

ed03897

slevenick force-pushed the stripQuery-required branch from 8cb10e7 to ed03897 Compare April 30, 2020 22:43

slevenick requested a review from danawillow April 30, 2020 22:55

Remove last default in description

f4cb4f2

danawillow approved these changes Apr 30, 2020

View reviewed changes

Default back to false in ansible urlmaps

87e7d1b

slevenick merged commit 29d82f2 into GoogleCloudPlatform:master May 1, 2020

This was referenced May 1, 2020

Revert stripquery change hashicorp/terraform-provider-google-beta#2023

Merged

Revert stripquery hashicorp/terraform-provider-google#6264

Merged

emilymye mentioned this pull request May 4, 2020

Fix TargetHttpProxy tests hashicorp/terraform-provider-google#6282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add back requireds on stripquery #3443

Add back requireds on stripquery #3443

slevenick commented Apr 29, 2020

modular-magician commented Apr 29, 2020

ctavan Apr 30, 2020

slevenick Apr 30, 2020

ctavan Apr 30, 2020

modular-magician commented Apr 30, 2020

danawillow Apr 30, 2020

slevenick Apr 30, 2020

danawillow Apr 30, 2020

ctavan Apr 30, 2020 •

edited

Loading

slevenick Apr 30, 2020

slevenick Apr 30, 2020 •

edited

Loading

danawillow Apr 30, 2020

slevenick Apr 30, 2020

danawillow Apr 30, 2020

modular-magician commented Apr 30, 2020

modular-magician commented Apr 30, 2020

modular-magician commented Apr 30, 2020

modular-magician commented Apr 30, 2020

danawillow Apr 30, 2020

slevenick Apr 30, 2020

slevenick Apr 30, 2020

modular-magician commented Apr 30, 2020

Add back requireds on stripquery #3443

Add back requireds on stripquery #3443

Conversation

slevenick commented Apr 29, 2020

modular-magician commented Apr 29, 2020

Diff report:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Apr 30, 2020

Diff report:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctavan Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slevenick Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Apr 30, 2020

Diff report:

modular-magician commented Apr 30, 2020

Diff report:

modular-magician commented Apr 30, 2020

Diff report:

modular-magician commented Apr 30, 2020

Diff report:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Apr 30, 2020

Diff report:

ctavan Apr 30, 2020 •

edited

Loading

slevenick Apr 30, 2020 •

edited

Loading