Log package origins #1104

ANeumann82 · 2019-11-25T12:41:05Z

What this PR does / why we need it:
Log package origins when installing
Log local paths with absolute path
Improved error messages

Fixes #1101

The original error case: User has a directory "kafka" in the cwd:

aneumanns-MBP:kudo aneumann$ go run ./cmd/kubectl-kudo/main.go install kafka --instance=kafka
kafka is a local file package
Error: failed to resolve package CRDs for operator: kafka: while parsing package files: operator package missing operator.yaml in /Users/aneumann/git/kudo/kafka
exit status 255

New output without "kafka" directory in cwd:

aneumanns-MBP:kudo aneumann$ go run ./cmd/kubectl-kudo/main.go install kafka --instance=kafka
kafka is a repository package from { name:community, url:https://kudo-repository.storage.googleapis.com }
instance.kudo.dev/v1beta1/kafka created

New output with direct URL:

aneumanns-MBP:kudo aneumann$ go run ./cmd/kubectl-kudo/main.go install https://kudo-repository.storage.googleapis.com/kafka-1.0.1.tgz --instance=kafka
https://kudo-repository.storage.googleapis.com/kafka-1.0.1.tgz is a remote tgz package
instance.kudo.dev/v1beta1/kafka created

Output with an (invalid) relative local directory

aneumanns-MBP:kudo aneumann$ pwd
/Users/aneumann/git/kudo
aneumanns-MBP:kudo aneumann$ go run ./cmd/kubectl-kudo/main.go install ../kafkay --instance=kafka
../kafkay is a local file package
Error: failed to resolve package CRDs for operator: ../kafkay: while parsing package files: unexpected file when reading package from filesystem: /Users/aneumann/git/kafkay/foo
exit status 255

Log local paths with absolute path Improved error messages

pkg/kudoctl/packages/reader/read_dir.go

Fixed integration test

gerred · 2019-11-25T14:48:00Z

@ANeumann82 reader_tar.go still has a few Wrapf usages in them. approving, but would you please fix those before you merge? :) Thanks!

More cleanup of pck/errors

alenkacz

Could you please provide an example outputs the CLI gives after this change to be able to review? Since this is changing the CLI output to the user that's the thing I am most interested about and I am kind of worried some of the %w will end up with weird output

see #1057 for inspiration :-)

pkg/kudoctl/packages/reader/read_dir.go

alenkacz · 2019-11-26T08:54:12Z

pkg/kudoctl/packages/reader/read_dir.go

@@ -26,13 +27,13 @@ func ReadDir(fs afero.Fs, path string) (*packages.Package, error) {
 	// 1. get files
 	files, err := FromDir(fs, path)
 	if err != nil {
-		return nil, errors.Wrap(err, "while parsing package files")
+		return nil, fmt.Errorf("while parsing package files: %w", err)


I don't understand why are we wrapping the error here, do we need in the upper layers distinguish between some error type? Otherwise this would end up in a pretty weird error message printed out.

btw. I would say Wrapf is more %v than %w. I think %w has a bit of different use cases. Recommend reading https://blog.golang.org/go1.13-errors including when to wrap (in case you did not so far)

Well, with regards to %v vs %w: From what I understand (and see from testing) %w just wraps the error, and allows it to be unwrappable - apart from that it behaves like %v. Which says to me, it's generally a good idea to use %w instead of %v.

In Go 1.13, the fmt.Errorf function supports a new %w verb. When this verb is present, the error returned by fmt.Errorf will have an Unwrap method returning the argument of %w, which must be an error. In all other ways, %w is identical to %v.

I just have this question in another PR. To quote that same blog post:

In other words, wrapping an error makes that error part of your API. If you don't want to commit to supporting that error as part of your API in the future, you shouldn't wrap the error. It’s important to remember that whether you wrap or not, the error text will be the same. A person trying to understand the error will have the same information either way; the choice to wrap is about whether to give programs additional information so they can make more informed decisions, or to withhold that information to preserve an abstraction layer.

We should get an agreement on this. Personally, I'm leaning towards:

Using %v by default to preserve abstractions boundaries

Using %w intentionally when exposing the underlying error type is necessary

And since, for now, we're the only users of our internal API, whenever I see %w in code, I expect somebody up in the calling chain to unwrap it. Thoughts?

+1 to what @zen-dog wrote

I do tend to lean towards: More information is better than less information, but I do get the abstraction boundaries. Coming from Java, I hate to not wrap exceptions, because of the enclosed stack traces, but as we don't have them here, I should probably adjust.

So, I can agree with @zen-dog proposal to use %v per default any only using %w when we expect to unwrap on the caller side. I don't think there needs to be an existing unwrap, but a high possibility and use case for an unwrap to happen though.

I've changed the %ws to %v. I don't think in this area we don't need to wrap any errors

ANeumann82 · 2019-11-27T09:22:38Z

@alenkacz Could you re-review?

alenkacz

Let's get an agreement on the error wrapping topic. But I think we don't need to and should not wrap an errors that are implementation details and should not be exposed.

Also what do you think about a slightly different wording? Instead of kafka is a local file package something like installing package 'kafka' from a local folder 'kafka' or something along those lines and also change the others in similar manner. Just a suggestion, I find it a bit nicer :)

pkg/kudoctl/packages/reader/read_dir.go

ANeumann82 · 2019-11-27T14:32:54Z

Also what do you think about a slightly different wording? Instead of kafka is a local file package something like installing package 'kafka' from a local folder 'kafka' or something along those lines and also change the others in similar manner. Just a suggestion, I find it a bit nicer :)

I kind of like the idea, but implementing that would be a tad difficult at the moment. The current logs are in the resolvers, which may be used by the update command (and potentially others) as well.

It would be possible to add the package type to the package.Package struct, or to extend the struct for the different package types, i'm not sure it's worth the effort right now though.

# Conflicts: # pkg/kudoctl/packages/reader/read_dir.go # pkg/kudoctl/packages/reader/reader_tar.go

alenkacz · 2019-11-27T17:45:56Z

@ANeumann82 oh I see, makes sense, thanks for the explanation. Oh now I see I am still rejecting, I was hoping I already dismissed that, sorry!

alenkacz

👍

#1101 Log package origins

32c6323

Log local paths with absolute path Improved error messages

ANeumann82 requested review from alenkacz, gerred, kensipe and zen-dog as code owners November 25, 2019 12:41

gerred requested changes Nov 25, 2019

View reviewed changes

pkg/kudoctl/packages/reader/read_dir.go Outdated Show resolved Hide resolved

ANeumann82 added 2 commits November 25, 2019 15:00

Removed usage of pck/errors

c4fdda1

Fixed integration test

Fixed import formatting

fd177b8

gerred approved these changes Nov 25, 2019

View reviewed changes

Replaced %s with %v

dacbafb

More cleanup of pck/errors

alenkacz requested changes Nov 26, 2019

View reviewed changes

alenkacz changed the title ~~Log package origins #1101~~ Log package origins Nov 26, 2019

Changed %v back to %s for strings

1e1b3ef

zen-dog mentioned this pull request Nov 27, 2019

removal of github.com/pkg/errors and commitment to go 1.13 errors #1107

Merged

alenkacz reviewed Nov 27, 2019

View reviewed changes

pkg/kudoctl/packages/reader/read_dir.go Outdated Show resolved Hide resolved

pkg/kudoctl/packages/reader/read_dir.go Outdated Show resolved Hide resolved

Changed %w to %v, don't need wrapped errors here

79e67c2

Merge branch 'master' into an/log-package-origins

9d352fa

# Conflicts: # pkg/kudoctl/packages/reader/read_dir.go # pkg/kudoctl/packages/reader/reader_tar.go

ANeumann82 requested a review from nfnt as a code owner November 27, 2019 14:35

alenkacz self-requested a review November 27, 2019 17:46

nfnt approved these changes Nov 28, 2019

View reviewed changes

alenkacz approved these changes Nov 28, 2019

View reviewed changes

ANeumann82 merged commit 7685bcd into master Nov 28, 2019

ANeumann82 deleted the an/log-package-origins branch November 28, 2019 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log package origins #1104

Log package origins #1104

ANeumann82 commented Nov 25, 2019 •

edited

Loading

gerred commented Nov 25, 2019

alenkacz left a comment

alenkacz Nov 26, 2019

ANeumann82 Nov 26, 2019 •

edited

Loading

zen-dog Nov 27, 2019 •

edited

Loading

alenkacz Nov 27, 2019

ANeumann82 Nov 27, 2019

ANeumann82 Nov 27, 2019

ANeumann82 commented Nov 27, 2019

alenkacz left a comment

ANeumann82 commented Nov 27, 2019

alenkacz commented Nov 27, 2019

alenkacz left a comment

Log package origins #1104

Log package origins #1104

Conversation

ANeumann82 commented Nov 25, 2019 • edited Loading

gerred commented Nov 25, 2019

alenkacz left a comment

Choose a reason for hiding this comment

alenkacz Nov 26, 2019

Choose a reason for hiding this comment

ANeumann82 Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

zen-dog Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

alenkacz Nov 27, 2019

Choose a reason for hiding this comment

ANeumann82 Nov 27, 2019

Choose a reason for hiding this comment

ANeumann82 Nov 27, 2019

Choose a reason for hiding this comment

ANeumann82 commented Nov 27, 2019

alenkacz left a comment

Choose a reason for hiding this comment

ANeumann82 commented Nov 27, 2019

alenkacz commented Nov 27, 2019

alenkacz left a comment

Choose a reason for hiding this comment

ANeumann82 commented Nov 25, 2019 •

edited

Loading

ANeumann82 Nov 26, 2019 •

edited

Loading

zen-dog Nov 27, 2019 •

edited

Loading