Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support environment variables in command #3537

Merged

Conversation

yangcao77
Copy link
Contributor

@yangcao77 yangcao77 commented Jul 9, 2020

What type of PR is this?
/kind user-story
/area devfile

What does does this PR do / why we need it:
This PR allows to specify environment variables in a command in the devfile so that the environment variables are used when executing the command.

Which issue(s) this PR fixes:

Fixes #3406

How to test changes / Special notes to the reviewer:

automation:
run make test-cmd-devfile-push

manual:
Use the devfile.yaml within this PR: devfile-with-multiple-defaults.yaml

run odo create and odo push --build-command firstbuild --run-command singleenv
kubectl exec -it <pod> ls /projects/nodejs-web-app/ should see test_env_variable directory exist

@openshift-ci-robot openshift-ci-robot added kind/user-story An issue of user-story kind area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. labels Jul 9, 2020
@yangcao77 yangcao77 marked this pull request as draft July 9, 2020 17:59
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jul 9, 2020
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #3537 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3537      +/-   ##
==========================================
+ Coverage   45.94%   46.00%   +0.06%     
==========================================
  Files         114      114              
  Lines       11584    11598      +14     
==========================================
+ Hits         5322     5336      +14     
  Misses       5745     5745              
  Partials      517      517              
Impacted Files Coverage Δ
pkg/devfile/adapters/kubernetes/utils/utils.go 59.49% <100.00%> (+3.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81adbcc...ed4acf9. Read the comment docs.

@yangcao77 yangcao77 marked this pull request as ready for review July 13, 2020 17:43
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jul 13, 2020
@dharmit
Copy link
Member

dharmit commented Jul 15, 2020

@yangcao77 can you please also share a working example of this that would help test the functionality of this PR? If it's already there at some place (code/issue), please point me to it.

@dharmit
Copy link
Member

dharmit commented Jul 15, 2020

AFAICS, there's not make target by the specified name. I hope I'm not doing something really stupid here. 😢

How to test changes / Special notes to the reviewer:

automation:
run make test-push-devfile-test

$ git checkout upstream/pr/3537
Previous HEAD position was f7c51a7b0 Refactor odo delete logic
HEAD is now at 9b7cc15dc Merge branch 'master' of github.com:openshift/odo into 3406-environmentVariable

$ make test-push-devfile-test
make: *** No rule to make target 'test-push-devfile-test'.  Stop.

$ git log --oneline -n7
9b7cc15dc (HEAD, upstream/pr/3537) Merge branch 'master' of github.com:openshift/odo into 3406-environmentVariable
4d69aab92 added deprecation and error messages for 'odo update' (#3453)
8791263cf Fixing kubeconfig path and login session for 4.x cluster (#3528)
72b44d7bb Adds a error message for creation of routes with host flag (#3541)
c7d7fcb16 Update odo delete for no component and devfile.yaml (#3405)
42fe69318 used MatchJSON in tests wherever needed (#3523)
941bcf83b catalog search services honor experiment flag (#3366)

$ make && cp -f odo ~/bin    
go build -ldflags="-w -X github.com/openshift/odo/pkg/version.GITCOMMIT=9b7cc15dc" cmd/odo/odo.go

$ make test-push-devfile-test
make: *** No rule to make target 'test-push-devfile-test'.  Stop

@mik-dass
Copy link
Contributor

How to test changes / Special notes to the reviewer:
automation:
run make test-push-devfile-test

@yangcao77 I think this should be make test-cmd-devfile-push

Stephanie added 2 commits July 15, 2020 10:15
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
@yangcao77
Copy link
Contributor Author

@dharmit Sorry for the confusion, it should be make test-cmd-devfile-push

Also updated the manual test in the PR description if you want to try it manually.

wantErr: false,
},
{
name: "Case: custom command with multiple environment variable",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add cases for the debug commands as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Stephanie added 2 commits July 16, 2020 10:32
@yangcao77
Copy link
Contributor Author

/retest

Signed-off-by: Stephanie <stephanie.cao@ibm.com>
@openshift-bot
Copy link

/retest

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

23 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 21, 2020
@yangcao77
Copy link
Contributor Author

@mik-dass Can you help add back the lgtm label? I had to rebase to pull in the fix for the flaky test.

@mik-dass
Copy link
Contributor

@mik-dass Can you help add back the lgtm label? I had to rebase to pull in the fix for the flaky test.

Sure.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 21, 2020
@openshift-merge-robot openshift-merge-robot merged commit 5c2c609 into redhat-developer:master Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. kind/user-story An issue of user-story kind lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support environment variables for a specific command
7 participants