Devfile support for odo create #2699

GeekArthur · 2020-03-10T19:15:12Z

Signed-off-by: jingfu wang jingfu.j.wang@ibm.com

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
This PR implements devfile support for odo create, basically the odo create command does two things:

Download supported devfile.yaml from predefined registries to user's local file system
Create configuration file (file path: ./.odo/env/env.yaml) and store component metadata (eg. component name, component namespace) into the the configuration file

It covers three cases below:

odo create <component type>
odo create <component type> <component name>
odo create <component type> <component name> --project <namespace>

Note: Supported devfile component should satisfy the following conditions:

Devfile has dockerimage as component type
Devfile has alias
Devfile has devRun command
Devfile has devBuild command

Which issue(s) this PR fixes:
Fixes #2557
Fixes #2684

How to test changes / Special notes to the reviewer:
Unit tests have been done in this PR. All devfile support integration tests can start once integration test suites PR merges in.

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

openshift-ci-robot · 2020-03-10T19:15:27Z

Hi @GeekArthur. 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.

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

kadel · 2020-03-11T17:03:34Z

/ok-to-test

pkg/envinfo/envinfo.go

openshift-ci-robot · 2020-03-11T18:24:46Z

@yangcao77: changing LGTM is restricted to collaborators

In response to this:

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.

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

johnmcollier

Nice changes Jingfu. I've left some review comments. Can you add some integration tests too? Since your changes don't rely on odo push, you won't need my changes in #2681.

Additionally, if I do odo create <component> where <component> is a non existent devfile component, it tells me:

johns-mbp-3:odo johncollier$ ./odo create fakedevfile
 ✗  imagestreams.image.openshift.io "fakedevfile" not found

We shouldn't be telling them that an image stream with that name didn't exist, but rather a devfile component with that name didn't exist

pkg/envinfo/envinfo.go

pkg/odo/cli/component/create.go

johnmcollier · 2020-03-12T01:01:25Z

Another thought I've had: I'm thinking odo createshould also be pulling down any associated projects in the devfile?

E.g. if a devfile has:

projects:
  -
    name: springbootproject
    source:
      type: git
      location: "https://github.com/spring-projects/spring-petclinic"

Then odo would git clone this project (and any other projects in the devfile) during odo create. This would be in-line with how Che treats projects in devfiles (it clones the projects when the workspaces is first created)

maysunfaisal

Just small nits and small changes around component name

pkg/odo/cli/component/create.go

pkg/envinfo/envinfo.go

GeekArthur · 2020-03-12T15:27:49Z

Nice changes Jingfu. I've left some review comments. Can you add some integration tests too?
Since your changes don't rely on odo push, you won't need my changes in #2681.

Sure, I can add the integration tests for odo create.

Additionally, if I do odo create where is a non existent devfile component, it tells me:
johns-mbp-3:odo johncollier$ ./odo create fakedevfile
✗  imagestreams.image.openshift.io "fakedevfile" not found
We shouldn't be telling them that an image stream with that name didn't exist, but rather a devfile component with that name didn't exist

That's the current expected behavior and I agree the error message a little bit misleading. I discussed this with Elson, basically the design is we wanna support both devfile and s2i but devfile has higher priority, that means if we can't find the component type in devfile we still keep checking if we can find that in s2i, that why you hit error from s2i instead of devfile. That's being said, I can add error message for devfile but don't terminate the code. So for the case that component don't exist in both devfile and s2i the error message would be something like:

Checking if the component type "fakedevfile" is supported by devfile
X Component type "fakedevfile" is not supported by devfile

Checking if the component type "fakedevfile" is supported by s2i
X imagestreams.image.openshift.io "fakedevfile" not found

GeekArthur · 2020-03-12T15:38:41Z

Another thought I've had: I'm thinking odo createshould also be pulling down any associated projects in the devfile?

E.g. if a devfile has:
projects:
  -
    name: springbootproject
    source:
      type: git
      location: "https://github.com/spring-projects/spring-petclinic"
Then odo would git clone this project (and any other projects in the devfile) during odo create. This would be in-line with how Che treats projects in devfiles (it clones the projects when the workspaces is first created)

If we follow the existing odo design, odo create shouldn't pull down the project source/template automatically according to the devfile.yaml. I think the problem is that user would have issue to build their own project if we help user pull down the project automatically, and I think this may results in the affection of bind case once we integrate with Codewind and cause some conflict since currently IDE/Portal handles pulling down project source/template.

…fileCreateCase Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

codecov · 2020-03-17T17:52:47Z

Codecov Report

Merging #2699 into master will increase coverage by 0.1%.
The diff coverage is 64.28%.

@@            Coverage Diff            @@
##           master    #2699     +/-   ##
=========================================
+ Coverage   43.48%   43.58%   +0.1%     
=========================================
  Files          91       91             
  Lines        8222     8264     +42     
=========================================
+ Hits         3575     3602     +27     
- Misses       4298     4311     +13     
- Partials      349      351      +2

Impacted Files	Coverage Δ
pkg/util/config_util.go	`0% <0%> (ø)`	⬆️
pkg/catalog/catalog.go	`51.59% <0%> (-0.19%)`	⬇️
pkg/envinfo/envinfo.go	`60.65% <33.33%> (-2.99%)`	⬇️
pkg/util/util.go	`67.09% <85.18%> (+1.36%)`	⬆️

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 0add600...2a8212d. Read the comment docs.

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

pkg/envinfo/envinfo_test.go

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

openshift-ci-robot · 2020-03-23T11:54:41Z

[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-03-23T13:13:07Z

@GeekArthur Since Integration tests are not here, could please add manual steps/example to test this PR.

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

GeekArthur · 2020-03-23T15:55:29Z

@GeekArthur Since Integration tests are not here, could please add manual steps/example to test this PR.

@adisky Thanks! I will add my integration tests to the corresponding .travis.yml and Makefile to enable the integration tests

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

GeekArthur · 2020-03-23T18:37:44Z

/retest

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

GeekArthur · 2020-03-23T22:14:01Z

/retest

adisky · 2020-03-24T11:50:00Z

pkg/odo/cli/component/create.go

+			spinner := log.Spinner("Validating component")
+			defer spinner.End(false)
+
+			if util.CheckPathExists(DevfilePath) || util.CheckPathExists(EnvFilePath) {


we need to check this before running catalogDevfileList, err := catalog.ListDevfileComponents(), currently if you run odo create openLiberty and then run odo create openLiberty in the same folder it is taking too much time to return "Devfile.yaml or env.yaml already exists, I think this should be the first validation.

By design after we implement devfile support for odo, odo needs to support both devfile and s2i components, but devfile component has higher priority.

catalogDevfileList, err := catalog.ListDevfileComponents() helps us check devfile registry so that we can validate if the component is supported devfile component, if it's not supported devfile component we won't validate Devfile.yaml or env.yaml already exists and we go to the existing s2i code path. That's why we call catalogDevfileList, err := catalog.ListDevfileComponents() before Devfile.yaml or env.yaml already exists

adisky · 2020-03-24T11:55:45Z

pkg/odo/cli/component/create.go

@@ -265,6 +285,109 @@ func (co *CreateOptions) setResourceLimits() error {

 // Complete completes create args
 func (co *CreateOptions) Complete(name string, cmd *cobra.Command, args []string) (err error) {
+	if experimental.IsExperimentalModeEnabled() {


just a thought should we make separate function func (co *CreateOptions) completeDevfile(...) and do like this here

if experimental.IsExperimentalModeEnabled() { completeDevfile(...) }

By design after we implement devfile support for odo, odo needs to support both devfile and s2i components, but devfile component has higher priority.

So the experimental flag will be removed finally, currently we don't separate the generic Complete, Validate and Run functions for all devfile support for odo features (eg odo create, odo push, odo url). I think the reason is it would be better for refactoring once we remove the experimental flag.

adisky · 2020-03-24T11:58:57Z

@GeekArthur I tried this out, Works well. Looks good to me.

kadel · 2020-03-24T12:27:13Z

/retest

johnmcollier · 2020-03-24T19:44:44Z

/retest

johnmcollier · 2020-03-25T15:30:48Z

/retest

johnmcollier

@GeekArthur See my note on the integration tests, it may be what's causing the flakiness in your tests

tests/integration/cmd_devfile_create_test.go

…fileCreateCase

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

GeekArthur · 2020-03-26T02:50:37Z

/retest

johnmcollier

/lgtm

openshift-bot · 2020-03-26T14:50:36Z

/retest

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

openshift-bot · 2020-03-26T17:00:34Z

/retest

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

Devfile support for odo create

54f681c

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

openshift-ci-robot added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. labels Mar 10, 2020

openshift-ci-robot added the size/L label Mar 10, 2020

openshift-ci-robot requested review from kanchwala-yusuf and rajivnathan March 10, 2020 19:16

rajivnathan mentioned this pull request Mar 11, 2020

Execute commands during Devfile push #2654

Closed

9 tasks

Add test case for envinfo

e6f6bc0

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. labels Mar 11, 2020

yangcao77 suggested changes Mar 11, 2020

View reviewed changes

pkg/envinfo/envinfo.go Show resolved Hide resolved

Update envinfo test case

8a8a1af

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

johnmcollier suggested changes Mar 11, 2020

View reviewed changes

pkg/envinfo/envinfo.go Show resolved Hide resolved

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

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

openshift-ci-robot assigned johnmcollier Mar 11, 2020

johnmcollier reviewed Mar 11, 2020

View reviewed changes

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

maysunfaisal reviewed Mar 12, 2020

View reviewed changes

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

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

pkg/envinfo/envinfo.go Show resolved Hide resolved

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

Merge branch 'master' of github.com:openshift/odo into issue-2557-dev…

635a333

…fileCreateCase Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

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

Update for all comments

3ee1e60

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

kadel reviewed Mar 19, 2020

View reviewed changes

pkg/envinfo/envinfo_test.go Outdated Show resolved Hide resolved

kadel reviewed Mar 19, 2020

View reviewed changes

pkg/envinfo/envinfo_test.go Outdated Show resolved Hide resolved

Add integration tests

faa2567

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

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

update integration tests

59499be

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

Enable integration tests

1a54631

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

Disable experimental flag after finishing tests

3a52327

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

adisky reviewed Mar 24, 2020

View reviewed changes

johnmcollier suggested changes Mar 25, 2020

View reviewed changes

tests/integration/cmd_devfile_create_test.go Outdated Show resolved Hide resolved

cdrage mentioned this pull request Mar 25, 2020

Unexpected project creation based on .odo/config.yaml setting #2285

Closed

GeekArthur added 2 commits March 25, 2020 18:13

Merge branch 'master' of github.com:openshift/odo into issue-2557-dev…

ad55a6e

…fileCreateCase

Update integration test

2a8212d

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>

johnmcollier approved these changes Mar 26, 2020

View reviewed changes

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

rajivnathan mentioned this pull request Mar 26, 2020

Odo catalog list components format #2736

Merged

openshift-merge-robot merged commit ad6c2cd into redhat-developer:master Mar 26, 2020

GeekArthur mentioned this pull request May 8, 2020

Add --devfile flag support #3118

Merged

7 tasks

rm3l mentioned this pull request Apr 11, 2022

All-numeric Kubernetes namespaces cannot be used with odo although they are valid for K8s #5652

Closed

rm3l added estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. and removed size/L labels Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Devfile support for odo create #2699

Devfile support for odo create #2699

GeekArthur commented Mar 10, 2020 •

edited

openshift-ci-robot commented Mar 10, 2020

kadel commented Mar 11, 2020

openshift-ci-robot commented Mar 11, 2020

johnmcollier left a comment •

edited

johnmcollier commented Mar 12, 2020 •

edited

maysunfaisal left a comment

GeekArthur commented Mar 12, 2020 •

edited

GeekArthur commented Mar 12, 2020

codecov bot commented Mar 17, 2020 •

edited

openshift-ci-robot commented Mar 23, 2020

adisky commented Mar 23, 2020

GeekArthur commented Mar 23, 2020

GeekArthur commented Mar 23, 2020

GeekArthur commented Mar 23, 2020

adisky Mar 24, 2020

GeekArthur Mar 25, 2020

adisky Mar 24, 2020

GeekArthur Mar 25, 2020

adisky commented Mar 24, 2020

kadel commented Mar 24, 2020

johnmcollier commented Mar 24, 2020

johnmcollier commented Mar 25, 2020

johnmcollier left a comment

GeekArthur commented Mar 26, 2020

johnmcollier left a comment

openshift-bot commented Mar 26, 2020

openshift-bot commented Mar 26, 2020

Devfile support for odo create #2699

Devfile support for odo create #2699

Conversation

GeekArthur commented Mar 10, 2020 • edited

openshift-ci-robot commented Mar 10, 2020

kadel commented Mar 11, 2020

openshift-ci-robot commented Mar 11, 2020

johnmcollier left a comment • edited

Choose a reason for hiding this comment

johnmcollier commented Mar 12, 2020 • edited

maysunfaisal left a comment

Choose a reason for hiding this comment

GeekArthur commented Mar 12, 2020 • edited

GeekArthur commented Mar 12, 2020

codecov bot commented Mar 17, 2020 • edited

Codecov Report

openshift-ci-robot commented Mar 23, 2020

adisky commented Mar 23, 2020

GeekArthur commented Mar 23, 2020

GeekArthur commented Mar 23, 2020

GeekArthur commented Mar 23, 2020

adisky Mar 24, 2020

Choose a reason for hiding this comment

GeekArthur Mar 25, 2020

Choose a reason for hiding this comment

adisky Mar 24, 2020

Choose a reason for hiding this comment

GeekArthur Mar 25, 2020

Choose a reason for hiding this comment

adisky commented Mar 24, 2020

kadel commented Mar 24, 2020

johnmcollier commented Mar 24, 2020

johnmcollier commented Mar 25, 2020

johnmcollier left a comment

Choose a reason for hiding this comment

GeekArthur commented Mar 26, 2020

johnmcollier left a comment

Choose a reason for hiding this comment

openshift-bot commented Mar 26, 2020

openshift-bot commented Mar 26, 2020

GeekArthur commented Mar 10, 2020 •

edited

johnmcollier left a comment •

edited

johnmcollier commented Mar 12, 2020 •

edited

GeekArthur commented Mar 12, 2020 •

edited

codecov bot commented Mar 17, 2020 •

edited