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

cmd, pkg, schema: stop using github.com/pkg/errors. #99

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

klihub
Copy link
Contributor

@klihub klihub commented Nov 11, 2022

Get rid of github.com/pkg/errors dependency.

@klihub klihub force-pushed the fixes/deps/github-pkg-errors branch 2 times, most recently from 3e9de95 to 1503b67 Compare November 11, 2022 14:18
@klihub klihub changed the title cmd, pkg, schema: stop using github.com/pkg/errors. go.mod: bump golang to 1.19, stop using github.com/pkg/errors. Nov 11, 2022
@klihub klihub force-pushed the fixes/deps/github-pkg-errors branch 2 times, most recently from a5b10d5 to e86a265 Compare November 11, 2022 14:44
@klihub klihub requested review from elezar and bart0sh November 11, 2022 14:47
Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

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

One question about the Golang version bump.

go.mod Outdated Show resolved Hide resolved
@@ -17,9 +17,9 @@
package cdi

import (
"errors"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use fmt.Errorf over errors.New for all errors even if no formatting is performed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would there be some real benefit to it ? This errors is golang stdlib (so always present/available by the toolchain) and we anyway depend on it because we use errors.Is() elsewhere in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was more of a general question. Not a blocker.

Copy link

Choose a reason for hiding this comment

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

a related comment is to make the cdi errors hierarchy so that, e.g., goerr113 linter is happy. In particular,
"error should not be created dynamically from scratch but by the wrapping the static (package-level) error."

$ golangci-lint run --disable-all -E goerr113

Copy link
Contributor

Choose a reason for hiding this comment

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

@mythi I may have been a bit hasty in resolving this thread. I have created #102 to address the lint errors. I think that this is out of scope of this PR and also doesn't make the situation worse than what it currently is.

Copy link

Choose a reason for hiding this comment

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

@elezar no worries, I think it's fine like this and we can work on the improvements through #102

@klihub klihub force-pushed the fixes/deps/github-pkg-errors branch from e86a265 to ab4d52f Compare November 11, 2022 19:56
@klihub klihub changed the title go.mod: bump golang to 1.19, stop using github.com/pkg/errors. go.mod: bump golang to 1.18, stop using github.com/pkg/errors. Nov 11, 2022
Get rid of github.com/pkg/errors dependency.

Signed-off-by: Krisztian Litkey <[email protected]>
go.mod Outdated Show resolved Hide resolved
@klihub klihub force-pushed the fixes/deps/github-pkg-errors branch from 2ce182d to dd57a82 Compare November 17, 2022 17:55
@klihub klihub changed the title go.mod: bump golang to 1.18, stop using github.com/pkg/errors. cmd, pkg, schema: stop using github.com/pkg/errors. Nov 17, 2022
@klihub klihub requested review from elezar and mythi November 17, 2022 17:56
Copy link
Contributor

@bart0sh bart0sh left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

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

Thanks @klihub. After dropping the golang version bump this looks good.

@elezar elezar merged commit e93a674 into cncf-tags:main Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants