Devfile v2 command group impl + Odo catalog list components #3291

maysunfaisal · 2020-06-03T15:04:18Z

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:

Implements complete feature of devfile v2 spec for command groups cdc9989
Refactored and cleaned up code around devfile parser and validate to be consistent across odo commands a82ca62
Update odo catalog to list components from new default devfile v2 registry 13d67fc
Update odo catalog list components to only read index.json entries for name instead of parsing the entire devfile. We also no longer show the SUPPORTED column for devfile catalog, see discussion at Improve performance for odo catalog list components . #3249 (comment) 065f19b 9da66e9
Unit tests 065f19b
Fix odo catalog & odo create bugs 66deec8
integration tests 6d2a0aa

Which issue(s) this PR fixes:

Fixes #2938 #3271 #3249 #3280 #3436

How to test changes / Special notes to the reviewer:

odo catalog list components (now lists new v2 devfiles)

$ odo catalog list components
Odo OpenShift Components:
NAME              PROJECT       TAGS                        SUPPORTED
java              openshift     11,8,latest                 YES
nodejs            openshift     10-SCL,8,8-RHOAR,latest     YES
dotnet            openshift     2.1,2.2,3.0,latest          NO
golang            openshift     1.11.5,latest               NO
httpd             
Odo Devfile Components:
NAME            DESCRIPTION                            REGISTRY
maven           Upstream Maven and OpenJDK 11          DefaultDevfileRegistry
nodejs          Stack with NodeJS 10                   DefaultDevfileRegistry
openLiberty     Open Liberty microservice in Java      DefaultDevfileRegistry
quarkus         Upstream Quarkus with Java+GraalVM     DefaultDevfileRegistry
springBoot      Spring Boot® using Java                DefaultDevfileRegistry

odo create springBoot mondayspringboot
odo url create
odo push

To test out the various push scenarios:

commands with multiple default will error out
commands with multiple default but with odo push --build-command --run-command should succeed
push without a run group should error out
push with flags using the wrong group should error out like odo push --build-command="a run command of group run"

openshift-ci-robot · 2020-06-03T15:04:34Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2020-06-08T17:14:00Z

Codecov Report

Merging #3291 into master will increase coverage by 0.10%.
The diff coverage is 71.73%.

@@            Coverage Diff             @@
##           master    #3291      +/-   ##
==========================================
+ Coverage   46.39%   46.49%   +0.10%     
==========================================
  Files         112      112              
  Lines       11233    11227       -6     
==========================================
+ Hits         5211     5220       +9     
+ Misses       5517     5506      -11     
+ Partials      505      501       -4

Impacted Files	Coverage Δ
pkg/devfile/parser/parse.go	`0.00% <0.00%> (ø)`
pkg/preference/preference.go	`61.87% <ø> (-0.63%)`	⬇️
pkg/testingutil/devfile.go	`0.00% <ø> (ø)`
pkg/catalog/catalog.go	`55.37% <84.61%> (+3.10%)`	⬆️
pkg/devfile/adapters/common/command.go	`95.34% <94.23%> (+1.77%)`	⬆️
pkg/devfile/adapters/common/utils.go	`59.61% <100.00%> (+5.26%)`	⬆️
pkg/sync/sync.go	`47.52% <0.00%> (-1.00%)`	⬇️

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 edc7c33...5ec8274. Read the comment docs.

maysunfaisal · 2020-06-10T15:26:44Z

This PR is pretty much ready except integration tests that needs to be converted to v2, which is being already covered by #3279

So until #3279 is merged, this will probably be blocked. Otherwise we will be stepping on each other toes.

maysunfaisal · 2020-06-15T20:05:40Z

👋

@adisky could you pls help review the command group code?

@GeekArthur could you pls help review the catalog and create code?

thx!

adisky · 2020-06-16T10:30:02Z

pkg/devfile/adapters/common/command.go

+				return command, validateCommand(data, command)
+			} else if reflect.DeepEqual(firstMatchedCommand, common.DevfileCommand{}) {
+				// store the first matched command in case there is no default command
+				firstMatchedCommand = command


should we rename it to onlyCommand or singleCommand? IIUC we are throwing error if more than one command of a grouptype is present and none of them is default and if only one command is present without any default value we are allowing it. So it should be that one command

yeah we can rename it.

Actually there is a case where multiple defaults can be present ie when we use odo push --build-command="mybuild1". mybuild1 and mybuild2 can be default or have no defaults at all and this is fine since we're explicitly using a flag and saying what command to run

but that case we are already covering above if commandName != "" , IIUC that case would not fallback here, also can we update the comment // store the first matched command in case there is no default command to return the only remaining command for the group

Not necessary to do in this PR, but should we separate the flag command and devfile command function for better code readability?
getCommandFromFlags
getCommandfromDevfile
and in the GetRunCommand, GetBuildCommand functions includes the check for command name?
IMO this one(what i had written also) is difficult to understand.

but that case we are already covering above if commandName != "" , IIUC that case would not fallback here, also can we update the comment // store the first matched command in case there is no default command to return the only remaining command for the group

you're right, i have updated the comment to

// return the only remaining command for the group if there is no default command // NOTE: we return outside the for loop since the next iteration can have a default command

Not necessary to do in this PR, but should we separate the flag command and devfile command function for better code readability?

yeah i think it makes sense to have different funcs and separate out their logic for better readability. I think anyone else reading it will be confused in the first few attempts.

I've updated the getCommand() func. Broken down into getCommandFromFlag() and getCommandfromDevfile() and moved their specific logic to these funcs.

getCommand() does the check for commandName and splits into either of the above two function calls. This way we can just use getCommand() from anywhere instead of doing this check everywhere when we need a command.

@maysunfaisal Thanks for doing this, now it is much easier to understand :)

adisky · 2020-06-16T10:31:18Z

pkg/devfile/adapters/common/command.go

 	if commandName == "" {
 		// if default command is not found return the first command found for the matching type.
-		for _, command := range commands {
-
-			if command.Exec.Group != nil && command.Exec.Group.Kind == groupType {
-				supportedCommand = command
-				return supportedCommand, nil
-			}
+		if !reflect.DeepEqual(firstMatchedCommand, common.DevfileCommand{}) {
+			return firstMatchedCommand, validateCommand(data, firstMatchedCommand)


IMO this is not required now, this block could be removed and we can return from above else if block

We need this. Prior to this PR, we used two for loops to get the default command and the first command of group type.

But now we're checking for both in the same for loop..

For example if a devfile has two build commands in order

devbuild1

devbuild2 {default:true}

if we removed the if condition and returned in else if, it will incorrectly return devbuild1 even tho in the next iteration there is a default command.

But with this if block it solves the two cases ie multiple commands of same group in any order and also one command of a group kind.

adisky · 2020-06-16T10:35:08Z

pkg/devfile/adapters/common/utils.go

@@ -109,6 +109,18 @@ func GetSupportedComponents(data data.DevfileData) []common.DevfileComponent {
 	return components
 }

+func getCommandsForGroup(data data.DevfileData, groupType common.DevfileCommandGroupType) []common.DevfileCommand {
+	var commands []common.DevfileCommand


nit: s/getCommandsForGroup/getCommandsByGroup

adisky · 2020-06-16T10:36:20Z

pkg/devfile/parser/data/2.0.0/components.go

+	var commands []common.DevfileCommand
+
+	for _, command := range d.Commands {
+		command.Exec.Id = strings.ToLower(command.Exec.Id)


why we need this? Please add comment

adisky · 2020-06-16T10:57:58Z

pkg/devfile/adapters/common/command.go

+
+	if defaultCommandCount != 1 {
+		return fmt.Errorf("there should be one default command for command group %v", groupType)
+	}


there should be only one default command for command group

there should be one default command for command group signifying that no default command is present.

actually this condition checks for both missing and multiple defaults if there is more than one command of same group.

I'll update to there should be at most one default command for command group

IMO, we could return different error for defaultCommandCount=0 & defaultCommandCount >1
or
there should be exactly one default command for command group

Yeah makes sense, updated the err statements based on the default command count

adisky · 2020-06-16T11:10:26Z

@maysunfaisal Overall looks good to me, tested for command scenarios working well.
Just left some nit's.

GeekArthur · 2020-06-16T22:28:20Z

pkg/catalog/catalog.go

-	// 3. Add devfile component types to the catalog devfile list
-	retrieveDevfiles := util.NewConcurrentTasks(len(registryIndices))
-	devfileMutex := &sync.Mutex{}
+	// Add stacks to the catalog devfile list
 	for _, index := range registryIndices {


If we remove the code block of downloading the devfile for getting information of devfile support criterias, we don't need this for loop either. DevfileComponentType population step can be done inside function getDevfileIndexEntries when we download the index file. With this for loop removal, we can increase the performance a lot as we don't need to iterate through the all index entries again.

good catch, updated

GeekArthur · 2020-06-16T22:34:20Z

pkg/odo/cli/component/create.go

 		var componentType string
 		var componentName string
 		var componentNamespace string
+		isDevfileRegistryPresent := true // defaulted to true since odo ships with a default registry set
+
+		// Component type: We provide devfile component list to let user choose


We can't make the function catalog.ListDevfileComponents global because we have the case where creating devfile component from existing devfile, that case doesn't need this function so that make this function global can reduce the performance. This function only use by creating devfile component from registry (either interactive or direct mode)

GeekArthur · 2020-06-16T22:39:42Z

pkg/odo/cli/component/create.go

-		co.devfileMetadata.componentType = componentType
-		co.devfileMetadata.componentName = strings.ToLower(componentName)
-		co.devfileMetadata.componentNamespace = strings.ToLower(componentNamespace)
+		if isDevfileRegistryPresent {


This isDevfileRegistryPresent flag should only apply to the case where creating devfile component from registry, which means if the registry list is empty user still can use the existing devfile to create devfile component. So isDevfileRegistryPresent flag should be used here https://github.com/openshift/odo/pull/3291/files#diff-58fe314b4e3f9120402717ab8c2b3049L512 instead of the whole code block.

actually it doesnt matter because isDevfileRegistryPresent is true by default but i moved it because local devfile is not devfile registry. So later when ppl comes to read the code they dont get confused,

But I had to move the header Validation to inside the if blocks to keep interactive mode clean.

GeekArthur · 2020-06-16T22:56:24Z

The PR looks good, request some changes for performance improvement as odo catalog and odo create get involve in the http request/response (network is always an important factor that we should consider for performance).

Also leave couple of comments below:

Regarding odo catalog and odo create, do we consider backwards compatibility? If yes we should modify this PR as this PR breaks the odo catalog list components support for devfile 1.0 registry. If no we should provide migration instruction/step because users may hit the issue where the new odo release doesn't work on their devfile 1.0 registry.
If we decide to remove the preliminary check for devfile support criteria (devfile has run command and build command etc), IMO we should add the check somewhere back before odo push in the same PR, otherwise the simple devfile incompatibility error will out until user run odo push, that's not a good UX I think.

GeekArthur · 2020-06-16T22:58:12Z

pkg/catalog/catalog.go

-	// 3. Add devfile component types to the catalog devfile list
-	retrieveDevfiles := util.NewConcurrentTasks(len(registryIndices))
-	devfileMutex := &sync.Mutex{}
+	// Add stacks to the catalog devfile list
 	for _, index := range registryIndices {
 		// Load the devfile


Nit: we can remove this comment as we don't download/load the devfile.

kadel · 2020-06-17T13:06:45Z

Regarding odo catalog and odo create, do we consider backwards compatibility? If yes we should modify this PR as this PR breaks the odo catalog list components support for devfile 1.0 registry. If no we should provide migration instruction/step because users may hit the issue where the new odo release doesn't work on their devfile 1.0 registry.

I don't think that we need to deal with backward compatibility in this case. All this is still done in the experimental mode. The main reason to introduce experimental mode was to allow us to experiment and break thinks if needed without dealing with backward compatibility issues.
If we think that someone already has its own devfile v1 registries we can document migration steps, but I wouldn't do more than that. We will drop whole devfile v1 support before devfile goes out of experimental anyway.

2. If we decide to remove the preliminary check for devfile support criteria (devfile has run command and build command etc), IMO we should add the check somewhere back before odo push in the same PR, otherwise, the simple devfile incompatibility error will out until user run odo push, that's not a good UX I think.

I don't think that we need to check for support criteria before odo push.
With this PR we are just using our own devfile registry that is under our full control so we can make sure that all devfiles in our registry are valid.
If someone adds their own registry it is their responsibility.

Erroring out in odo push is ok. In some cases, user will be able to resolve the issues by just providing correct flags to odo push command.

/approve

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

GeekArthur · 2020-06-26T18:06:21Z

Add lgtm back after rebase
/lgtm

openshift-bot · 2020-06-26T19:44:54Z

/retest

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

maysunfaisal · 2020-06-26T21:49:36Z

/retest

johnmcollier · 2020-06-26T21:55:45Z

@maysunfaisal Trying out your branch, it seems interactive mode is broken (but with a different issue than #3436):

maysunfaisal · 2020-06-26T21:57:33Z

@johnmcollier you need to delete ~/.odo, it is still pointing to v1 devfiles. v2 registry devfile index has changed and has the name and hence i think its showing empty names.

johnmcollier · 2020-06-26T21:58:55Z

@maysunfaisal Ah, good catch. Yeah, that fixed things.

We should maybe send a note out on #odo-dev then, if ~/.odo needs to be deleted

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Jun 3, 2020

openshift-ci-robot requested review from cdrage and GeekArthur June 3, 2020 15:04

maysunfaisal mentioned this pull request Jun 4, 2020

Improve performance for odo catalog list components . #3249

Closed

maysunfaisal mentioned this pull request Jun 8, 2020

Converts v1 devfiles to v2 #3279

Merged

maysunfaisal changed the title ~~Devfile v2 Command Group Impl~~ Devfile v2 command group impl + Odo catalog list components Jun 8, 2020

maysunfaisal force-pushed the 2938-1 branch from 065f19b to 9da66e9 Compare June 9, 2020 20:35

maysunfaisal marked this pull request as ready for review June 10, 2020 15:46

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 10, 2020

maysunfaisal mentioned this pull request Jun 12, 2020

"odo catalog list components --all" displays components without displaying name #3340

Closed

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

maysunfaisal force-pushed the 2938-1 branch from 9da66e9 to 51b7199 Compare June 12, 2020 20:46

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

maysunfaisal force-pushed the 2938-1 branch from 51b7199 to 6d2a0aa Compare June 15, 2020 19:57

adisky reviewed Jun 16, 2020

View reviewed changes

maysunfaisal force-pushed the 2938-1 branch 2 times, most recently from f193ab0 to 2653ae1 Compare June 16, 2020 19:05

GeekArthur suggested changes Jun 16, 2020

View reviewed changes

openshift-ci-robot assigned GeekArthur Jun 16, 2020

GeekArthur suggested changes Jun 16, 2020

View reviewed changes

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

maysunfaisal added 12 commits June 26, 2020 11:03

Cleanup, Unit tests, update catalog list components

b09750a

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

Remove SUPPORTED for devfile catalog

32900af

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

Update integration tests for devfile v2 - 1

c29a5da

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

Fix odo catalog list and odo create

a3fbfbf

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

Update devfile integration tests

dd240db

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

Update devfile tests for Kube cluster

90c87df

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

PR Feedback for command group

34643b0

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

Apply Feedback 2 for commands group

333a06b

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

Catalog & Create PR Feedback 1

fa4252b

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

Create and Catalog Feedback 2

2aa0d2a

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

rebase with master

471ab6a

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

Update Registry integration tests for v2

8074790

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

maysunfaisal mentioned this pull request Jun 26, 2020

odo watch does not tell me the err on devfile validation failure #3435

Closed

rebase with master and update test

5ec8274

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

maysunfaisal force-pushed the 2938-1 branch from 3f9176b to 5ec8274 Compare June 26, 2020 17:17

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

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

johnmcollier mentioned this pull request Jun 26, 2020

Fix odo create interactive mode #3437

Closed

openshift-merge-robot merged commit 9a47062 into redhat-developer:master Jun 27, 2020

This was referenced Jun 29, 2020

Throw Error, If multiple commands of same kind specified in devfile v2 and none is default #3271

Closed

Wrong error formatting in devfile #3280

Closed

odo create interactive mode is broken for devfiles #3436

Closed

mik-dass mentioned this pull request Jul 1, 2020

Fixes TestDownloadFile for go 1.14.2 #3434

Closed

girishramnani mentioned this pull request Jul 3, 2020

make test failing on 1.14 #3433

Closed

Devfile v2 command group impl + Odo catalog list components #3291

Devfile v2 command group impl + Odo catalog list components #3291

Conversation

maysunfaisal commented Jun 3, 2020 • edited by johnmcollier Loading

openshift-ci-robot commented Jun 3, 2020

codecov bot commented Jun 8, 2020 • edited Loading

Codecov Report

maysunfaisal commented Jun 10, 2020 • edited Loading

maysunfaisal commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisky Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisky commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeekArthur commented Jun 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Jun 17, 2020 • edited Loading

GeekArthur commented Jun 26, 2020

openshift-bot commented Jun 26, 2020

maysunfaisal commented Jun 26, 2020

johnmcollier commented Jun 26, 2020

maysunfaisal commented Jun 26, 2020

johnmcollier commented Jun 26, 2020

maysunfaisal commented Jun 3, 2020 •

edited by johnmcollier

Loading

codecov bot commented Jun 8, 2020 •

edited

Loading

maysunfaisal commented Jun 10, 2020 •

edited

Loading

adisky Jun 17, 2020 •

edited

Loading

maysunfaisal Jun 17, 2020 •

edited

Loading

maysunfaisal Jun 16, 2020 •

edited

Loading

maysunfaisal Jun 16, 2020 •

edited

Loading

kadel commented Jun 17, 2020 •

edited

Loading