fix(internal/provider): correctly override from extra_env #44

johnstcn · 2024-09-03T16:47:56Z

Relates to #43

Our previous logic did not pass options from extra_env to envbuilder.RunCacheProbe.
This fixes the logic and adds more comprehensive tests around the overriding.

Before, we were setting environment variables directly from the data source model but not applying them properly to envbuilder.Options
The new overall flow is:

Data source model is converted to an envbuilder options struct
extra_env is applied to envbuilder options struct
Computed env is generated from envbuilder options struct

johnstcn · 2024-09-03T20:50:59Z

internal/provider/cached_image_resource_test.go

+			extraEnv: map[string]string{
+				"FOO":                               testEnvValue,
+				"ENVBUILDER_GIT_URL":                "https://not.the.real.git/url",
+				"ENVBUILDER_CACHE_REPO":             "not-the-real-cache-repo",
+				"ENVBUILDER_DEVCONTAINER_DIR":       "path/to/.devcontainer",
+				"ENVBUILDER_DEVCONTAINER_JSON_PATH": "path/to/.devcontainer/devcontainer.json",
+				"ENVBUILDER_DOCKERFILE_PATH":        "path/to/.devcontainer/Dockerfile",
+			},


This test case should assert the correct behaviour of the provider.

johnstcn · 2024-09-03T20:53:28Z

internal/provider/cached_image_resource.go

+		if sa, ok := opt.Value.(*serpent.StringArray); ok {
+			val = strings.Join(sa.GetSlice(), ",")


I'm not sure if this is due to the incorrect String() method being called or what; when I stepped through in a debugger I could see the method of a serpent.StringArray being called. Although I do note that the method in question has a value receiver instead of a pointer receiver.

johnstcn · 2024-09-03T20:54:26Z

internal/provider/cached_image_resource.go

+		case "", "false", "0":
+			// Skip zero values.
+			continue
+		}


AFAIK all of our boolean-ish options are false by default.

This is a bit flimsy IMHO; could we be more explicit about the types we want to skip, or use reflect to determine if a zero value is set?

Reflect would definitely be an option here; I chose to just hard-code these for simplicity.

johnstcn · 2024-09-03T20:55:04Z

internal/provider/cached_image_resource.go

+			// XXX: workaround for serpent behaviour where calling Set() on a
+			// string slice will append instead of replace: set to empty first.
+			if key == "ENVBUILDER_IGNORE_PATHS" {
+				_ = optsMap[key].Set("")
+			}


I'm explicitly choosing to overwrite instead of append here as I believe the latter will be suprising and unexpected.

Is there a chance this could get set to empty but then not replaced?

If somehow ENVBUILDER_IGNORE_PATHS ended up in extraEnv but not in optsMap, which would likely result in a nil pointer error on line 457. I could only see that happening if we removed that option.

This whole options situation is horrible and I agree we need a better way to do it. For now, I'm just focusing on getting it working.

mtojek

The overall change LGTM. My main concern is keeping different ENV vars hardcoded instead of a dedicated property/schema file. If these are magic strings, maybe we should create a dedicated file to keep track of them?

Nothing blocking. Thanks for working on this!

internal/provider/cached_image_resource.go

mtojek · 2024-09-04T07:38:00Z

internal/provider/cached_image_resource.go

+	return diags
+}
+
+func computeEnvFromOptions(opts eboptions.Options, extraEnv map[string]string) map[string]string {


nit: Maybe we should move computeEnvFromOptions to a helper function or lib as cached_image_resource.go is getting larger.

Will address in a follow-up.

mtojek · 2024-09-04T07:39:22Z

internal/provider/cached_image_resource.go

+		overrides["ENVBUILDER_BASE_IMAGE_CACHE_DIR"] = struct{}{}
+		opts.BaseImageCacheDir = data.BaseImageCacheDir.ValueString()


nit: I understand there is no simple way to replace this wall of condition-override-value 's with any dynamic logic?

One option I've been considering is using struct tags to match up the fields between these two. A 'dirty hack' could be using json.Marshal / json.Unmarshal to convert between the two. Some of the subtleties of the Terraform data model might be lost though (e.g. "unknown" versus "zero value").

If there is a chance of losing the information, then let's leave it as is, although I admit it is tempting to make it more dynamic.

mtojek · 2024-09-04T07:41:55Z

internal/provider/provider_internal_test.go

+		},
+		{
+			name: "all options without extra_env",
+			data: CachedImageResourceModel{


nit: would it help if we have some YAML files with test input data? I mean, less Go code...

I like the idea of using x/tools/txtar to store this.
Danny used it for the provisionerd timings tests (example).

mtojek · 2024-09-04T07:49:30Z

internal/provider/cached_image_resource.go

+		switch key {
+
+		// These options may not be overridden.
+		case "ENVBUILDER_CACHE_REPO", "ENVBUILDER_GIT_URL":


nit: would love to see these keys defined in some schema instead of hardcoded

dannykopping

LGTM

dannykopping · 2024-09-04T09:13:16Z

internal/provider/cached_image_resource.go

+			// XXX: workaround for serpent behaviour where calling Set() on a
+			// string slice will append instead of replace: set to empty first.
+			if key == "ENVBUILDER_IGNORE_PATHS" {
+				_ = optsMap[key].Set("")
+			}


Is there a chance this could get set to empty but then not replaced?

dannykopping · 2024-09-04T09:17:46Z

internal/provider/cached_image_resource.go

+		case "", "false", "0":
+			// Skip zero values.
+			continue
+		}


This is a bit flimsy IMHO; could we be more explicit about the types we want to skip, or use reflect to determine if a zero value is set?

dannykopping · 2024-09-04T09:18:36Z

internal/provider/cached_image_resource.go

+// tfMapToStringMap converts a types.Map to a map[string]string by calling
+// tfValueToString on each element.
+func tfMapToStringMap(m types.Map) map[string]string {
+	res := make(map[string]string)


Nit: could we preallocate here based on the length of m?

Addresses some non-blocking comments from #44: - Extracts some of the functions in cached_image_resource.go to separate internal packages tfutil and imgutil. - Some other functions are extracted to helpers.go. - Extracts non-overridable flags to a package-level variable. - Pre-allocates some slices where possible. - Removes some unused code and renames some existing code for readability

johnstcn added 2 commits September 3, 2024 17:35

fix(internal/provider): correctly override from extra_env

156ca0e

ensure that arguments passed in via extra_env are acknowledged

3356f9f

johnstcn self-assigned this Sep 3, 2024

johnstcn requested review from mafredri, dannykopping and mtojek September 3, 2024 16:51

johnstcn added 2 commits September 3, 2024 21:26

improve tests for extra_env

71fb950

add test helpers for asserting env

39a8f39

johnstcn commented Sep 3, 2024

View reviewed changes

mtojek approved these changes Sep 4, 2024

View reviewed changes

johnstcn mentioned this pull request Sep 4, 2024

refactor: use golden files to store test data for integraiton tests #45

Open

dannykopping approved these changes Sep 4, 2024

View reviewed changes

johnstcn merged commit 23f2cf5 into main Sep 4, 2024
14 checks passed

johnstcn deleted the cj/extra-env-opts branch September 4, 2024 10:19

johnstcn mentioned this pull request Sep 4, 2024

chore(internal/provider): refactor cached_image_resource #46

Merged

		if sa, ok := opt.Value.(*serpent.StringArray); ok {
		val = strings.Join(sa.GetSlice(), ",")

		overrides["ENVBUILDER_BASE_IMAGE_CACHE_DIR"] = struct{}{}
		opts.BaseImageCacheDir = data.BaseImageCacheDir.ValueString()

fix(internal/provider): correctly override from extra_env #44

fix(internal/provider): correctly override from extra_env #44

Uh oh!

Conversation

johnstcn commented Sep 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannykopping Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstcn Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannykopping left a comment

Choose a reason for hiding this comment

Uh oh!

dannykopping Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dannykopping Sep 4, 2024 •

edited

Loading

mtojek left a comment •

edited

Loading

johnstcn Sep 4, 2024 •

edited

Loading

dannykopping Sep 4, 2024 •

edited

Loading