magic-modules icon indicating copy to clipboard operation
magic-modules copied to clipboard

Default compute backend buckets TTLs dynamically

Open d10i opened this issue 3 years ago • 10 comments

Instead of always defaulting default_ttl, max_ttl and client_ttl, only do it when it makes sense, depending on the value of cache_mode.

Fixes https://github.com/hashicorp/terraform-provider-google/issues/10622.

If this PR is for Terraform, I acknowledge that I have:

  • [X] Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • [X] Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • [X] Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • [ ] Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • [X] Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

compute: fixed a bug when `cache_mode` is set to USE_ORIGIN_HEADERS on `google_compute_backend_bucket`

d10i avatar Dec 07 '21 17:12 d10i

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.

@rileykarson, please review this PR or find an appropriate assignee.

modular-magician avatar Dec 07 '21 17:12 modular-magician

See issue comment for more context on this draft PR.

d10i avatar Dec 07 '21 17:12 d10i

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

Diff report:

Terraform GA: Diff ( 2 files changed, 97 insertions(+), 2 deletions(-)) Terraform Beta: Diff ( 3 files changed, 115 insertions(+), 20 deletions(-)) TF Validator: Diff ( 1 file changed, 55 insertions(+), 2 deletions(-))

modular-magician avatar Dec 07 '21 17:12 modular-magician

@ndmckinley I'm fairly underwater on reviews. Mind picking this up, given you're already engaged in discussions on the linked issue? Thanks!

rileykarson avatar Dec 08 '21 19:12 rileykarson

I think this is right. Let's run the tests - if they pass, I'm good to merge.

/gcbrun

nat-henderson avatar Dec 10 '21 01:12 nat-henderson

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

Diff report:

Terraform GA: Diff ( 3 files changed, 137 insertions(+), 42 deletions(-)) Terraform Beta: Diff ( 2 files changed, 97 insertions(+), 2 deletions(-)) TF Validator: Diff ( 1 file changed, 55 insertions(+), 2 deletions(-))

modular-magician avatar Dec 10 '21 02:12 modular-magician

The test seems to fail.

    provider_test.go:270: Step 5/12 error: After applying this test step, the plan was not empty.                                                                                                                                              
        stdout:                                                                                                        
                                                           
                                                           
        An execution plan has been generated and is shown below.
        Resource actions are indicated with the following symbols:
          ~ update in-place                                
                                                           
        Terraform will perform the following actions:
                                                           
          # google_compute_backend_bucket.foobar will be updated in-place
          ~ resource "google_compute_backend_bucket" "foobar" {
                bucket_name             = "tf-test-uzanb9zfqs"
                creation_timestamp      = "2021-12-13T10:49:57.235-08:00"
                custom_response_headers = []
                enable_cdn              = true
                id                      = "projects/personal-graphite-testing/global/backendBuckets/tf-test-bx9p7rfjnj"
                name                    = "tf-test-bx9p7rfjnj"
                project                 = "personal-graphite-testing"
                self_link               = "https://www.googleapis.com/compute/v1/projects/personal-graphite-testing/global/backendBuckets/tf-test-bx9p7rfjnj"
                                                           
              ~ cdn_policy {                               
                    cache_mode                   = "CACHE_ALL_STATIC"
                  ~ client_ttl                   = 3600 -> 0
                  ~ default_ttl                  = 3600 -> 0
                    max_ttl                      = 86400
                    negative_caching             = true
                    serve_while_stale            = 0
                    signed_url_cache_max_age_sec = 0
                                                           
                    negative_caching_policy {
                        code = 404
                        ttl  = 0
                    }                                      
                }                                          
            }                                              
                         

nat-henderson avatar Dec 13 '21 18:12 nat-henderson

Yeah @ndmckinley, see this issue comment for more context.

d10i avatar Dec 14 '21 12:12 d10i

I think the problem is related to GetOk.

c, cdnPolicyOk := d.GetOk("cdn_policy")

unfortunately, cdnPolicyOk will be false if the user doesn't set cdn_policy - but only on initial create. Afterwards, it will be true, and c will be 0. This is an annoying detail of Terraform's internal serialization logic, which we can't really work around.

If I'm right, the linked issue is not fixable this way. Very frustrating! We probably won't merge a PR that breaks existing tests or examples, even if it improves behavior for a different set of users, so you'd need to find another way to express this. I'm not really sure what that way would be... I don't currently know how to fix this so it works for everyone.

nat-henderson avatar Jan 04 '22 21:01 nat-henderson

What's the status here? Is there anything we could do to speed up the fix? ☺️

IchordeDionysos avatar Sep 01 '22 16:09 IchordeDionysos