fix(internal/provider): correct escaping of strings in envbuilder_cached_image.env (#32)

Fixes #31

We had previously been doing the equivalent of value.String() when writing envbuilder_cached_image.env. This was incorrectly escaping newlines, potentially breaking ENVBUILDER_INIT_SCRIPT.

This PR modifies the behaviour to correctly handle string values via ValueString() instead.

examples/resources/envbuilder_cached_image/envbuilder_cached_image_resource.tf

@@ -66,10 +66,11 @@ resource "envbuilder_cached_image" "example" {
   builder_image = var.builder_image
   git_url       = var.repo_url
   cache_repo    = local.cache_repo
+  insecure      = true
   extra_env = {
     "ENVBUILDER_VERBOSE" : "true"
     "ENVBUILDER_INSECURE" : "true" # due to local registry
-    "ENVBUILDER_INIT_SCRIPT" : "sleep infinity"
+    "ENVBUILDER_INIT_SCRIPT" : "#!/usr/bin/env bash\necho Hello && sleep infinity"
     "ENVBUILDER_PUSH_IMAGE" : "true"
   }
   depends_on = [docker_container.registry]
@@ -77,8 +78,8 @@ resource "envbuilder_cached_image" "example" {
 
 // Run the cached image. Depending on the contents of
 // the cache repo, this will either be var.builder_image
-// or a previously built image pusehd to var.cache_repo.
-// Running `terraform apply` once (assuming empty cache) 
+// or a previously built image pushed to var.cache_repo.
+// Running `terraform apply` once (assuming empty cache)
 // will result in the builder image running, and the built
 // image being pushed to the cache repo.
 // Running `terraform apply` again will result in the
@@ -105,4 +106,3 @@ output "id" {
 output "image" {
   value = envbuilder_cached_image.example.image
 }
-

internal/provider/cached_image_resource.go

@@ -22,6 +22,7 @@ import (
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/google/uuid"
 
+	"github.com/hashicorp/terraform-plugin-framework/attr"
 	"github.com/hashicorp/terraform-plugin-framework/resource"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier"
@@ -631,20 +632,35 @@ func extractEnvbuilderFromImage(ctx context.Context, imgRef, destPath string) er
 	return fmt.Errorf("extract envbuilder binary from image %q: %w", imgRef, os.ErrNotExist)
 }
 
-// NOTE: the String() method of Terraform values will evalue to `<null>` if unknown.
-// Check IsUnknown() first before calling String().
-type stringable interface {
-	IsUnknown() bool
-	IsNull() bool
-	String() string
+// tfValueToString converts an attr.Value to its string representation
+// based on its Terraform type. This is needed because the String()
+// method on an attr.Value creates a 'human-readable' version of the type, which
+// leads to quotes, escaped characters, and other assorted sadness.
+func tfValueToString(val attr.Value) string {
+	if val.IsUnknown() || val.IsNull() {
+		return ""
+	}
+	if vs, ok := val.(interface{ ValueString() string }); ok {
+		return vs.ValueString()
+	}
+	if vb, ok := val.(interface{ ValueBool() bool }); ok {
+		return fmt.Sprintf("%t", vb.ValueBool())
+	}
+	if vi, ok := val.(interface{ ValueInt64() int64 }); ok {
+		return fmt.Sprintf("%d", vi.ValueInt64())
+	}
+	panic(fmt.Errorf("tfValueToString: value %T is not a supported type", val))
 }
 
-func appendKnownEnvToList(list types.List, key string, value stringable) types.List {
+func appendKnownEnvToList(list types.List, key string, value attr.Value) types.List {
 	if value.IsUnknown() || value.IsNull() {
 		return list
 	}
-	val := strings.Trim(value.String(), `"`)
-	elem := types.StringValue(fmt.Sprintf("%s=%s", key, val))
+	var sb strings.Builder
+	_, _ = sb.WriteString(key)
+	_, _ = sb.WriteRune('=')
+	_, _ = sb.WriteString(tfValueToString(value))
+	elem := types.StringValue(sb.String())
 	list, _ = types.ListValue(types.StringType, append(list.Elements(), elem))
 	return list
 }
@@ -652,13 +668,7 @@ func appendKnownEnvToList(list types.List, key string, value stringable) types.L
 func tfListToStringSlice(l types.List) []string {
 	var ss []string
 	for _, el := range l.Elements() {
-		if sv, ok := el.(stringable); !ok {
-			panic(fmt.Sprintf("developer error: element %+v must be stringable", el))
-		} else if sv.IsUnknown() {
-			ss = append(ss, "")
-		} else {
-			ss = append(ss, sv.String())
-		}
+		ss = append(ss, tfValueToString(el))
 	}
 	return ss
 }

internal/provider/cached_image_resource_test.go

@@ -20,7 +20,8 @@ func TestAccCachedImageResource(t *testing.T) {
 	}
 
 	deps := setup(ctx, t, files)
-	deps.ExtraEnv["FOO"] = "bar"
+	deps.ExtraEnv["FOO"] = `bar
+baz` // THIS IS A LOAD-BEARING NEWLINE. DO NOT REMOVE.
 	resource.Test(t, resource.TestCase{
 		ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
 		Steps: []resource.TestStep{
@@ -42,7 +43,7 @@ func TestAccCachedImageResource(t *testing.T) {
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "image", deps.BuilderImage),
 					// Inputs should still be present.
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "cache_repo", deps.CacheRepo),
-					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "extra_env.FOO", "bar"),
+					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "extra_env.FOO", "bar\nbaz"),
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "git_url", deps.Repo.URL),
 					// Should be empty
 					resource.TestCheckNoResourceAttr("envbuilder_cached_image.test", "git_username"),
@@ -63,7 +64,7 @@ func TestAccCachedImageResource(t *testing.T) {
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "image", deps.BuilderImage),
 					// Inputs should still be present.
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "cache_repo", deps.CacheRepo),
-					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "extra_env.FOO", "bar"),
+					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "extra_env.FOO", "bar\nbaz"),
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "git_url", deps.Repo.URL),
 					// Should be empty
 					resource.TestCheckNoResourceAttr("envbuilder_cached_image.test", "git_username"),
@@ -81,7 +82,7 @@ func TestAccCachedImageResource(t *testing.T) {
 				Check: resource.ComposeAggregateTestCheckFunc(
 					// Inputs should still be present.
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "cache_repo", deps.CacheRepo),
-					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "extra_env.FOO", "bar"),
+					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "extra_env.FOO", "bar\nbaz"),
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "git_url", deps.Repo.URL),
 					// Should be empty
 					resource.TestCheckNoResourceAttr("envbuilder_cached_image.test", "git_username"),
@@ -92,7 +93,7 @@ func TestAccCachedImageResource(t *testing.T) {
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "exists", "true"),
 					resource.TestCheckResourceAttrSet("envbuilder_cached_image.test", "image"),
 					resource.TestCheckResourceAttrWith("envbuilder_cached_image.test", "image", quotedPrefix(deps.CacheRepo)),
-					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.0", "FOO=bar"),
+					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.0", "FOO=bar\nbaz"),
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.1", fmt.Sprintf("ENVBUILDER_CACHE_REPO=%s", deps.CacheRepo)),
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.2", fmt.Sprintf("ENVBUILDER_GIT_URL=%s", deps.Repo.URL)),
 					resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.3", "ENVBUILDER_REMOTE_REPO_BUILD_MODE=true"),