Add devfile command flags to watch #3075

maysunfaisal · 2020-05-04T22:39:31Z

Signed-off-by: Maysun J Faisal maysun.j.faisal@ibm.com

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
This PR:

~~saves non default devfile commands odo push --build-command="mybuild" --run-command="myrun" --init-command="myinit" in .odo/env/env.yaml~~
Future push reads the non default commands from .odo/env/env.yaml and executes them without having to explicitly mention them, eg odo push after step 1 will execute mybuild instead of devbuild
~~odo watch can also now execute the non default push commands~~
~~Any future push with non custom commands, eg odo push --build-command="b", overwrites the previously saved push command in .odo/env/env.yaml~~
updates odo watch with devfile command flags as per Add devfile command flags to watch #3075 (comment), updated issue odo config support for settings in devfile.yaml #2688 for saving devfile commands
introduces new integration test for odo watch
helper functions for saving devfile commands to env.yaml that was written as part of the initial PR before pivoting to new logic

Which issue(s) this PR fixes:

Fixes #2819

How to test changes / Special notes to the reviewer:

Run watch with some custom commands odo watch --build-command="mybuild" --run-command="myrun" --init-command="myinit"

maysunfaisal · 2020-05-04T23:58:21Z

/retest v4.3-integration-e2e

openshift-ci-robot · 2020-05-04T23:58:34Z

@maysunfaisal: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

/test unit
/test v4.2-integration-e2e
/test v4.3-integration-e2e
/test v4.4-integration-e2e

Use /test all to run all jobs.

In response to this:

/retest v4.3-integration-e2e

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.

maysunfaisal · 2020-05-04T23:59:53Z

/test v4.3-integration-e2e

maysunfaisal · 2020-05-05T00:00:28Z

/test unit

maysunfaisal · 2020-05-05T18:32:36Z

/test v4.4-integration-e2e

maysunfaisal · 2020-05-05T18:33:31Z

@johnmcollier @mik-dass pls help review the PR changes for push, watch and devfile commands! thx..

codecov · 2020-05-05T20:51:26Z

Codecov Report

Merging #3075 into master will increase coverage by 0.02%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #3075      +/-   ##
==========================================
+ Coverage   45.46%   45.49%   +0.02%     
==========================================
  Files         110      110              
  Lines       10814    10830      +16     
==========================================
+ Hits         4917     4927      +10     
- Misses       5424     5429       +5     
- Partials      473      474       +1

Impacted Files	Coverage Δ
pkg/watch/watch.go	`64.60% <50.00%> (-0.69%)`	⬇️
pkg/envinfo/envinfo.go	`61.24% <71.42%> (+0.58%)`	⬆️
pkg/devfile/adapters/docker/component/adapter.go	`77.50% <100.00%> (+0.18%)`	⬆️

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 a35bba5...69de028. Read the comment docs.

johnmcollier · 2020-05-06T23:10:33Z

pkg/envinfo/envinfo.go

@@ -44,7 +42,7 @@ type EnvInfoURL struct {
 	// Indicates if the URL should be a secure https one
 	Secure bool `yaml:"Secure,omitempty"`
 	// Cluster host
-	Host string `yaml:"host,omitempty"`
+	Host string `yaml:"Host,omitempty"`


Does changing host to Host break anything (it shouldn't, but we should double check)

I tested kube and it was fine, but i will double confirm it again

maysunfaisal · 2020-05-06T23:18:12Z

/retest

mik-dass · 2020-05-07T11:25:33Z

Can we have some integration tests for both push and watch?

Also should we, instead on saving the commands during push, do it via some commands like odo env set? What am I missing here? @kadel WDUT?

maysunfaisal · 2020-05-07T13:39:27Z

@mik-dass @kadel that would require to execute one more command odo env set before doing odo push. Personally I'm more with the flow of saving the commands passed during odo push --build-command="xyz" --run-command="abc"; so that we can just do odo push/odo watch next onwards with the save commands.

As for the integration tests, how do we test odo watch? Seems like there are no integration tests for odo watch even for odo 1(non experimental) other than the bad cases (EDIT: I see e2e test have odo watch, so let me see if i can reuse it and have some integration test)

mik-dass · 2020-05-07T16:39:14Z

As for the integration tests, how do we test odo watch? Seems like there are no integration tests for odo watch even for odo 1(non experimental) other than the bad cases (EDIT: I see e2e test have odo watch, so let me see if i can reuse it and have some integration test)

Sorry I forget that we don't have integration tests for watch. Let's skip it for now.

@mik-dass @kadel that would require to execute one more command odo env set before doing odo push. Personally I'm more with the flow of saving the commands passed during odo push --build-command="xyz" --run-command="abc"; so that we can just do odo push/odo watch next onwards with the save commands.

I think if the user wants to do odo push --build-command="xyz" --run-command="abc" only for one time (maybe for debugging), he will have to rerun the command with the previous configuration to go back to the previous state. I think this will remove the user's ability to perform one time actions with the push command. Or am I missing something?

maysunfaisal · 2020-05-07T16:54:19Z

@mik-dass yes, your understanding is correct.

Case 1: devbuild and devrun commands:
I can just do odo push and odo watch everytime to execute these commands

Case 2: custom xyz and abc commands:
I have to odo push --build-command="xyz" --run-command="abc" but currently there is no way for odo watch to push with these commands because we dont have these flags. So i think it simplifies the UX, if watch was simply odo watch instead of odo watch --build-command="xyz" --run-command="abc"

This was the idea behind saving the non-default commands; so once you run odo push --build-command="xyz" --run-command="abc"; you can just do odo push & odo watch to execute xyz and abc commands.

But yes, if you want to switch back to some other commands, you will have to re-run push with them: odo push --build-command="devbuild", will switch to devbuild and still have abc for the run command; and you can still do odo push & odo watch with these new commands.

So, to summarize:

Minimize changes to odo watch without introducing any new flags for UX
Dont want to mention build-command or run-command everytime, if i am going to be pushing with them for some time. If I want to default back or change, run push again with new command.

mik-dass · 2020-05-07T19:09:32Z

So, to summarize: Minimize changes to odo watch without introducing any new flags for UX
Dont want to mention build-command or run-command everytime, if i am going to be pushing with them for some time. If I want to default back or change, run push again with new command.

I agree with those points 👍 But my only concern is that a user will lose the ability to execute push with those parameters only once without the need to roll back again. The options I can think of

A user defaults to these options using odo env set.
OR
A warning message while performing this action with push about the consequences and documentation regarding the flag usage as otherwise it might confuse him/her.

maysunfaisal · 2020-05-08T14:24:59Z

@mik-dass not sure I follow, when you say uses will lose the ability to use those parameters.. If users want they can still run push with those flags

odo push --build-command="xyz"
odo push is the same as running as odo push --build-command="xyz" at this point. Users can opt to use either of the way..

mik-dass · 2020-05-08T15:31:07Z

@mik-dass not sure I follow, when you say uses will lose the ability to use those parameters.. If users want they can still run push with those flags

I mean the user will have to use the flags again to go back to the previous state.

The normal build command is devBuild
The user wants to use odo push --build-command="xyz" maybe for one time.
Now the default is xyz so I will have to do `odo push --build-command="devBuild" to go back to the default.

So the user won't have a option to trigger with xyz for only one time. The user will have to switch back using the push command with the previous settings.

So with this behavior, let's display a info message indicating that the default has now changed and it is also in the flag descriptions and examples. Let's display something like
The default build command was updated after push is triggered with those flags.

johnmcollier · 2020-05-08T15:55:14Z

+1 on displaying an info message whenever the default has changed as a result of build-command, run-command, or init-command

maysunfaisal · 2020-05-08T16:20:57Z

@mik-dass @johnmcollier when would you like to display the info msg:

whenever --build-command/--run-command/--init-command is used during push
whenever non-default commands are executed?

With option 1; it will display only whenever user executes odo push --build-command, not thereafter with odo push

With option 2; it will display the info whenever a non-default command is executed; irrespective of odo push --build-command/odo push

kadel · 2020-05-26T08:03:41Z

/hold cancel

kadel · 2020-05-26T08:04:14Z

/retest

openshift-bot · 2020-05-26T08:09:27Z

/retest