From 2af8a94aac8e38a6f179926110bfe8d618874d1f Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Thu, 29 Jul 2021 14:37:58 -0400 Subject: [PATCH 1/4] Add support for workspaceEnv via an attribute on components Add support for workspaceEnv (i.e. environment variables provided by a component that get added to all other components) though setting an attribute 'workspaceEnv' in a component. If that component is added to the workspace (either directly or through a plugin) the environment variables defined within are added to all containers and init containers in the workspace. Example: a component components: - name: my-component attributes: workspaceEnv: - name: MY_ENV value: my-value - name: MY_ENV_2 value: my-value-2 will add the MY_ENV and MY_ENV_2 environment variables to all containers in the DevWorkspace. Signed-off-by: Angel Misevski --- pkg/library/flatten/flatten.go | 5 ++ pkg/library/flatten/helper.go | 13 +++++ pkg/library/flatten/workspaceEnv.go | 81 +++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 pkg/library/flatten/workspaceEnv.go diff --git a/pkg/library/flatten/flatten.go b/pkg/library/flatten/flatten.go index 02d938f11..a4f26cc05 100644 --- a/pkg/library/flatten/flatten.go +++ b/pkg/library/flatten/flatten.go @@ -68,6 +68,11 @@ func ResolveDevWorkspace(workspace *dw.DevWorkspaceTemplateSpec, tooling Resolve return resolvedDW, &warnings, nil } + err = resolveWorkspaceEnvVar(resolvedDW) + if err != nil { + return nil, nil, fmt.Errorf("failed to process workspaceEnv: %w", err) + } + return resolvedDW, nil, nil } diff --git a/pkg/library/flatten/helper.go b/pkg/library/flatten/helper.go index 75f3d05e8..206a4f106 100644 --- a/pkg/library/flatten/helper.go +++ b/pkg/library/flatten/helper.go @@ -17,6 +17,7 @@ import ( "reflect" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/devworkspace-operator/pkg/constants" ) // resolutionContextTree is a recursive structure representing information about the devworkspace that is @@ -64,3 +65,15 @@ func formatImportCycle(end *resolutionContextTree) string { } return cycle } + +func getOriginalNameForComponent(component dw.Component) string { + if component.Attributes.Exists(constants.PluginSourceAttribute) { + var err error + componentName := component.Attributes.GetString(constants.PluginSourceAttribute, &err) + if err != nil { + return component.Name + } + return componentName + } + return component.Name +} diff --git a/pkg/library/flatten/workspaceEnv.go b/pkg/library/flatten/workspaceEnv.go new file mode 100644 index 000000000..ca25a2bc7 --- /dev/null +++ b/pkg/library/flatten/workspaceEnv.go @@ -0,0 +1,81 @@ +// +// Copyright (c) 2019-2021 Red Hat, Inc. +// This program and the accompanying materials are made +// available under the terms of the Eclipse Public License 2.0 +// which is available at https://www.eclipse.org/legal/epl-2.0/ +// +// SPDX-License-Identifier: EPL-2.0 +// +// Contributors: +// Red Hat, Inc. - initial API and implementation +// + +package flatten + +import ( + "fmt" + + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" +) + +const ( + // WorkspaceEnvAttribute is an attribute that specifies a set of environment variables provided by a component + // that should be added to all workspace containers. The structure of the attribute value should be a map of strings + // to strings, e.g. + // + // attributes: + // workspaceEnv: + // - name: ENV_1 + // value: VAL_1 + // - name: ENV_2 + // value: VAL_2 + WorkspaceEnvAttribute = "workspaceEnv" +) + +func resolveWorkspaceEnvVar(flattenedDW *dw.DevWorkspaceTemplateSpec) error { + workspaceEnv, err := collectWorkspaceEnv(flattenedDW) + if err != nil { + return err + } + + for idx, component := range flattenedDW.Components { + if component.Container != nil { + flattenedDW.Components[idx].Container.Env = append(component.Container.Env, workspaceEnv...) + } + } + + return nil +} + +func collectWorkspaceEnv(flattenedDW *dw.DevWorkspaceTemplateSpec) ([]dw.EnvVar, error) { + var workspaceEnv []dw.EnvVar + + // Keep track of used workspace env vars to detect conflicts + usedEnvVar := map[string]string{} + // Bookkeeping map so that we can format error messages in case of conflict + envVarToComponent := map[string]string{} + + for _, component := range flattenedDW.Components { + if !component.Attributes.Exists(WorkspaceEnvAttribute) { + continue + } + + var componentEnv []dw.EnvVar + err := component.Attributes.GetInto(WorkspaceEnvAttribute, &componentEnv) + if err != nil { + return nil, fmt.Errorf("failed to read attribute %s on component %s: %w", WorkspaceEnvAttribute, getOriginalNameForComponent(component), err) + } + + for _, envVar := range componentEnv { + if existingVal, exists := usedEnvVar[envVar.Name]; exists && existingVal != envVar.Value { + return nil, fmt.Errorf("conflicting definition of environment variable %s in components '%s' and '%s'", + envVar.Name, envVarToComponent[envVar.Name], component.Name) + } + usedEnvVar[envVar.Name] = envVar.Value + envVarToComponent[envVar.Name] = getOriginalNameForComponent(component) + } + + workspaceEnv = append(workspaceEnv, componentEnv...) + } + return workspaceEnv, nil +} From f8f6c0e9417545b95ef0e90cf93052b00054ea54 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Thu, 29 Jul 2021 15:07:19 -0400 Subject: [PATCH 2/4] Reduce repetition in DW flattening tests Extract common setup for tests to a separate function Signed-off-by: Angel Misevski --- pkg/library/flatten/flatten_test.go | 134 +++++++++------------------- 1 file changed, 40 insertions(+), 94 deletions(-) diff --git a/pkg/library/flatten/flatten_test.go b/pkg/library/flatten/flatten_test.go index 938027ead..2d2127eec 100644 --- a/pkg/library/flatten/flatten_test.go +++ b/pkg/library/flatten/flatten_test.go @@ -28,15 +28,7 @@ func TestResolveDevWorkspaceKubernetesReference(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - WorkspaceNamespace: "test-ignored", - K8sClient: testClient, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -59,14 +51,8 @@ func TestResolveDevWorkspaceInternalRegistry(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testRegistry := &testutil.FakeInternalRegistry{ - Plugins: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - InternalRegistry: testRegistry, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -89,15 +75,8 @@ func TestResolveDevWorkspacePluginRegistry(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - HttpClient: testHttpGetter, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -120,15 +99,8 @@ func TestResolveDevWorkspacePluginURI(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - HttpClient: testHttpGetter, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -150,21 +122,8 @@ func TestResolveDevWorkspaceParents(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testK8sClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - WorkspaceNamespace: "test-ignored", - K8sClient: testK8sClient, - HttpClient: testHttpGetter, - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -189,20 +148,9 @@ func TestResolveDevWorkspaceMissingDefaults(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines workspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testK8sClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - K8sClient: testK8sClient, - HttpClient: testHttpGetter, - } + testResolverTools := getTestingTools(tt.Input, "") + testResolverTools.InternalRegistry = nil + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -224,21 +172,8 @@ func TestResolveDevWorkspaceAnnotations(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines devworkspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testK8sClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - K8sClient: testK8sClient, - HttpClient: testHttpGetter, - WorkspaceNamespace: "default-namespace", - } + testResolverTools := getTestingTools(tt.Input, "test-ignored") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -260,21 +195,8 @@ func TestResolveDevWorkspaceTemplateNamespaceRestriction(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check: input defines components assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines devworkspace with no components") - testHttpGetter := &testutil.FakeHTTPGetter{ - DevfileResources: tt.Input.DevfileResources, - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testK8sClient := &testutil.FakeK8sClient{ - DevWorkspaceResources: tt.Input.DevWorkspaceResources, - Errors: tt.Input.Errors, - } - testResolverTools := ResolverTools{ - Context: context.Background(), - K8sClient: testK8sClient, - HttpClient: testHttpGetter, - WorkspaceNamespace: "test-namespace", - } + testResolverTools := getTestingTools(tt.Input, "test-namespace") + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") @@ -289,3 +211,27 @@ func TestResolveDevWorkspaceTemplateNamespaceRestriction(t *testing.T) { }) } } + +func getTestingTools(input testutil.TestInput, testNamespace string) ResolverTools { + testHttpGetter := &testutil.FakeHTTPGetter{ + DevfileResources: input.DevfileResources, + DevWorkspaceResources: input.DevWorkspaceResources, + Errors: input.Errors, + } + testK8sClient := &testutil.FakeK8sClient{ + DevWorkspaceResources: input.DevWorkspaceResources, + Errors: input.Errors, + } + testRegistry := &testutil.FakeInternalRegistry{ + Plugins: input.DevWorkspaceResources, + + Errors: input.Errors, + } + return ResolverTools{ + Context: context.Background(), + InternalRegistry: testRegistry, + K8sClient: testK8sClient, + HttpClient: testHttpGetter, + WorkspaceNamespace: testNamespace, + } +} From eb8048a3a5fa49362736fd8b9ac01997a7edaea2 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Thu, 29 Jul 2021 15:36:17 -0400 Subject: [PATCH 3/4] Add test cases for processing workspaceEnv Signed-off-by: Angel Misevski --- pkg/library/flatten/flatten_test.go | 23 +++++ .../adds-workspace-env-to-all-containers.yaml | 84 +++++++++++++++++++ .../adds-workspace-env-written-as-json.yaml | 84 +++++++++++++++++++ ...plicated-env-var-with-different-value.yaml | 34 ++++++++ .../error_workspaceEnv-formatted-wrong.yaml | 29 +++++++ 5 files changed, 254 insertions(+) create mode 100644 pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml create mode 100644 pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml create mode 100644 pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml create mode 100644 pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml diff --git a/pkg/library/flatten/flatten_test.go b/pkg/library/flatten/flatten_test.go index 2d2127eec..293f59147 100644 --- a/pkg/library/flatten/flatten_test.go +++ b/pkg/library/flatten/flatten_test.go @@ -212,6 +212,29 @@ func TestResolveDevWorkspaceTemplateNamespaceRestriction(t *testing.T) { } } +func TestResolveDevWorkspaceWorkspaceEnv(t *testing.T) { + tests := testutil.LoadAllTestsOrPanic(t, "testdata/workspace-env") + for _, tt := range tests { + t.Run(tt.Name, func(t *testing.T) { + // sanity check: input defines components + assert.True(t, len(tt.Input.DevWorkspace.Components) > 0, "Test case defines devworkspace with no components") + testResolverTools := getTestingTools(tt.Input, "test-ignored") + + outputWorkspace, _, err := ResolveDevWorkspace(tt.Input.DevWorkspace, testResolverTools) + if tt.Output.ErrRegexp != nil && assert.Error(t, err) { + assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") + } else { + if !assert.NoError(t, err, "Should not return error") { + return + } + assert.Truef(t, cmp.Equal(tt.Output.DevWorkspace, outputWorkspace, testutil.WorkspaceTemplateDiffOpts), + "DevWorkspace should match expected output:\n%s", + cmp.Diff(tt.Output.DevWorkspace, outputWorkspace, testutil.WorkspaceTemplateDiffOpts)) + } + }) + } +} + func getTestingTools(input testutil.TestInput, testNamespace string) ResolverTools { testHttpGetter := &testutil.FakeHTTPGetter{ DevfileResources: input.DevfileResources, diff --git a/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml new file mode 100644 index 000000000..a97533553 --- /dev/null +++ b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml @@ -0,0 +1,84 @@ +name: "Adds workspace environment variables from plugin" + +input: + devworkspace: + components: + + - name: test-plugin + plugin: + uri: "https://my-plugin.io/test" + + - name: regular-container + container: + image: test-image-regular + + - name: init-container + container: + image: test-image-init + + commands: + - id: make-init + apply: + component: init-container + + events: + prestart: [make-init] + + devfileResources: + "https://my-plugin.io/test": + schemaVersion: 2.0.0 + metadata: + name: "plugin-a" + components: + - name: plugin-a + attributes: + workspaceEnv: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + container: + name: test-container + image: test-image + +output: + devworkspace: + components: + - name: plugin-a + attributes: + controller.devfile.io/imported-by: test-plugin + workspaceEnv: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + container: + name: test-container + image: test-image + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + - name: regular-container + container: + image: test-image-regular + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + - name: init-container + container: + image: test-image-init + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + commands: + - id: make-init + apply: + component: init-container + events: + prestart: [make-init] diff --git a/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml new file mode 100644 index 000000000..ad7419cf5 --- /dev/null +++ b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml @@ -0,0 +1,84 @@ +name: "Adds JSON workspace environment variables from plugin " + +input: + devworkspace: + components: + + - name: test-plugin + plugin: + uri: "https://my-plugin.io/test" + + - name: regular-container + container: + image: test-image-regular + + - name: init-container + container: + image: test-image-init + + commands: + - id: make-init + apply: + component: init-container + + events: + prestart: [make-init] + + devfileResources: + "https://my-plugin.io/test": + schemaVersion: 2.0.0 + metadata: + name: "plugin-a" + components: + - name: plugin-a + attributes: + workspaceEnv: + [{"name": "TEST_ENV_1", + "value": "TEST_VAL_1"}, + {"name": "TEST_ENV_2", + "value": "TEST_VAL_2"}] + container: + name: test-container + image: test-image + +output: + devworkspace: + components: + - name: plugin-a + attributes: + controller.devfile.io/imported-by: test-plugin + workspaceEnv: + [{"name": "TEST_ENV_1", + "value": "TEST_VAL_1"}, + {"name": "TEST_ENV_2", + "value": "TEST_VAL_2"}] + container: + name: test-container + image: test-image + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + - name: regular-container + container: + image: test-image-regular + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + - name: init-container + container: + image: test-image-init + env: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + commands: + - id: make-init + apply: + component: init-container + events: + prestart: [make-init] diff --git a/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml b/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml new file mode 100644 index 000000000..0dacf4a88 --- /dev/null +++ b/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml @@ -0,0 +1,34 @@ +name: "Returns an error when there are conflicting definitions of workspace env var" + +input: + devworkspace: + components: + - name: test-container + attributes: + workspaceEnv: + - name: TEST_ENV_2 + value: NOT_TEST_VAL_2 + container: + image: test-image + - name: test-plugin + plugin: + uri: "https://my-plugin.io/test" + devfileResources: + "https://my-plugin.io/test": + schemaVersion: 2.0.0 + metadata: + name: "plugin-a" + components: + - name: plugin-a + attributes: + workspaceEnv: + - name: TEST_ENV_1 + value: TEST_VAL_1 + - name: TEST_ENV_2 + value: TEST_VAL_2 + container: + name: test-container + image: test-image + +output: + errRegexp: "failed to process workspaceEnv: conflicting definition of environment variable TEST_ENV_2 in components 'test-plugin' and 'test-container'" diff --git a/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml b/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml new file mode 100644 index 000000000..e77858689 --- /dev/null +++ b/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml @@ -0,0 +1,29 @@ +name: "Returns an error when workspaceEnv cannot be read as []EnvVar" + +input: + devworkspace: + components: + - name: test-plugin + plugin: + uri: "https://my-plugin.io/test" + devfileResources: + "https://my-plugin.io/test": + schemaVersion: 2.0.0 + metadata: + name: "plugin-a" + components: + - name: plugin-a + attributes: + # Note this fails to deserialize because it's just a yaml string, not a list. + # JSON can be used here if you remove the pipe + workspaceEnv: | + [{"name": "TEST_ENV_1", + "value": "TEST_VAL_1"}, + {"name": "TEST_ENV_2", + "value": "TEST_VAL_2"}] + container: + name: test-container + image: test-image + +output: + errRegexp: "failed to read attribute workspaceEnv on component test-plugin" From fc3b77fe226f7e6e82923432b2833b71ba56f833 Mon Sep 17 00:00:00 2001 From: Angel Misevski Date: Thu, 29 Jul 2021 15:53:32 -0400 Subject: [PATCH 4/4] Switch format for workspaceEnv from []EnvVar to map[string]string Switch the definition of workspaceEnv from attributes: workspaceEnv: - name: TEST_ENV_1 value: TEST_VAL_1 - name: TEST_ENV_2 value: TEST_VAL_2 to attributes: workspaceEnv: TEST_ENV_1: TEST_VAL_1 TEST_ENV_2: TEST_VAL_2 Signed-off-by: Angel Misevski --- .../flatten/internal/testutil/common.go | 3 +++ .../adds-workspace-env-to-all-containers.yaml | 12 ++++------ .../adds-workspace-env-written-as-json.yaml | 16 ++++++------- ...plicated-env-var-with-different-value.yaml | 9 +++----- .../error_workspaceEnv-formatted-wrong.yaml | 8 +++---- pkg/library/flatten/workspaceEnv.go | 23 ++++++++++--------- 6 files changed, 34 insertions(+), 37 deletions(-) diff --git a/pkg/library/flatten/internal/testutil/common.go b/pkg/library/flatten/internal/testutil/common.go index af59c77b0..0568d6946 100644 --- a/pkg/library/flatten/internal/testutil/common.go +++ b/pkg/library/flatten/internal/testutil/common.go @@ -34,6 +34,9 @@ var WorkspaceTemplateDiffOpts = cmp.Options{ cmpopts.SortSlices(func(a, b string) bool { return strings.Compare(a, b) > 0 }), + cmpopts.SortSlices(func(a, b dw.EnvVar) bool { + return strings.Compare(a.Name, b.Name) > 0 + }), // TODO: Devworkspace overriding results in empty []string instead of nil cmpopts.IgnoreFields(dw.DevWorkspaceEvents{}, "PostStart", "PreStop", "PostStop"), } diff --git a/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml index a97533553..b1097a361 100644 --- a/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml +++ b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml @@ -33,10 +33,8 @@ input: - name: plugin-a attributes: workspaceEnv: - - name: TEST_ENV_1 - value: TEST_VAL_1 - - name: TEST_ENV_2 - value: TEST_VAL_2 + TEST_ENV_1: TEST_VAL_1 + TEST_ENV_2: TEST_VAL_2 container: name: test-container image: test-image @@ -48,10 +46,8 @@ output: attributes: controller.devfile.io/imported-by: test-plugin workspaceEnv: - - name: TEST_ENV_1 - value: TEST_VAL_1 - - name: TEST_ENV_2 - value: TEST_VAL_2 + TEST_ENV_1: TEST_VAL_1 + TEST_ENV_2: TEST_VAL_2 container: name: test-container image: test-image diff --git a/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml index ad7419cf5..85c277f06 100644 --- a/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml +++ b/pkg/library/flatten/testdata/workspace-env/adds-workspace-env-written-as-json.yaml @@ -33,10 +33,10 @@ input: - name: plugin-a attributes: workspaceEnv: - [{"name": "TEST_ENV_1", - "value": "TEST_VAL_1"}, - {"name": "TEST_ENV_2", - "value": "TEST_VAL_2"}] + { + "TEST_ENV_1": "TEST_VAL_1", + "TEST_ENV_2": "TEST_VAL_2", + } container: name: test-container image: test-image @@ -48,10 +48,10 @@ output: attributes: controller.devfile.io/imported-by: test-plugin workspaceEnv: - [{"name": "TEST_ENV_1", - "value": "TEST_VAL_1"}, - {"name": "TEST_ENV_2", - "value": "TEST_VAL_2"}] + { + "TEST_ENV_1": "TEST_VAL_1", + "TEST_ENV_2": "TEST_VAL_2", + } container: name: test-container image: test-image diff --git a/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml b/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml index 0dacf4a88..9dfd56aa5 100644 --- a/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml +++ b/pkg/library/flatten/testdata/workspace-env/error_duplicated-env-var-with-different-value.yaml @@ -6,8 +6,7 @@ input: - name: test-container attributes: workspaceEnv: - - name: TEST_ENV_2 - value: NOT_TEST_VAL_2 + TEST_ENV_2: NOT_TEST_VAL_2 container: image: test-image - name: test-plugin @@ -22,10 +21,8 @@ input: - name: plugin-a attributes: workspaceEnv: - - name: TEST_ENV_1 - value: TEST_VAL_1 - - name: TEST_ENV_2 - value: TEST_VAL_2 + TEST_ENV_1: TEST_VAL_1 + TEST_ENV_2: TEST_VAL_2 container: name: test-container image: test-image diff --git a/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml b/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml index e77858689..54fe3aaf9 100644 --- a/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml +++ b/pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml @@ -17,10 +17,10 @@ input: # Note this fails to deserialize because it's just a yaml string, not a list. # JSON can be used here if you remove the pipe workspaceEnv: | - [{"name": "TEST_ENV_1", - "value": "TEST_VAL_1"}, - {"name": "TEST_ENV_2", - "value": "TEST_VAL_2"}] + { + "TEST_ENV_1": "TEST_ENV_1", + "TEST_ENV_2": "TEST_ENV_2", + } container: name: test-container image: test-image diff --git a/pkg/library/flatten/workspaceEnv.go b/pkg/library/flatten/workspaceEnv.go index ca25a2bc7..4b09ecd44 100644 --- a/pkg/library/flatten/workspaceEnv.go +++ b/pkg/library/flatten/workspaceEnv.go @@ -48,10 +48,8 @@ func resolveWorkspaceEnvVar(flattenedDW *dw.DevWorkspaceTemplateSpec) error { } func collectWorkspaceEnv(flattenedDW *dw.DevWorkspaceTemplateSpec) ([]dw.EnvVar, error) { - var workspaceEnv []dw.EnvVar + workspaceEnvMap := map[string]string{} - // Keep track of used workspace env vars to detect conflicts - usedEnvVar := map[string]string{} // Bookkeeping map so that we can format error messages in case of conflict envVarToComponent := map[string]string{} @@ -60,22 +58,25 @@ func collectWorkspaceEnv(flattenedDW *dw.DevWorkspaceTemplateSpec) ([]dw.EnvVar, continue } - var componentEnv []dw.EnvVar - err := component.Attributes.GetInto(WorkspaceEnvAttribute, &componentEnv) + componentEnvMap := map[string]string{} + err := component.Attributes.GetInto(WorkspaceEnvAttribute, &componentEnvMap) if err != nil { return nil, fmt.Errorf("failed to read attribute %s on component %s: %w", WorkspaceEnvAttribute, getOriginalNameForComponent(component), err) } - for _, envVar := range componentEnv { - if existingVal, exists := usedEnvVar[envVar.Name]; exists && existingVal != envVar.Value { + for name, value := range componentEnvMap { + if existingVal, exists := workspaceEnvMap[name]; exists && existingVal != value { return nil, fmt.Errorf("conflicting definition of environment variable %s in components '%s' and '%s'", - envVar.Name, envVarToComponent[envVar.Name], component.Name) + name, envVarToComponent[name], component.Name) } - usedEnvVar[envVar.Name] = envVar.Value - envVarToComponent[envVar.Name] = getOriginalNameForComponent(component) + workspaceEnvMap[name] = value + envVarToComponent[name] = getOriginalNameForComponent(component) } + } - workspaceEnv = append(workspaceEnv, componentEnv...) + var workspaceEnv []dw.EnvVar + for name, value := range workspaceEnvMap { + workspaceEnv = append(workspaceEnv, dw.EnvVar{Name: name, Value: value}) } return workspaceEnv, nil }