Adding a failing test/logs that illustrates problem with lifecycle age

[thomasmaclean]

This is related to hashicorp/terraform-provider-google#14044

The issue appears to be caused by a confusion in the way Terraform handles config integer values as 0 vs nil. The API requires a *int64 which only complicates matters because the conversion from the config to the API request could also be a problem as well.

This draft PR illustrates the problem by way of a failing test. I've tried a lot of different ways of resolving the issue within the bucket resource code but so far I haven't been able to come up with a fix.

From where I am now it seems like the central issue is that when converting the config to an API request, Terraform always sees nil values as 0, even after I changed the config for the age field to default to nil. Isolating the relevant changes you get something like this:

// in ResourceStorageBucket()
								Schema: map[string]*schema.Schema{
									"age": {
										Type:        schema.TypeInt,
										Default:     nil,
										Optional:    true,
										Description: `Minimum age of an object in days to satisfy this condition.`,
									},
...
// in expandStorageBucketLifecycleRuleCondition()
	log.Printf("[DEBUG] expanding Age? %s\n\n", condition["age"])
	if v, ok := condition["age"]; v != nil && ok {
		log.Printf("[DEBUG] expanding Age %d\n\n", v)
		age := int64(v.(int))
		transformed.Age = &age
	} else {
		log.Printf("[DEBUG] nil Age %d\n\n", v)
		transformed.NullFields = append(transformed.NullFields, "Age")
	}

When updating a bucket so that the lifecycle rule condition age is unset (i.e. nil) the logs always show the following:

2023/11/22 09:40:33 [DEBUG] expanding Age? %!s(int=0)

2023/11/22 09:40:33 [DEBUG] expanding Age 0

Obviously I would expect the value output here to be nil rather than 0, given that the age field is clearly both optional and nil by default.

Release Note Template for Downstream PRs (will be copied)

[modular-magician]

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will run automatically.

@slevenick, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 146 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 2 files changed, 146 insertions(+), 2 deletions(-))

[modular-magician]

Tests analytics

Total tests: 3238
Passed tests 2907
Skipped tests: 330
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccStorageBucket_lifecycleRuleAge

Get to know how VCR tests work

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccStorageBucket_lifecycleRuleAge[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

[slevenick]

I think the general way we handle this is by adding a boolean that controls if the empty value is sent or not. See zero_max_issuer_path_length in CertificateAuthority to see if the functionality matches what you need

[thomasmaclean]

I think the general way we handle this is by adding a boolean that controls if the empty value is sent or not. See zero_max_issuer_path_length in CertificateAuthority to see if the functionality matches what you need

Based on what I'm seeing it looks like you're adding a Terraform-only field to distinguish between the 0 and nil values. So we'd need something like this to set age to 0:

  lifecycle_rule {
    action {
      type = "Delete"
    }

    condition {
      zero_age   = true
      age        = 0
      with_state = "ARCHIVED"
    }
  }

That way, age would (finally) be nil by default. Is that fair to say?

[slevenick]

I think the general way we handle this is by adding a boolean that controls if the empty value is sent or not. See zero_max_issuer_path_length in CertificateAuthority to see if the functionality matches what you need

Based on what I'm seeing it looks like you're adding a Terraform-only field to distinguish between the 0 and nil values. So we'd need something like this to set age to 0:

  lifecycle_rule {
    action {
      type = "Delete"
    }

    condition {
      zero_age   = true
      age        = 0
      with_state = "ARCHIVED"
    }
  }

That way, age would (finally) be nil by default. Is that fair to say?

I don't think that we can change the existing default as it would be a breaking change. It might have to be switched around so that we keep the existing default and use the boolean to set age to nil. Does that sound right?

[thomasmaclean]

Makes sense. So I'll implement something more like the following:

  lifecycle_rule {
    action {
      type = "Delete"
    }

    condition {
      no_age     = true
      with_state = "ARCHIVED"
    }
  }

[rileykarson]

Hey! I'm closing this PR as a part of a cleanup of older inactive PRs, using a threshold of PRs last updated over 3 months ago. This doesn't represent rejection of the change, and feel free to comment for me to reopen it if you plan to pick it back up, or feel free to start a new PR with the same changes in the future.