fix: use null as default for lookup

[cyuanli]

Currently, any retention_policy throws this error:

╷
│ Error: Invalid function argument
│
│   on .terraform\modules\my_buckets.gcs_bucket\main.tf line 114, in resource "google_storage_bucket" "buckets":
│  114:     for_each = lookup(var.retention_policy, each.value, {}) != {} ? [var.retention_policy[each.value]] : []
│     ├────────────────
│     │ while calling lookup(inputMap, key, default...)
│
│ Invalid value for "default" parameter: the default value must have the same type as the map elements.
╵

This fixes the default for the lookup, similar to #318. Given that this seems to be a recurring bug, it might be worth looking into

terraform-google-cloud-storage/main.tf

Line 122 in 07e3a4e

for_each = lookup(var.custom_placement_config, each.value, {}) != {} ? [var.custom_placement_config[each.value]] : []

terraform-google-cloud-storage/main.tf

Line 152 in 07e3a4e

for_each = lookup(var.logging, each.value, {}) != {} ? { v = lookup(var.logging, each.value) } : {}

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[cyuanli]

When can I expect a review?

[apeabody]

/gcbrun

[apeabody]

/gcbrun

[cyuanli]

Is there anything else I can provide in order to have this PR reviewed?

[apeabody]

Thanks for the contribution @cyuanli!

This LGTM, would it be possible to update one of the examples to provide test coverage that this resolves the issue you observed?

[cyuanli]

So I realized it might be user error, here's a minimal example that reproduces the error that I get:

resource "random_string" "prefix" {
  length  = 4
  upper   = false
  special = false
}

module "cloud_storage" {
  source  = "terraform-google-modules/cloud-storage/google"
  version = "~> 6.0"

  project_id = var.project_id

  prefix           = "buckets-${random_string.prefix.result}"
  names            = ["one", "two"]

  retention_policy = true ? {
    "one" = {
      is_locked        = false
      retention_period = 3600
    }
  } : {}
}

and without the ternary operator

resource "random_string" "prefix" {
  length  = 4
  upper   = false
  special = false
}

module "cloud_storage" {
  source  = "terraform-google-modules/cloud-storage/google"
  version = "~> 6.0"

  project_id = var.project_id

  prefix           = "buckets-${random_string.prefix.result}"
  names            = ["one", "two"]

  retention_policy = {
    "one" = {
      is_locked        = false
      retention_period = 3600
    }
  }
}

it works fine.

I'm aware of conversions made by conditional expressions, but I don't know Terraform well enough to understand why it would complain here. Is that intended behavior?

In any case, I leave it up to the maintainers to judge if this PR is valuable still 😁

Edit: This also works

resource "random_string" "prefix" {
  length  = 4
  upper   = false
  special = false
}

module "cloud_storage" {
  source  = "terraform-google-modules/cloud-storage/google"
  version = "~> 6.0"

  project_id = var.project_id

  prefix           = "buckets-${random_string.prefix.result}"
  names            = ["one", "two"]

  retention_policy = {}
}

In any case, null would work for all the cases.

[apeabody]

retention_policy = true ? {

Thanks @cyuanli - What is the intention of true in the first example? Should that be var.something or local.something?

If we have a syntactically correct reproducible test case this resolves, I'm absolutely happy to merge it.

[cyuanli]

I added retention_policy in the example and adapted the integration test, let me know if I should adjust anything.

retention_policy = true ? {

Thanks @cyuanli - What is the intention of true in the first example? Should that be var.something or local.something?

I tried to keep it minimal there, in reality it's more something like

retention_policy = var.my_retention_policy != null ? { (var.my_bucket_name) = var.my_retention_policy } : {}

[cyuanli]

For reference, created an issue for terraform here: hashicorp/terraform#35662

Very curious to find out if that is intended behavior 😄

[cyuanli]

hashicorp/terraform#35662 (comment)

As explained here, the empty map shouldn't be used in these cases. I replaced them with nulls and added one integration test, let me know if I should adjust anything.

[apeabody]

/gcbrun

[apeabody]

Hi @cyuanli!

Great to have test coverage, but can we set retention_period for an extremely short period, as currently it blocks the CI test lifecycle :)

        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; 
        	            	Error: Error deleting contents of object prod/: googleapi: Error 403: Object 'multiple-buckets-gpev-two-b249/prod/' is subject to bucket's retention policy or object retention and cannot be deleted or overwritten until 2024-09-03T09:46:50.928463-07:00, retentionPolicyNotMet
        	            	
        	            	
        	            	Error: Error deleting contents of object dev/: googleapi: Error 403: Object 'multiple-buckets-gpev-two-b249/dev/' is subject to bucket's retention policy or object retention and cannot be deleted or overwritten until 2024-09-03T09:46:50.706047-07:00, retentionPolicyNotMet
        	            	}
        	Test:       	TestMultipleBuckets

[cyuanli]

Ah of course, set it to 1 ✌️

[apeabody]

/gcbrun

[apeabody]

Thanks for the contribution @cyuanli!