juju / terraform-provider-juju

A Terraform provider for Juju
Apache License 2.0
21 stars 41 forks source link

Add support for "null" on juju_application.config #434

Open mcarvalhor opened 8 months ago

mcarvalhor commented 8 months ago

Description

I believe the config block for juju_application should support null configuration values - indicating unset Juju settings.

From Juju's perspective, "" and unset have different meanings.

Please note that Terraform Configuration below is simplified: we are using multiple variables, modules and plans, and therefore option "simply not include unset settings into config block" is not suitable for complex Terraform code.

Urgency

Annoying bug in our test suite

Terraform Juju Provider version

v0.10.1

Terraform version

v1.7.2

Terraform Configuration(s)

resource "juju_application" "my-test-application" {
  name  = "test-application"
  model = "test-model"
  trust = true

  charm {
    name     = "grafana-k8s"
    channel  = "latest/stable"
    revision = 105
  }

  config = {
    # In a real-world scenario, we use variables and locals.
    # Therefore, "null" should mean unset juju config.
    memory = null
  }

  units = 1
}

Reproduce / Test

terraform init && terraform apply

Debug/Panic Output

│ Error: Value Conversion Error
│ 
│   with module.cos-lite.juju_application.alertmanager,
│   on ../../modules/cos-lite-traefik-ingress/main.tf line 3, in resource "juju_application" "alertmanager":
│    3: resource "juju_application" "alertmanager" {
│ 
│ An unexpected error was encountered trying to build a value. This is always an error in the provider. Please report the following to the provider developer:
│ 
│ Received null value, however the target type cannot handle null values. Use the corresponding `types` package type, a pointer type or a custom type that handles null values.
│ 
│ Path: ["memory"]
│ Target Type: string
│ Suggested `types` Type: basetypes.StringValue
│ Suggested Pointer Type: *string

Terraform logs (internal only):
https://private-fileshare.canonical.com/~mcarvalhor/2024-03-15_JujuGitHubIssue/prod-is-cos-terraform.log

Notes & References

There's no workaround currently: I defined "" as the value for the setting on Terraform code, and then unset that manually after it's applied.

hmlanigan commented 7 months ago

The provider only keeps track of config which has been changed from the default. Would a better terraform behavior be to remove the key from the config map? If it's been removed, we should reset the value to the default, a la juju config --reset

Note: I'm not sure if that's what we actually do right now.

mcarvalhor commented 7 months ago

Hi @hmlanigan! 👋

I agree that not including the key on the config map works as expected; but using null currently throws the following error:

An unexpected error was encountered trying to build a value. This is always an error in the provider. [...] Received null value, however the target type cannot handle null values.

Probably worth further discussion, but I believe null should essentially behave the same way as not including the key at all - meaning null values should be completely ignored, as if they weren't there.

This is the default behavior used by a lot of resources on distinct providers. If not agreed, then at least the error message should be improved (eg.: "null is not supported here").

As an example, I've taken an arbitrarily selected provider and resource to do this same comparison: https://pastebin.ubuntu.com/p/zGfXR9tpHX/

As we can see (^), including "availability_zone = null" and not including anything at all produces the same result in the plan - and applying it also works as expected.

In addition, noticeably this issue is not urgent.