Support custom builder image environment variables in edit flow #10593

rottencandy · 2021-12-01T15:51:55Z

Fixes:
https://issues.redhat.com/browse/ODC-6414

Description:
This is a follow up of #10331. Adds support for setting builder image environments defined by the ImportEnvironment extension to be loaded and shown in edit application form.

Unit test coverage report:

Test setup:

Create a node-js application from git import flow and use the run command field.
Open the edit form for the application.

Browser conformance:

Chrome
Firefox
Safari
Edge

christoph-jerolimov · 2021-12-02T16:00:33Z

/retest
/assign

christoph-jerolimov · 2021-12-02T16:01:01Z

/cc

christoph-jerolimov

(remove duplicate)

christoph-jerolimov

@rottencandy Found an issue: When editing the npm start command and saving the Deployment, the BuildConfig environment variables were added and not replaced:

Can you also add some unit tests for BuilderImageEnvironments? :)

rottencandy · 2021-12-07T12:14:45Z

Fixed duplicating envs and added tests.

rottencandy · 2021-12-07T16:27:25Z

/retest-required

rottencandy · 2021-12-08T10:49:32Z

Propagating Docs & PX ack labels from epic ODC-6266

/label docs-approved
/label px-approved

christoph-jerolimov · 2021-12-08T10:56:12Z

/retest

christoph-jerolimov · 2021-12-08T11:32:52Z

frontend/packages/dev-console/src/components/import/import-submit-utils.ts

As discussed in slack (only as info for other reviews) that the merge here doesn't allow the user to unset the "Start command" at the moment. It is automatically restored from the collapsed environment variables.

Updated to hide envs from build config section, and to not pick emptied fields.

I personally would use the customEnvs first and then override it with user envs, but that's a big IMHO and I wouldn't add this comment if I didn't find the "not saving" issue above. :)

christoph-jerolimov

Small bug found when saving a Deployment without npm start command.

christoph-jerolimov · 2021-12-09T17:50:27Z

frontend/packages/dev-console/src/components/import/import-submit-utils.ts

When editing a Deployment with a BuildConfig environment variable (A=a) and no npm start command (no NPM_RUN) it was not possible to to save the edit form. It shows "Cannot convert undefined or null to object"

This happens when building the buildEnvs because imageEnv is undefined.

I think this would help but any solution is fine for me! :)

Suggested change

const customBuildEnvs = imageEnv

const customEnvs = imageEnv

? Object.keys(imageEnv)

.filter((k) => !!imageEnv[k])

.map((k) => ({ name: k, value: imageEnv[k] }))

: [];

const buildEnvs = env.filter(

(buildEnv) => !Object.keys(imageEnv).some((envKey) => buildEnv.name === envKey),

);

const imageEnvKeys = imageEnv ? Object.keys(imageEnv) : [];

const customEnvs = imageEnvKeys

.filter((k) => !!imageEnv[k])

.map((k) => ({ name: k, value: imageEnv[k] }));

const buildEnvs = env.filter(

(buildEnv) => !imageEnvKeys.some((envKey) => buildEnv.name === envKey),

);

Or could we simplify this by extracting buildEnvs with an imageEnvKeys?.includes(buildEnv.name) instead of .some? Not 100% sure.

Thanks, I forgot it could be undefined. Updated.

christoph-jerolimov · 2021-12-09T17:51:39Z

...nd/packages/dev-console/src/components/import/advanced/__tests__/BuildConfigSection.spec.tsx

Not necessary change or?

Was testing this but did not revert : p

christoph-jerolimov · 2021-12-09T17:52:33Z

frontend/packages/dev-console/src/components/import/import-submit-utils.ts

I personally would use the customEnvs first and then override it with user envs, but that's a big IMHO and I wouldn't add this comment if I didn't find the "not saving" issue above. :)

christoph-jerolimov · 2021-12-09T17:55:06Z

frontend/packages/dev-console/integration-tests/support/step-definitions/common/addFlow.ts

Nit. Can we also check that it only exists once? Wdyt?

(Again, only add this comment because you need to touch the PR anyway because saving doesn't work when start command is not defined.)

Suggested change

cy.get(`input[data-test="pairs-list-name"][value="${envKey}"]`).should('exist');

cy.get(`input[data-test="pairs-list-value"][value="${envVal}"]`).should('exist');

cy.get(`input[data-test="pairs-list-name"][value="${envKey}"]`).should('have.length', 1)

cy.get(`input[data-test="pairs-list-value"][value="${envVal}"]`).should('have.length', 1)

christoph-jerolimov · 2021-12-10T08:49:50Z

/retest

christoph-jerolimov · 2021-12-10T11:41:20Z

/retest

christoph-jerolimov · 2021-12-10T11:49:54Z

Tested this locally and on a cluster created with cluster bot. Tested many cases with and without, changing and deleting the npm "Start command". Works fine. 👍

/approve
/lgtm
/label qe-approved

openshift-ci · 2021-12-10T11:50:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, rottencandy

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [jerolimov]

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

openshift-bot · 2021-12-10T14:01:54Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-10T14:13:54Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-12-10T16:44:03Z

@rottencandy: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci bot requested review from andrewballantyne and invincibleJai December 1, 2021 15:52

openshift-ci bot added the component/dev-console Related to dev-console label Dec 1, 2021

openshift-ci bot assigned christoph-jerolimov Dec 2, 2021

openshift-ci bot requested a review from christoph-jerolimov December 2, 2021 16:01

christoph-jerolimov suggested changes Dec 2, 2021

View reviewed changes

rottencandy force-pushed the feat/import-env-edit branch from 6ba9ca1 to 5cf1c6a Compare December 6, 2021 18:48

openshift-ci bot added component/core Related to console core functionality component/topology Related to topology labels Dec 6, 2021

rottencandy force-pushed the feat/import-env-edit branch from 5cf1c6a to a5ae128 Compare December 7, 2021 12:13

rottencandy requested a review from christoph-jerolimov December 8, 2021 10:49

openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Dec 8, 2021

christoph-jerolimov suggested changes Dec 8, 2021

View reviewed changes

rottencandy force-pushed the feat/import-env-edit branch from a5ae128 to 0239a4f Compare December 9, 2021 09:18

openshift-ci bot added the component/sdk Related to console-plugin-sdk label Dec 9, 2021

rottencandy force-pushed the feat/import-env-edit branch 2 times, most recently from f9460a5 to bd7feea Compare December 9, 2021 14:13

christoph-jerolimov suggested changes Dec 9, 2021

View reviewed changes

support custom BuilderImage env vars in edit flow

a044195

rottencandy force-pushed the feat/import-env-edit branch from bd7feea to a044195 Compare December 9, 2021 19:00

openshift-ci bot added qe-approved Signifies that QE has signed off on this PR lgtm Indicates that a PR is ready to be merged. labels Dec 10, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2021

openshift-merge-robot merged commit b850b61 into openshift:master Dec 10, 2021

rottencandy deleted the feat/import-env-edit branch April 21, 2022 14:16

-  const customBuildEnvs = imageEnv
-  const customEnvs = imageEnv
-    ? Object.keys(imageEnv)
-        .filter((k) => !!imageEnv[k])
-        .map((k) => ({ name: k, value: imageEnv[k] }))
-    : [];
-  const buildEnvs = env.filter(
-    (buildEnv) => !Object.keys(imageEnv).some((envKey) => buildEnv.name === envKey),
-  );
+  const imageEnvKeys = imageEnv ? Object.keys(imageEnv) : [];
+  const customEnvs = imageEnvKeys
+    .filter((k) => !!imageEnv[k])
+    .map((k) => ({ name: k, value: imageEnv[k] }));
+  const buildEnvs = env.filter(
+    (buildEnv) => !imageEnvKeys.some((envKey) => buildEnv.name === envKey),
+  );

-    cy.get(`input[data-test="pairs-list-name"][value="${envKey}"]`).should('exist');
-    cy.get(`input[data-test="pairs-list-value"][value="${envVal}"]`).should('exist');
+    cy.get(`input[data-test="pairs-list-name"][value="${envKey}"]`).should('have.length', 1)
+    cy.get(`input[data-test="pairs-list-value"][value="${envVal}"]`).should('have.length', 1)

Support custom builder image environment variables in edit flow #10593

Support custom builder image environment variables in edit flow #10593

Uh oh!

Conversation

rottencandy commented Dec 1, 2021

Uh oh!

christoph-jerolimov commented Dec 2, 2021

Uh oh!

christoph-jerolimov commented Dec 2, 2021

Uh oh!

christoph-jerolimov 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!

christoph-jerolimov left a comment

Choose a reason for hiding this comment

Uh oh!

rottencandy commented Dec 7, 2021

Uh oh!

rottencandy commented Dec 7, 2021

Uh oh!

rottencandy commented Dec 8, 2021

Uh oh!

christoph-jerolimov commented Dec 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christoph-jerolimov Dec 9, 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!

christoph-jerolimov left a comment

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!

christoph-jerolimov Dec 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christoph-jerolimov commented Dec 10, 2021

Uh oh!

christoph-jerolimov commented Dec 10, 2021

Uh oh!

christoph-jerolimov commented Dec 10, 2021

Uh oh!

openshift-ci bot commented Dec 10, 2021

Uh oh!

openshift-bot commented Dec 10, 2021

Uh oh!

openshift-bot commented Dec 10, 2021

Uh oh!

openshift-ci bot commented Dec 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

christoph-jerolimov left a comment •

edited

Loading

christoph-jerolimov Dec 9, 2021 •

edited

Loading

christoph-jerolimov Dec 9, 2021 •

edited

Loading