Support global environment variables defined by a component-level attribute #538

amisevsk · 2021-07-30T19:55:39Z

This PR is a slightly cleaned up iteration of #529

What does this PR do?

Add support for the devfile 1.0 feature workspaceEnv (env vars supplied by a component that get added to all containers in the workspace). This is defined using an attribute on components named workspaceEnv formatted as []EnvVar, i.e.:

attributes:
  workspaceEnv: 
    - name: MY_ENV_1
      value: value
    - name: MY_ENV_2
      value: value

Note: it's necessary to define the attribute on components for now, because merging plugin components with top-level attributes is unsuitable to some use cases:

If the base DevWorkspace defines a workspaceEnv attribute, any plugin that also defines a top-level workspaceEnv attribute will fail to merge during flattening
If two plugins define workspaceEnv attributes at the top level, the second plugin will overwrite the value from the first, i.e. it's only possible to define workspaceEnv in one place in this way.

An alternative approach that would allow using top-level attributes would be some sort of namespacing mechanism, e.g. using workspace-env/my-component as the attribute, but this would be harder to process and write.

What issues does this PR fix or reference?

Closes #530

Is it tested? How?

Test cases are included; can be tested directly using the DevWorkspace

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: theia-next
spec:
  started: true
  template:
    components:
      - name: tooling
        attributes:
          workspaceEnv: 
            - name: TEST_ENV_1
              value: TEST_VAL_1
            - name: TEST_ENV_2
              value: TEST_VAL_2
        container:
          image: quay.io/wto/web-terminal-tooling
          args: ["tail", "-f", "/dev/null"]
      - name: theia
        plugin:
          uri: https://che-plugin-registry-main.surge.sh/v3/plugins/eclipse/che-theia/next/devfile.yaml

and checking that all containers in the resulting pod have the environment variables listed.

PR Checklist

E2E tests pass (when PR is ready, comment /test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path to trigger)
- v7-devworkspaces-operator-e2e: DevWorkspace e2e test
- v7-devworkspace-happy-path: DevWorkspace e2e test

sleshchenko

LGTM

Just a few minor comments. Good job!

sleshchenko · 2021-08-02T14:29:20Z

pkg/library/flatten/helper.go

@@ -64,3 +65,14 @@ func formatImportCycle(end *resolutionContextTree) string {
 	}
 	return cycle
 }
+
+func getOriginalNameForComponent(component dw.Component) string {


Well, we don't modify components names, it returns exactly what is written in body: SourceComponent. Maybe then rename it to getSourceForComponent

We don't modify component names per se, but we delete plugin components and add e.g. container components:

components: - name: my-plugin plugin: [...]

becomes

components: - name: plugin-container container: [...]

But I see your point -- I'll update.

pkg/library/flatten/workspaceEnv.go

sleshchenko · 2021-08-02T14:39:55Z

pkg/library/flatten/workspaceEnv.go

+		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)


validate the format of attributes can be +1 things we can validate with validate webhooks.

sleshchenko · 2021-08-02T14:41:45Z

pkg/library/flatten/testdata/workspace-env/no-error-on-duplicate-workspace-env.yaml

@@ -0,0 +1,75 @@
+name: "Adds workspace environment variables from plugin"


please allign test files names, now some are with _, some only -, some with CamelCase for workspaceEnv, some has workspace-env.

Fixed for consistency -- note the format I've used thus far for _ vs - is that dashes replace spaces in the sentence describing the test, and error_ is the prefix that specifies that a test case is expected to return an error (it's mostly so that an alphabetical sort puts all error testcases together). If I've missed a place in that framework, I'll fix it.

sleshchenko · 2021-08-02T14:42:29Z

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 "


File name and case name does not seem to be really the same.

Copy-paste error :)

sleshchenko · 2021-08-02T14:45:25Z

pkg/library/flatten/testdata/workspace-env/adds-workspace-env-to-all-containers.yaml

@@ -0,0 +1,84 @@
+name: "Adds workspace environment variables from plugin"


This suite case appear the second time

pkg/library/flatten/testdata/workspace-env/error_workspaceEnv-formatted-wrong.yaml

JPinkney

Other then Serhii's comments it looks good to me

openshift-ci · 2021-08-04T08:30:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, JPinkney, sleshchenko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JPinkney,amisevsk,sleshchenko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sleshchenko · 2021-08-04T08:31:11Z

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

sleshchenko · 2021-08-04T10:31:23Z

@amisevsk feel free to merge with squash or rebase to get rid of fixups and rerun tests.

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. Note we use component attributes rather than top-level attributes for workspaceEnv, since top-level attributes pose some problems for flattening: - If the base DevWorkspace defines workspaceEnv in top-level attributes and a plugin does the same, flattening fails - If two plugins define top-level workspaceEnv attributes, the workspaceEnv of the first if overwritten by the second Signed-off-by: Angel Misevski <[email protected]>

Extract common setup for tests to a separate function Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

openshift-ci · 2021-08-04T14:29:17Z

New changes are detected. LGTM label has been removed.

amisevsk · 2021-08-04T14:29:28Z

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

amisevsk requested review from sleshchenko and JPinkney July 30, 2021 19:55

openshift-ci bot added the approved label Jul 30, 2021

sleshchenko mentioned this pull request Aug 2, 2021

DevWorkspace 0.8.0 Final Checklist #518

Closed

8 tasks

sleshchenko approved these changes Aug 2, 2021

View reviewed changes

openshift-ci bot assigned sleshchenko Aug 2, 2021

openshift-ci bot added the lgtm label Aug 2, 2021

JPinkney approved these changes Aug 3, 2021

View reviewed changes

openshift-ci bot assigned JPinkney Aug 3, 2021

openshift-ci bot removed the lgtm label Aug 4, 2021

sleshchenko approved these changes Aug 4, 2021

View reviewed changes

openshift-ci bot added the lgtm label Aug 4, 2021

amisevsk added 3 commits August 4, 2021 10:28

Reduce repetition in DW flattening tests

0616fb6

Extract common setup for tests to a separate function Signed-off-by: Angel Misevski <[email protected]>

Add test cases for processing workspaceEnv

a3b6122

Signed-off-by: Angel Misevski <[email protected]>

amisevsk force-pushed the support-workspace-env branch from 6a5d12f to a3b6122 Compare August 4, 2021 14:29

openshift-ci bot removed the lgtm label Aug 4, 2021

amisevsk merged commit 0f248ed into devfile:main Aug 6, 2021

amisevsk deleted the support-workspace-env branch August 6, 2021 18:26

		@@ -0,0 +1,75 @@
		name: "Adds workspace environment variables from plugin"

		@@ -0,0 +1,84 @@
		name: "Adds JSON workspace environment variables from plugin "

Support global environment variables defined by a component-level attribute #538

Support global environment variables defined by a component-level attribute #538

Uh oh!

Conversation

amisevsk commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

Uh oh!

sleshchenko left a comment

Choose a reason for hiding this comment

Uh oh!

sleshchenko Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

amisevsk Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sleshchenko Aug 2, 2021 • 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!

sleshchenko Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

amisevsk Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

sleshchenko Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

amisevsk Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

sleshchenko Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JPinkney left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Aug 4, 2021

Uh oh!

sleshchenko commented Aug 4, 2021

Uh oh!

sleshchenko commented Aug 4, 2021

Uh oh!

openshift-ci bot commented Aug 4, 2021

Uh oh!

amisevsk commented Aug 4, 2021

Uh oh!

Uh oh!

amisevsk commented Jul 30, 2021 •

edited

Loading

sleshchenko Aug 2, 2021 •

edited

Loading