`NewValue` function should also return an `Error` type

[BBBmau]

terraform-plugin-go version

v0.14.2

Relevant provider source code

func NewValue(t Type, val interface{}) Value {
	v, err := newValue(t, val)
	if err != nil {
		panic(err)
	}
	return v
}

Terraform Configuration Files

terraform {
  required_providers {
    kubernetes = {
      source = "hashicorp/kubernetes"
    }
  }
}

provider "kubernetes" {
  # Configuration options
    config_path = "~/.kube/config"
}

locals {
  condition = false
  postStart = {
    exec = {
      command = ["bash", "-c", "echo postStart"]
    }
  }
  preStop = {
    exec = {
      command = ["bash", "-c", "echo preStop"]
    }
  }
}
resource "kubernetes_manifest" "deploy_foo" {
  manifest = {
    apiVersion = "apps/v1"
    kind       = "Deployment"
    metadata = {
      name      = "foo"
      namespace = "foo"
      labels = {
        "app" = "foo"
      }
    }
    spec = {
      selector = {
        matchLabels = {
          "app" = "foo"
        }
      }
      template = {
        metadata = {
          labels = {
            "app" = "foo"
          },
        }
        spec = {
          containers = [
            {
              name  = "foo"
              image = "busybox"
              lifecycle = true ? {
                postStart = local.postStart
              } :{
                preStop = local.preStop
              }
            }
          ]
        }
      }
    }
  }
}

Expected Behavior

Should return an error in order to prevent having to catch the error with the k8s provider

Actual Behavior

no error is returned forcing us to have to catch the error right before the panic within the provider.

References

hashicorp/terraform-provider-kubernetes#2018

[bflad]

Hi @BBBmau 👋 Thank you for raising this and sorry it caused you trouble.

The original and current design of the tftypes package is that value creation with panic safety be done in two steps, checking that the value would be valid via tftypes.ValidateValue() and then if no error is raised, calling tftypes.NewValue(). While this is not necessarily ergonomic or performant, this is the documented behavior. Since the code is working as documented and intended, I'm going to relabel this issue as an enhancement instead of a bug. Since doing anything here would likely require breaking changes to existing function signatures, I'm also going to add the breaking change label.

There are likely a few considerations that should occur if we were to take a look at reworking the tftypes package with breaking changes:

• Most drastically, re-evaluate whether tftypes should exist at all, or whether it would be preferred to use cty on this side of the protocol to match the core side. There are a lot of difficult consequences to something like this, but it could be seen as beneficial to always have all core type system functionality available as-is, letting downstream implementations such as terraform-plugin-framework be more preferable locations for type system abstractions.
• Removing Type comparison restrictions, where currently developers are forced to compare types using the Is() method
• Removing the primitive types out of mutable package variables, instead promoting them as their own exported types
• Moving the MessagePack/JSON methods into their own (potentially internal) package, so they are not awkwardly presented as deprecated to "discourage" external usage
• Whether terraform-plugin-go should implement shared-across-protocol-version diagnostic types and whether tftypes functionality should use/support those
• Value creation (this issue)

With respect to this issue, assuming tftypes does stay around in some form and without shared diagnostics types, I would propose value creation/validation be something like the following:

• NewValue(Type, any) (Value, error) similar to what is being proposed here, so safe value creation is always a single call
• NewValueMust(Type, any) Value similar to how NewValue() works today, so known-safe value creation is still easy
• Removing the ValidateValue() function

To offer a migration period rather than breaking the existing ecosystem outright, the NewValueMust() implementation could be created and NewValue() deprecated. A later version would change the NewValue() signature and remove ValidateValue().

Another option would be introducing something like NewValueWithError(Type, any) (Value, error). Since this Go module has different compatibility concerns than most others though, it seems best to not leave multiple developer implementations around.

With all that said, I'm going to leave this issue as-is without any commitment to gauge further developer interest. Additional problem reports are definitely appreciated. If there are no other upvotes or commentary though, this sort of change will likely linger longer or be closed because it would represent a large amount of ecosystem burden to implement.