Allow a project to be specified in --downloadSource #3044

Kamran64 · 2020-04-29T11:07:22Z

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
This PR will enable the user to specify which project they want to download when using the --downloadSource flag. If no project is specified when using the flag, we download the first one in the list if there is only one, or resort to interactive mode if there are multiple.

Due to a limitation with the NoOptDefVal attribute in flags the format must be --downloadSource=<project-name> or just --downloadSource. See: spf13/pflag#134

Which issue(s) this PR fixes:
Fixes: #3000

How to test changes / Special notes to the reviewer:

Set odo to experimental mode
Run an odo create nodejs --downloadSource and optionally specify a project using --downloadSource=<project-name>.
Since the currently supported devfiles only have one project, the tests don't cover the interactive mode portion (although I'm not sure how that could be tested anyway)

openshift-ci-robot · 2020-04-29T11:07:44Z

Hi @Kamran64. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

mik-dass · 2020-04-29T11:10:35Z

/ok-to-test

…InDownloadSource

Kamran64 · 2020-04-29T11:15:45Z

/retest

Kamran64 · 2020-04-29T12:35:56Z

/retest

kadel · 2020-04-29T14:14:02Z

If no project is specified when using the flag, we download the first one in the list if there is only one, or resort to interactive mode if there are multiple.

I don't think that this is a good experience. From the begging of odo we have a philosophy that if you want to run interactive mode you do it from start, that means that you just do odo create to invoke full interactive mode.

When we start invoking interactive mode partially (just for some parameters and only if some conditions are met) it becomes really hard to create scripts that use odo.

I would prefer odo to show an error when you do odo create --downloadSource and the devfile has multiple projects. Other option would be to always download the first, and in case devfile has multiple projects include a warning to inform the user that odo is automatically picking the first one

kadel · 2020-04-29T14:19:47Z

Due to a limitation with the NoOptDefVal attribute in flags the format must be --downloadSource=<project-name> or just --downloadSource. See: spf13/pflag#134

hmm :-( is making it inconsistent with other flags :-(

pkg/odo/cli/component/create.go

Kamran64 · 2020-04-29T14:26:08Z

@kadel - I agree with the first point, will push a commit that downloads the first project if there are multiple whilst warning the user of this.

On your second point: I'm not too sure of any feasible solutions to get around this until its fixed in the package.. We could:

Make it mandatory that an argument is passed to the flag meaning we don't need to use NoOptDefVal but this may not be ideal either.

kadel · 2020-04-30T06:19:23Z

@kadel - I agree with the first point, will push a commit that downloads the first project if there are multiple whilst warning the user of this.

On your second point: I'm not too sure of any feasible solutions to get around this until its fixed in the package.. We could:

Make it mandatory that an argument is passed to the flag meaning we don't need to use NoOptDefVal but this may not be ideal either.

I was thinking about it, It would solve the problem, but I'm afraid
that it will create bad UX :-(
I assume that in most of the cases, there will be just one project in
a devfile. It will be pretty annoying to force users to always specify
project name especially when we don't have an easy way to list
projects from Devfiles via cli :-(

girishramnani · 2020-04-30T10:20:24Z

Is the project mentioned here different from odo project project? if so then this definitely creates confusion. because we would have openshift project, kubernetes project (namespace) and then devfile project.

neeraj-laad · 2020-04-30T10:38:12Z

Is the project mentioned here different from odo project project? if so then this definitely creates confusion. because we would have openshift project, kubernetes project (namespace) and then devfile project.

These are starter projects (specified in the Devfile). Typically there will be one, but there could be multiple projects.

no project specified for download - download the first one (optionally, show a warning if there are multiple projects defined in the devfile)
if project specified for download - download the specified one

Would love to get some feedback/suggestions on how to achieve this with best UX

kadel · 2020-04-30T11:14:52Z

These are starter projects (specified in the Devfile). Typically there will be one, but there could be multiple projects.

no project specified for download - download the first one (optionally, show a warning if there are multiple projects defined in the devfile)

+1
I like this.

@Kamran64 I really think that odo should not jump into an interactive mode like this.

if project specified for download - download the specified one

Kamran64 · 2020-04-30T11:26:17Z

@neeraj-laad @kadel - I agree this flow makes more sense. Thanks!

Kamran64 · 2020-04-30T11:54:13Z

@girishramnani - I can see this point too. This is supposed to allow the user to specify a devfile project. We will have to clarify this in docs/help text.

Kamran64 · 2020-04-30T14:20:05Z

Comments have been addressed in the latest commit. Now:

We select the first project if one hasn't been specified in --downloadSource. If there is more than one project in the devfile we still download the first but also warn the user that multiple exist.
Otherwise, a user can specify a project via --downloadSource=<project-name> and we verify that project exists before downloading.

I'll see if I can restart a discussion via an issue/PR to pflag and see if there's any way we can workaround/fix this NoOptDefVal problem.

GeekArthur · 2020-05-01T15:42:33Z

Is the project mentioned here different from odo project project? if so then this definitely creates confusion. because we would have openshift project, kubernetes project (namespace) and then devfile project.

These are starter projects (specified in the Devfile). Typically there will be one, but there could be multiple projects.
* no project specified for download - download the first one (optionally, show a warning if there are multiple projects defined in the devfile)

* if project specified for download - download the specified one
Would love to get some feedback/suggestions on how to achieve this with best UX

I think we have terminology confusion here regarding the terminology "project", with this new feature, we have two definitions for "project" now:

OpenShift project: Similar to namespace in k8s world
Devfile project: It's the project template that is hosted on GitHub or other source host

IMO, we should either make the terminology more clear in both our code (eg. help page, comment, etc) and documentation or we choose to use another terminology such as template, otherwise it may cause more confusions for OpenShift/Red Hat users as they assume "project" is same as "namespace" which is used for isolating generic k8s resource.

GeekArthur · 2020-05-01T15:52:36Z

Due to a limitation with the NoOptDefVal attribute in flags the format must be --downloadSource=<project-name> or just --downloadSource. See: spf13/pflag#134

hmm :-( is making it inconsistent with other flags :-(

I agree with @kadel, it's inconsistent with other flags if I run odo create --help

odo create --help                                                                                                                                                 ✔  11:19:39 
Create a configuration describing a component.

 If a component name is not provided, it'll be auto-generated.

 A full list of component types that can be deployed is available using: 'odo catalog list'

 By default, builder images (component type) will be used from the current namespace. You can explicitly supply a namespace by using: odo create namespace/name:version If version is not specified by default, latest will be chosen as the version.

Usage:
  odo create <component_type> [component_name] [flags]

Examples:
  # Create new Node.js component with the source in current directory.
  odo create nodejs

  # Create new Node.js component named 'frontend' with the source in './frontend' directory
  odo create nodejs frontend --context ./frontend

  # Create new Java component with binary named sample.jar in './target' directory
  odo create java:8  --binary target/sample.jar

  # Create new Node.js component with source from remote git repository
  odo create nodejs --git https://github.com/openshift/nodejs-ex.git

  # Create new Node.js component with custom ports, additional environment variables and memory and cpu limits
  odo create nodejs --port 8080,8100/tcp,9100/udp --env key=value,key1=value1 --memory 4Gi --cpu 2

Flags:
      --app string                                            Application, defaults to active application
  -b, --binary string                                         Create a binary file component component using given artifact. Works only with Java components. File needs to be in the context directory.
      --context string                                        Use given context directory as a source for component settings
      --cpu string                                            Amount of cpu to be allocated to the component. ex. 100m or 0.1 (sets min-cpu and max-cpu to this value)
      --downloadSource string[="no-project-passed-to-flag"]   Download sample project from devfile. (ex. odo component create <component_type> [component_name] --downloadSource/--downloadSource=<devfile-project>
      --env stringSlice                                       Environmental variables for the component. For example --env VariableName=Value
  -g, --git string                                            Create a git component using this repository.
  -h, --help                                                  Help for create
      --max-cpu string                                        Limit maximum amount of cpu to be allocated to the component. ex. 1
      --max-memory string                                     Limit maximum amount of memory to be allocated to the component. ex. 100Mi
      --memory string                                         Amount of memory to be allocated to the component. ex. 100Mi (sets min-memory and max-memory to this value)
      --min-cpu string                                        Limit minimum amount of cpu to be allocated to the component. ex. 100m
      --min-memory string                                     Limit minimum amount of memory to be allocated to the component. ex. 100Mi
      --now                                                   Push changes to the cluster immediately
  -p, --port stringSlice                                      Ports to be used when the component is created (ex. 8080,8100/tcp,9100/udp)
      --project string                                        Project, defaults to active project
  -r, --ref string                                            Use a specific ref e.g. commit, branch or tag of the git repository

Additional Flags:
  -o, --o string             Specify output format, supported format: json (default "json")
  -v, --v Level              Log level for V logs. Level varies from 0 to 9 (default 0).
      --vmodule moduleSpec   Comma-separated list of pattern=N settings for file-filtered logging

Especially this help message --downloadSource string[="no-project-passed-to-flag"] Download sample project from devfile. (ex. odo component create <component_type> [component_name] --downloadSource/--downloadSource=<devfile-project> it's a little confused here --downloadSource [project name] VS --downloadSource=[project name] (I know part of help message is generated automatically from cobra package and it's hard resolve the combination issue)

GeekArthur · 2020-05-01T15:58:00Z

Another comment from UX perspective for this use case:

We select the first project if one hasn't been specified in --downloadSource. If there is more than one project in the devfile we still download the first but also warn the user that multiple exist.

I think we should return the project name directly to user in warning message instead of simply return we are downloading the first project, so that user doesn't need to go back to check the devfile again for confirming which project is downloading.

 ⚠  There are multiple projects in this devfile but none have been specified in --downloadSource. Downloading the first.

Kamran64 · 2020-05-01T16:00:16Z

@GeekArthur - ah I didn't realise the default value would be displayed in the help text. Maybe we can change the name to something clearer that indicates a devfile project has to be passed after the = e.g. devfile-project-name.

GeekArthur · 2020-05-01T16:03:42Z

Regarding integration test, even though we remove the sub-interactive mode this feature still need to be tested for full-interactive mode as users can use their own devfile with multiple/single projects in their working directory, given currently we don't have tests for interactive mode, we have to wait for this issue #2194, @amitkrout any progress for that issue? It would be great we can finish that as soon as possible.

GeekArthur · 2020-05-01T16:05:01Z

/area devfile

Kamran64 · 2020-05-04T09:05:46Z

@GeekArthur - Are you suggesting we implement interactive mode to some level in this PR?

codecov · 2020-05-04T12:20:20Z

Codecov Report

Merging #3044 into master will increase coverage by 1.32%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3044      +/-   ##
==========================================
+ Coverage   44.69%   46.01%   +1.32%     
==========================================
  Files         107      108       +1     
  Lines       10126    10259     +133     
==========================================
+ Hits         4526     4721     +195     
+ Misses       5164     5082      -82     
- Partials      436      456      +20

Impacted Files	Coverage Δ
...g/devfile/adapters/kubernetes/component/adapter.go	`31.08% <0.00%> (-0.87%)`	⬇️
pkg/occlient/occlient.go	`50.83% <0.00%> (-0.52%)`	⬇️
pkg/envinfo/envinfo.go	`60.65% <0.00%> (ø)`
pkg/config/fakeConfig.go	`0.00% <0.00%> (ø)`
pkg/testingutil/devfile.go	`0.00% <0.00%> (ø)`
pkg/devfile/adapters/docker/component/adapter.go	`69.60% <0.00%> (ø)`
pkg/testingutil/routes.go	`0.00% <0.00%> (ø)`
pkg/devfile/adapters/common/command.go	`97.84% <0.00%> (+0.31%)`	⬆️
pkg/devfile/adapters/docker/component/utils.go	`73.85% <0.00%> (+0.61%)`	⬆️
pkg/debug/info.go	`67.56% <0.00%> (+1.85%)`	⬆️
... and 3 more

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 3b13d98...5502239. Read the comment docs.

GeekArthur · 2020-05-04T14:35:46Z

@GeekArthur - Are you suggesting we implement interactive mode to some level in this PR?

Not adding extra interactive mode to this PR, just adding auto tests to test the changes with existing interactive mode to ensure no regression, but that can't be implemented for now due to #3044 (comment), for this PR I manually test it, so we all good.

Kamran64 · 2020-05-04T14:46:05Z

Understood, just wanted to be sure. I changed the default string and warning message so hopefully the help text is a little bit more clear now. As for the = problem, still no solution in sight 😞

kadel · 2020-05-07T12:17:25Z

I think we have terminology confusion here regarding the terminology "project", with this new feature, we have two definitions for "project" now:

OpenShift project: Similar to namespace in k8s world

Devfile project: It's the project template that is hosted on GitHub or other source host

IMO, we should either make the terminology more clear in both our code (eg. help page, comment, etc) and documentation or we choose to use another terminology such as template, otherwise it may cause more confusions for OpenShift/Red Hat users as they assume "project" is same as "namespace" which is used for isolating generic k8s resource.

I agree that the terminology is confusing :-(
But I can't think of a good solution right now :-(
If we use something like "template" then it will be confusing for anyone familiar with devfile, as there is no template in devfile :-(

pkg/odo/cli/component/create.go

Kamran64 · 2020-05-07T19:53:08Z

@johnmcollier @kadel Done :)

kadel · 2020-05-11T12:22:25Z

/retest
/approve

kadel · 2020-05-13T10:25:45Z

/approve

kadel · 2020-05-13T10:25:57Z

/retest

openshift-ci-robot · 2020-05-13T10:26:18Z

[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

adisky · 2020-05-13T11:09:53Z

Looks good to me, i believe the documentation has been updated in this PR.
#3163

johnmcollier

Code looks good, and CI failure is known flake.

/lgtm

johnmcollier · 2020-05-13T12:42:25Z

/retest

Allow a project to be specified in --downloadSource

7835874

openshift-ci-robot added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Apr 29, 2020

openshift-ci-robot requested review from adisky and mohammedzee1000 April 29, 2020 11:07

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. label Apr 29, 2020

Merge branch 'master' of github.com:openshift/odo into specifyProject…

9b270a2

…InDownloadSource

Added test for devfile with no projects

bc7dc1b

kadel reviewed Apr 29, 2020

View reviewed changes

pkg/odo/cli/component/create.go Outdated Show resolved Hide resolved

Remove interactive mode feature.

4b16239

openshift-ci-robot added the area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. label May 1, 2020

Added project name to warning, altered default name

ba8f12d

kadel reviewed May 7, 2020

View reviewed changes

pkg/odo/cli/component/create.go Outdated Show resolved Hide resolved

johnmcollier suggested changes May 7, 2020

View reviewed changes

pkg/odo/cli/component/create.go Outdated Show resolved Hide resolved

openshift-ci-robot assigned johnmcollier May 7, 2020

Address comments

5502239

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 10, 2020

Merge branch 'master' into specifyProjectInDownloadSource

5c9438c

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label May 10, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label May 13, 2020

johnmcollier approved these changes May 13, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 13, 2020

openshift-merge-robot merged commit 19ed979 into redhat-developer:master May 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow a project to be specified in --downloadSource #3044

Allow a project to be specified in --downloadSource #3044

Kamran64 commented Apr 29, 2020 •

edited

Loading

openshift-ci-robot commented Apr 29, 2020

mik-dass commented Apr 29, 2020

Kamran64 commented Apr 29, 2020

Kamran64 commented Apr 29, 2020

kadel commented Apr 29, 2020

kadel commented Apr 29, 2020

Kamran64 commented Apr 29, 2020

kadel commented Apr 30, 2020

girishramnani commented Apr 30, 2020

neeraj-laad commented Apr 30, 2020

kadel commented Apr 30, 2020

Kamran64 commented Apr 30, 2020

Kamran64 commented Apr 30, 2020

Kamran64 commented Apr 30, 2020 •

edited

Loading

GeekArthur commented May 1, 2020

GeekArthur commented May 1, 2020

GeekArthur commented May 1, 2020

Kamran64 commented May 1, 2020

GeekArthur commented May 1, 2020 •

edited

Loading

GeekArthur commented May 1, 2020

Kamran64 commented May 4, 2020

codecov bot commented May 4, 2020 •

edited

Loading

GeekArthur commented May 4, 2020

Kamran64 commented May 4, 2020 •

edited

Loading

kadel commented May 7, 2020

Kamran64 commented May 7, 2020

kadel commented May 11, 2020 •

edited

Loading

kadel commented May 13, 2020

kadel commented May 13, 2020

openshift-ci-robot commented May 13, 2020

adisky commented May 13, 2020

johnmcollier left a comment

johnmcollier commented May 13, 2020

Allow a project to be specified in --downloadSource #3044

Allow a project to be specified in --downloadSource #3044

Conversation

Kamran64 commented Apr 29, 2020 • edited Loading

openshift-ci-robot commented Apr 29, 2020

mik-dass commented Apr 29, 2020

Kamran64 commented Apr 29, 2020

Kamran64 commented Apr 29, 2020

kadel commented Apr 29, 2020

kadel commented Apr 29, 2020

Kamran64 commented Apr 29, 2020

kadel commented Apr 30, 2020

girishramnani commented Apr 30, 2020

neeraj-laad commented Apr 30, 2020

kadel commented Apr 30, 2020

Kamran64 commented Apr 30, 2020

Kamran64 commented Apr 30, 2020

Kamran64 commented Apr 30, 2020 • edited Loading

GeekArthur commented May 1, 2020

GeekArthur commented May 1, 2020

GeekArthur commented May 1, 2020

Kamran64 commented May 1, 2020

GeekArthur commented May 1, 2020 • edited Loading

GeekArthur commented May 1, 2020

Kamran64 commented May 4, 2020

codecov bot commented May 4, 2020 • edited Loading

Codecov Report

GeekArthur commented May 4, 2020

Kamran64 commented May 4, 2020 • edited Loading

kadel commented May 7, 2020

Kamran64 commented May 7, 2020

kadel commented May 11, 2020 • edited Loading

kadel commented May 13, 2020

kadel commented May 13, 2020

openshift-ci-robot commented May 13, 2020

adisky commented May 13, 2020

johnmcollier left a comment

Choose a reason for hiding this comment

johnmcollier commented May 13, 2020

Kamran64 commented Apr 29, 2020 •

edited

Loading

Kamran64 commented Apr 30, 2020 •

edited

Loading

GeekArthur commented May 1, 2020 •

edited

Loading

codecov bot commented May 4, 2020 •

edited

Loading

Kamran64 commented May 4, 2020 •

edited

Loading

kadel commented May 11, 2020 •

edited

Loading