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

odo create support existing devfile.yaml #2798

Conversation

GeekArthur
Copy link
Contributor

@GeekArthur GeekArthur commented Apr 1, 2020

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 improves the odo create to make it support the following use cases:

If devfile.yaml is not present
Case 1: odo create <component type> -> odo downloads devfile from devfile registry and creates env.yaml
Case 2: odo create -> User can specify component type via interactive mode, then odo downloads devfile from devfile egistry and creates env.yaml

If devfile.yaml is present
Case 1: odo create <component type> -> odo throws errors and tells user to delete the existing devfile.yaml then run odo create again
Case 2: odo create -> odo directly uses the existing devfile.yaml and creates env.yaml, user can specify component name via interactive mode

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

How to test changes / Special notes to the reviewer:
N/A

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Apr 1, 2020
@GeekArthur
Copy link
Contributor Author

/assign

componentType = ui.SelectDevfileComponentType(supDevfileCatalogList)

// devfile.yaml is not present, user has to specify the component type
if !util.CheckPathExists(DevfilePath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move this check before listing from registry, as for the case devfile is present it might save one call.

Copy link
Contributor Author

@GeekArthur GeekArthur Apr 2, 2020

Choose a reason for hiding this comment

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

Nice suggestion! I can do that to improve the performance.

@@ -338,6 +342,10 @@ func (co *CreateOptions) Complete(name string, cmd *cobra.Command, args []string
// Component type: Get from full command's first argument (mandatory)
Copy link
Contributor

Choose a reason for hiding this comment

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

move the comments at the right place, above componentType = args[0]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I can refactor the comments for both interactive mode and direct mode

@@ -319,7 +319,11 @@ func (co *CreateOptions) Complete(name string, cmd *cobra.Command, args []string
supDevfileCatalogList = append(supDevfileCatalogList, devfileComponent)
}
}
componentType = ui.SelectDevfileComponentType(supDevfileCatalogList)

Copy link
Contributor

Choose a reason for hiding this comment

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

as [1] is the first check, if a <component-type not present and devfile.yaml is present, it is default going into interactive, IIUC it should create from existing devfile.yaml
[1]

if len(args) == 0 {
  co.interactive = true
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should create the component using the existing devfile.yaml, but user is still able to specify the <component name> via interactive mode, that's why it falls down to interactive mode

@kadel
Copy link
Member

kadel commented Apr 2, 2020

/retest

@kadel
Copy link
Member

kadel commented Apr 2, 2020

odo create should not create a devfile.yaml if there is already "old style" odo component.

In the following scenario, the second odo create should fail with the message that the current directory already contains a component

▶ export ODO_EXPERIMENTAL=false

▶ odo create nodejs
 ✓  Validating component [389ms]

Please use `odo push` command to create the component with source deployed

▶ export ODO_EXPERIMENTAL=true

▶ odo create nodejs
 ✓  Checking if the specified component type is supported devfile component type [122381ns]
 ✓  Validating component [85245ns]

Please use `odo push` command to create the component with source deployed


```

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

codecov bot commented Apr 2, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2798      +/-   ##
==========================================
+ Coverage   43.70%   43.81%   +0.10%     
==========================================
  Files         102      102              
  Lines        9160     9160              
==========================================
+ Hits         4003     4013      +10     
+ Misses       4780     4774       -6     
+ Partials      377      373       -4     
Impacted Files Coverage Δ
pkg/watch/watch.go 69.41% <0.00%> (+5.88%) ⬆️

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 3f49286...d1699f0. Read the comment docs.

@GeekArthur
Copy link
Contributor Author

@kadel @adisky Comments addressed, please review again, thanks!

Copy link
Contributor

@amitkrout amitkrout left a comment

Choose a reason for hiding this comment

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

@GeekArthur Thanks for the pr.

We can add Integration test to validate the changes. Scenarios are pretty clear from your pr section of What does does this PR do / why we need it:.

BTW we have an open issue #2194 for interactive mode test script implementation. So you can skip the interactive mode integration test atm.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 5, 2020
@kadel
Copy link
Member

kadel commented Apr 6, 2020

/approve

@GeekArthur it needs rebase

@openshift-ci-robot
Copy link
Collaborator

[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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 6, 2020
…ateImprovement

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 6, 2020
Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
@GeekArthur
Copy link
Contributor Author

@kadel @amitkrout comments addressed, please review again, thanks!

@kadel
Copy link
Member

kadel commented Apr 7, 2020

/retest

…ateImprovement

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
@GeekArthur
Copy link
Contributor Author

/retest

@openshift-bot
Copy link

/retest

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

2 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.

…ateImprovement

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 15, 2020
Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
@GeekArthur
Copy link
Contributor Author

/retest

Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

Adding back the lgtm label since it was removed with the rebase earlier

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. Required by Prow. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Apr 16, 2020
…ateImprovement

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. Required by Prow. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Apr 16, 2020
@maysunfaisal
Copy link
Contributor

Re-adding the lgtm after rebase
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 16, 2020
@GeekArthur
Copy link
Contributor Author

/retest

2 similar comments
@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

Signed-off-by: jingfu wang <jingfu.j.wang@ibm.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 16, 2020
@maysunfaisal
Copy link
Contributor

re-add lgtm after rebase
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 16, 2020
@GeekArthur
Copy link
Contributor Author

/retest

3 similar comments
@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@GeekArthur
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit dc7d168 into redhat-developer:master Apr 17, 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. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation 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.

odo create support existing devfile.yaml
10 participants