Add extension to provide custom builder image environment variables #10331
Conversation
openshift-ci
bot
added
component/dev-console
|
/cc @jerolimov |
|
/retest |
|
Since the epic acceptance criteria have not changed and the epic ODC-6266 has a doc-ack and px-ack, I approve this on behalf of doc and PX team. /label docs-approved |
openshift-ci
bot
added
docs-approved
rottencandy
force-pushed
the
feat/import-env
branch
from
1882628
to
68cf261
Compare
frontend/packages/dev-console/src/components/import/import-submit-utils.ts
Outdated
Show resolved
Hide resolved
frontend/packages/console-dynamic-plugin-sdk/src/extensions/import-environments.ts
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/import/builder/BuilderImageEnvironments.tsx
Outdated
Show resolved
Hide resolved
rottencandy
force-pushed
the
feat/import-env
branch
from
68cf261
to
8b32baf
Compare
|
Thanks for adopting the suggestions. Retested this now on a cluster and touching the field doesn't break the Deployment anymore. 👍 UI looks fine for me, but we should now wait for a lgtm from UX/PM. /cc @serenamarie125 @beaumorley /label qe-approved |
openshift-ci
bot
added
qe-approved
rottencandy
force-pushed
the
feat/import-env
branch
2 times, most recently
from
8b233d6
to
0dd89a4
Compare
frontend/packages/dev-console/src/components/import/builder/BuilderImageEnvironments.tsx
Outdated
Show resolved
Hide resolved
rottencandy
force-pushed
the
feat/import-env
branch
from
0dd89a4
to
2855b95
Compare
openshift-ci
bot
added
component/topology
rottencandy
force-pushed
the
feat/import-env
branch
2 times, most recently
from
aecf3b1
to
9afe312
Compare
|
@jerolimov Wondering if we need all the help text under the form field? Could we remove the second sentence to reduce noise? Like this:
|
|
Updated: |
|
Tested this again on a cluster and works fine. Christian comment is implemented, so adding finally lgtm. /lgtm |
|
/retest |
| "type": "dev-console.import/environment", | ||
| "properties": { | ||
| "imageStreamName": "nodejs", | ||
| "imageStreamTags": ["14-ubi8", "14-ubi8-minimal", "12-ubi8", "latest"], |
There was a problem hiding this comment.
@rottencandy @christianvogt Any thoughts here if we should also add node 16- or ubi9 here already or should we update this shortly before we release 4.10?
See sclorg/s2i-nodejs-container#295
I created this follow up story to check this later: https://issues.redhat.com/browse/ODC-6414
There was a problem hiding this comment.
Don't think it would affect anything so added 16-ubi8 to the list.
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
Stop rerunning tests until we take a new look why this fails. |
openshift-ci
bot
added
the
do-not-merge/hold
| @@ -203,3 +203,13 @@ Feature: Create Application from git form | |||
| | git_url | | |||
| | https://github.com/sclorg/httpd-ex.git | | |||
| | https://github.com/sclorg/nginx-ex.git | | |||
|
|
|||
| @regression @manual | |||
There was a problem hiding this comment.
Btw, it would be still cool if this is not a manual (manually clicked through the UI) test and the statements below could run via cypress.
There was a problem hiding this comment.
Yup, removed manual. Since it is only a single API call, I don't think it would affect the rate limit issue that much.
| Scenario: Provide custom build environments for nodejs git import | ||
| Given user is at Import from Git form | ||
| When user enters Git Repo URL as "https://github.com/sclorg/nodejs-ex" | ||
| And user enters "NPM_RUN" build environment field as "build2" |
There was a problem hiding this comment.
That is not what the user does. The user enters a "Run command", not a build environment variable.
| And user enters Name as "nodejs-env" | ||
| And user clicks Create button on Add page | ||
| Then user will be redirected to Topology page | ||
| And user is able to see environment variable "NPM_RUN" in details page for Build Config "nodejs-env" |
There was a problem hiding this comment.
I'm sure the user needs to click on "Build #1" first, switch then to the environment variable tab and then can verify that the NPM_RUN variable is set.
There was a problem hiding this comment.
Updated to click on build instead of BuildConfig and changed statement text to reflect it. Or were you suggesting we do this in multiple separate steps instead of one?
rottencandy
force-pushed
the
feat/import-env
branch
from
0693d9b
to
8e35068
Compare
|
Thanks @rottencandy, looks much better. My local cypress has some (unrelated) issues. Let check if the tests pass on CI, but the code is fine 👍 /lgtm |
openshift-ci
bot
added
lgtm
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
Story:
https://issues.redhat.com/browse/ODC-6379
Description:
Create an extension that allows specifying extra build environment options for builder images in git import form.
Use the extension to provide option to customize npm run command(
NPM_RUN) for nodejs builder images.Screen shots / Gifs for design review:
Unit test coverage report:
(Unchanged)
Browser conformance: