validator should not return an error if no default command, should return warning instead

[yangcao77]

/kind bug

Which area this bug is related to?

/area api
/area library

What versions of software are you using?

Operating System:

Go Pkg Version:

Bug Summary

Describe the bug:

If devfile has no default command for each group kind, the validator error out.

To Reproduce:

use the validator on this devfile:https://raw.githubusercontent.com/yangcao77/web-coolstore/completed-sample/devfile.yaml

Expected behavior

since the devfile may contain multiple projects, it may not have default commands. The validation rule says if no default command defined, a warning will be displayed. but the current validator returns an error.

{build, run, test, debug}, each kind of group can only have one default command associated with it. If there are multiple commands of the same kind without a default, a warning will be displayed.

Any logs, error output, screenshots etc? Provide the devfile that sees this bug, if applicable.

run parser from devfile/library on https://raw.githubusercontent.com/yangcao77/web-coolstore/completed-sample/devfile.yaml

parsing devfile from ./devfile.yaml

command group build error - there should be exactly one default command, currently there is no default command
command group run error - there should be exactly one default command, currently there is no default command

Additional context

Any workaround?

Suggestion on how to fix the bug

[yangcao77]

Three possible approaches, and I personally prefer the 3rd one:

1. devfile/api ValidateCommands still returns (err error), but the warning message will start with a "WARNING" prefix. This approach will be harder to split warnings and errors, if the return value has all errors and warnings appended. For example:

WARNING: command group build warning - there should be exactly one default command, currently there is no default command
command group run error - there should be exactly one default command, currently there is more than one default command; command: cmd1; command: cmd2

2. devfile/api ValidateCommands returns (warning string, err error). And devfile/library parser returns an extra warning field.
   tools will need to handle the extra returned field.

3. create a new struct, devfile/api ValidateCommands and devfile/library parser return this struct instead. This requires tools to handle the new return type.

{
error err
warning string
}

[yangcao77]

Hi @kadel, we are going to update devfile validator to return warnings, thus parser is also going to make the same change. I listed 3 feasible approaches in the above comment. Since Odo is now consuming the validator & parser, I would like to hear from your advice; which one would help Odo adopt the new change more easier?

[kadel]

The situation when there is no default run command is problematic for odo.

If there is no default run command odo can't know what to execute, and being CLI tool it can't easily ask users.
Odo's users will have to specify which command to run using odo push --run-command.

Odo will have to still treat this as an error.

Other approach could be to let ValidateCommands return errors like it does now. And have extra error types for each type of validation check, so tools can decide they want to treat each failed validation. Similarly, how Go's os standard library does it, where you can do os.IsNotExist(err) to check if the error was of a "Not Exist type".

For example the library could have a validation.IsMisingDefault(err) function to detect what validation "failed".

EDIT:
I just realized that in this case ValidateCommands should return []error containing all failed validation.

[yangcao77]

Other approach could be to let ValidateCommands return errors like it does now. And have extra error types for each type of validation check, so tools can decide they want to treat each failed validation. Similarly, how Go's os standard library does it, where you can do os.IsNotExist(err) to check if the error was of a "Not Exist type".

For example the library could have a validation.IsMisingDefault(err) function to detect what validation "failed".

EDIT:
I just realized that in this case ValidateCommands should return []error containing all failed validation.

That would also work.

So to summarize, validator and parser will return []error instead of error. And we will create error types for specific validation errors. We are not going to create extra function to help identify the underlying error type. tools can use err.(type) to check for specific error type, e.g. in this case err.(type) == *validation.MisingDefaultCmdWarning

[yangcao77]

The situation when there is no default run command is problematic for odo.

If there is no default run command odo can't know what to execute, and being CLI tool it can't easily ask users.
Odo's users will have to specify which command to run using odo push --run-command.

Odo will have to still treat this as an error.

I thought this is a warning in Odo? Looks like if Odo has no default command found, it will just run the first command defined under this groupkind, and won't block the push.
https://github.com/openshift/odo/blob/57976875f2b1a61d5dd7bd027379abcedc8df943/pkg/devfile/adapters/common/command.go#L82-L85
@kadel Can you confirm?

When we were creating the validation rule, we also considered Odo's behavior. We planned to return warning to tools, and let tools to decide if it is an error or if needs to display more tool specific information for this warning.
So for Odo, we are expecting if Odo sees this MisingDefaultCmdWarning returned, should display the received warning msg and plus Odo specific warning message. For example:

command group build warning - there should be exactly one default command, currently there is no default command
Odo will use the first command defined for this group kind as the default command

[kadel]

I thought this is a warning in Odo? Looks like if Odo has no default command found, it will just run the first command defined under this groupkind, and won't block the push.
https://github.com/openshift/odo/blob/57976875f2b1a61d5dd7bd027379abcedc8df943/pkg/devfile/adapters/common/command.go#L82-L85
@kadel Can you confirm?

Yes, currently it does this. But it can be confusing for users. If odo can't safely determine correct run command it should error out.
We can't just pick one o the commands.
The only exception that I can see is when there is only one command of a given kind. Then it makes sense to use is even if it is not marked as a default one. But if there is a multiple command in a group and no command is default command it should error out

[yangcao77]

The change merged into api repo uses multierror.Append to construct the error list, and the returned type is still error.
tools like odo calls parser and validator won't get affected.

if want to extract for individual error, can use

merr, ok := err.(*multierror.Error)
// merr.Errors is the []error list

for more information on multierror usage, refer to : https://github.com/hashicorp/go-multierror