Odo: Clean up error wrapping

[maysunfaisal]

• Looks like with moving up the go version, error wrapping functionality has changed a bit. For example, you can't return errors.Wrap(err, "unable to parse devfile") from odo. It has to be fmt.Errorf("unable to parse the devfile %s, with error: %s", o.DevfilePath, err). Otherwise you may lose all the error context wrapping from the child funcs.

• I have also noticed that sometimes we have way too much wrapping. Is the following error wrapping really necessary?

Maysuns-MacBook-Pro:nodejs-ex maysun$ odo push

Validation
 ✗  Validating the devfile [358336ns]
 ✗  Failed to start component with name mjfnode. Error: Failed to create the component: failed to validate devfile build and run commands: 
there should be exactly one default command for command group build, currently there is more than one default command

• I have also noticed that the error wrapping has diff messages for the same func call across the odo code, we should stay consistent with the messaging. Its best to have an error.go file with all the custom errors so that error msgs are consistent across the board :) For example, https://github.com/openshift/odo/blob/master/pkg/devfile/validate/errors.go

/kind cleanup
/area devfile

[johnmcollier]

FWIW, I posted a similar message on slack here: https://openshiftdo.slack.com/archives/CQK6HE78S/p1599268565141800, as did Jingfu https://openshiftdo.slack.com/archives/CQK6HE78S/p1588964792403000. A lot of our error wrapping is being ignored because util.LogErrorAndExit only returns the cause of the error, rather than the error itself. We can work around that by returning a new error with fmt.Errorf like you showed, rather than using error.Wrap

So from some past conversations, we _do_ want to limit the amount of error wrapping that we do, to avoid monoliths like the one that you posted (or even bigger error messages), but there are cases where it is useful still.

+1 on doing a general clean up of our error wrapping, most of it's not being printed at all, and I think a lot isn't even necessary. For the error wrapping that we determine useful, we should convert to use fmt.Errorf instead of error.Wrap

[maysunfaisal]

looks like we both encountered it while working with devfile parsing

[adisky]

agreed with both the concerns, at some places it is redundant and at some places nothing, logged similar kind of issue long back
https://github.com/openshift/odo/issues/2709

[elsony]

Given the scope of the change, this item is going to be a post v2 GA item to avoid introducing potential instability to the GA code.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

[openshift-bot]

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

[openshift-ci-robot]

@openshift-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.