Remove devfile push cmd #2598

kanchwala-yusuf · 2020-02-12T17:07:39Z

What type of PR is this?

/kind feature

What does does this PR do / why we need it:
This PR removes the push-devfile command and replaces it's behavior by adding the --devfile flag to the odo push command itself.

Which issue(s) this PR fixes:

Fixes #2538

How to test changes / Special notes to the reviewer:
The experimental mode needs to be enabled and the --devfile flag needs to be explicitly set for the devfile feature to work.

elsony · 2020-02-12T20:50:08Z

Do we need the --devfile flag to decide whether we use the devfile path for odo push? It means that the user will need to specify --devfile on every odo push command. Will it be simpler if we detect the existence of the devfile in the project to switch to use the devfile code path for odo push when EXPERIMENTAL flag is set to true?

kanchwala-yusuf · 2020-02-13T06:44:51Z

/retest

kanchwala-yusuf · 2020-02-13T07:02:18Z

Do we need the --devfile flag to decide whether we use the devfile path for odo push? It means that the user will need to specify --devfile on every odo push command. Will it be simpler if we detect the existence of the devfile in the project to switch to use the devfile code path for odo push when EXPERIMENTAL flag is set to true?

@elsony,
While the devfile feature is in experimental mode, wouldn't it make sense to provide the odo user with flexibility to choose between the orthodox odo push behavior and the new devfile feature? The --devfile precisely does that. If we automatically detect the presence of devfile.yaml, even though the experimental mode is enabled, there is no way for the user to switch back to the old odo push behavior. The only way is to disable the experimental mode.

We may add other experimental features which are not related to devfiles (operator hub integration for example), in such a case we end up using devfiles even for these experimental features.

Your suggestion would definitely be helpful, when the devfile feature is not in experimental mode anymore and devfile is accepted as the default standard for configuring odo.

cc: @kadel

kanchwala-yusuf · 2020-02-13T10:42:59Z

/test v4.2-integration-e2e-benchmark

kadel · 2020-02-13T11:24:53Z

Do we need the --devfile flag to decide whether we use the devfile path for odo push? It means that the user will need to specify --devfile on every odo push command. Will it be simpler if we detect the existence of the devfile in the project to switch to use the devfile code path for odo push when EXPERIMENTAL flag is set to true?

@elsony,
While the devfile feature is in experimental mode, wouldn't it make sense to provide the odo user with flexibility to choose between the orthodox odo push behavior and the new devfile feature? The --devfile precisely does that. If we automatically detect the presence of devfile.yaml, even though the experimental mode is enabled, there is no way for the user to switch back to the old odo push behavior. The only way is to disable the experimental mode.

We may add other experimental features which are not related to devfiles (operator hub integration for example), in such a case we end up using devfiles even for these experimental features.

Your suggestion would definitely be helpful, when the devfile feature is not in experimental mode anymore and devfile is accepted as the default standard for configuring odo.

cc: @kadel

I was actually also thinking that if experimental is enabled and there is devfile.yaml in the directory odo push should automatically pick it up.
One of the reasons why ODO_EXPERIMENTAL can be useful is to test odo behavior before we make it default. And I think that this is how it should work.

EXPERIMENTAL	devfile.yaml present	.odo/config.yaml present	odo push behavior	odo push --devfile devfile.yaml
TRUE	yes	yes	use devfile.yaml	use devfile.yaml
TRUE	yes	no	use devfile.yaml	use devfile.yaml
TRUE	no	yes	use .odo/config.yaml	use devfile.yaml
TRUE	no	no	error out (not existing component)	error out (not existing component)
FALSE	yes	yes	use .odo/config.yaml	error out (not existing flag --devfile)
FALSE	yes	no	use .odo/config.yaml	error out (not existing flag --devfile)
FALSE	no	yes	use .odo/config.yaml	error out (not existing flag --devfile)
FALSE	no	no	error out (not existing component)	error out (not existing flag --devfile)

--devfile flag is still needed as it will force the use of devfile even if someone names it differently. For example ODO_EXPERIMENTAL=true odo push --devfile mydevfile.yaml

kanchwala-yusuf · 2020-02-13T12:31:56Z

Do we need the --devfile flag to decide whether we use the devfile path for odo push? It means that the user will need to specify --devfile on every odo push command. Will it be simpler if we detect the existence of the devfile in the project to switch to use the devfile code path for odo push when EXPERIMENTAL flag is set to true?

@elsony,
While the devfile feature is in experimental mode, wouldn't it make sense to provide the odo user with flexibility to choose between the orthodox odo push behavior and the new devfile feature? The --devfile precisely does that. If we automatically detect the presence of devfile.yaml, even though the experimental mode is enabled, there is no way for the user to switch back to the old odo push behavior. The only way is to disable the experimental mode.
We may add other experimental features which are not related to devfiles (operator hub integration for example), in such a case we end up using devfiles even for these experimental features.
Your suggestion would definitely be helpful, when the devfile feature is not in experimental mode anymore and devfile is accepted as the default standard for configuring odo.
cc: @kadel

I was actually also thinking that if experimental is enabled and there is devfile.yaml in the directory odo push should automatically pick it up.
One of the reasons why ODO_EXPERIMENTAL can be useful is to test odo behavior before we make it default. And I think that this is how it should work.

EXPERIMENTAL devfile.yaml present .odo/config.yaml present odo push behavior odo push --devfile devfile.yaml
TRUE yes yes use devfile.yaml use devfile.yaml
TRUE yes no use devfile.yaml use devfile.yaml
TRUE no yes use .odo/config.yaml use devfile.yaml
TRUE no no error out (not existing component) error out (not existing component)
FALSE yes yes use .odo/config.yaml error out (not existing flag --devfile)
FALSE yes no use .odo/config.yaml error out (not existing flag --devfile)
FALSE no yes use .odo/config.yaml error out (not existing flag --devfile)
FALSE no no error out (not existing component) error out (not existing flag --devfile)
--devfile flag is still needed as it will force the use of devfile even if someone names it differently. For example ODO_EXPERIMENTAL=true odo push --devfile mydevfile.yaml

@kadel,
Wouldn't it make sense to implement the above matrix when the devfile feature implementation is complete? Currently, only the devfile parser has been implemented. The complete odo push workflow for devfile is still under development.

We can implement the above matrix, once the devfile feature implementation is complete and then gradually move the devfile feature out of the experimental mode.

Another aspect here is that with the above matrix, wouldn't it be difficult for us to add any other feature into experimental mode, since we would be defaulting to devfile feature?

codecov · 2020-02-13T13:03:36Z

Codecov Report

Merging #2598 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2598      +/-   ##
==========================================
- Coverage   42.77%   42.62%   -0.15%     
==========================================
  Files          74       74              
  Lines        7089     7089              
==========================================
- Hits         3032     3022      -10     
- Misses       3768     3774       +6     
- Partials      289      293       +4

Impacted Files	Coverage Δ
pkg/component/watch.go	`66% <0%> (-6.67%)`	⬇️

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 e21d6c6...7e54afb. Read the comment docs.

kanchwala-yusuf · 2020-02-13T15:46:54Z

After having a chat with @kadel, it seems that we want the features in experimental mode to work just the way we would want them to work when these features are removed out of the experimental mode. So, I would make the necessary changes to accommodate for the suggestion given by @kadel and @elsony .

kanchwala-yusuf · 2020-02-14T04:32:47Z

/test v4.1-integration-e2e-benchmark

amitkrout · 2020-02-14T09:55:23Z

How to test changes / Special notes to the reviewer:
The experimental mode needs to be enabled and the --devfile flag needs to be explicitly set for the devfile feature to work.

@kanchwala-yusuf As a user i want to visualize the change done by this pr, so can you please elaborate it or may be the steps would really be helpful.

kadel · 2020-02-14T10:57:54Z

/approve

openshift-ci-robot · 2020-02-14T10:58:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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~~ [kadel]

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

dharmit · 2020-02-17T04:15:59Z

After having a chat with @kadel, it seems that we want the features in experimental mode to work just the way we would want them to work when these features are removed out of the experimental mode. So, I would make the necessary changes to accommodate for the suggestion given by @kadel and @elsony .

Do we think this will have any impact on the work-flow of Adapters that use our tool? If yes, have we discussed the changes they need to make?

dharmit

Just one question in the code and one question outside the code. Looks good otherwise. 👍

dharmit · 2020-02-17T04:21:37Z

pkg/odo/cli/component/push.go

@@ -32,6 +34,9 @@ const PushRecommendedCommandName = "push"
 // PushOptions encapsulates options that push command uses
 type PushOptions struct {
 	*CommonPushOptions
+
+	// devfile path
+	devfilePath string


Why not make this a part of the CommonPushOptions struct in common_push.go?

Because the changes in this PR do not affect the common push code path in common_push.go.

In my understanding, common push has code which is common to the various odo push code paths whereas in the case of devfile there is no common code at all (atleast for now).

I think odo push is commong across different use cases, i.e., local sourcetype or git sourcetype or binary sourcetype. It behaves according to the underlying sourcetype. Considering DevFile is going to replace odo config (.odo/config.yaml) at some point, IMO, it would make more sense to make devfilePath string a part of CommonPushOptions.

Having said that, I agree that it's probably OK to keep it as-is right now. I hope we don't forget/overlook when we make it a default behaviour. 😉

kadel · 2020-02-17T09:44:23Z

Do we think this will have any impact on the work-flow of Adapters that use our tool? If yes, have we discussed the changes they need to make?

Not at this point. This is one of the reasons why we want to hide it under an experimental flag, at least for now.

dharmit · 2020-02-18T06:02:31Z

/lgtm

openshift-bot · 2020-02-18T08:24:04Z

/retest