-
Notifications
You must be signed in to change notification settings - Fork 157
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
Errkit migration 2 (pkg/kopia) #2537
Conversation
…tdout/stderr streams.
…e.ExecWithOptions`
Output could be obtained by writers passed via `ExecOptions`
# Conflicts: # pkg/kube/pod.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should return nil
when wrapping nil
errors?
It's really easy to miss, it's missed several times in this PR.
Especially since we have things like pkg/kopia/repository/connect.go:31
92cfbdc
to
20d1867
Compare
e1c3136
to
2921b99
Compare
b1ba71b
to
4b6c62f
Compare
d5d5249
to
b18e381
Compare
4b6c62f
to
32661a3
Compare
32661a3
to
4e5ee80
Compare
4e5ee80
to
107d323
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to correct import grouping. Rest LGTM
"github.com/kanisterio/kanister/pkg/field" | ||
"github.com/kanisterio/kanister/pkg/log" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put internal packages in a separate group
pkg/kopia/repository/client.go
Outdated
"github.com/kopia/kopia/repo" | ||
"github.com/kopia/kopia/repo/content" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep kanister and other packages into separate groups
"k8s.io/client-go/kubernetes" | ||
|
||
"github.com/kanisterio/errkit" | ||
"github.com/kanisterio/kanister/pkg/kopia/command" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"k8s.io/client-go/kubernetes" | |
"github.com/kanisterio/errkit" | |
"github.com/kanisterio/kanister/pkg/kopia/command" | |
"k8s.io/client-go/kubernetes" | |
"github.com/kanisterio/errkit" | |
"github.com/kanisterio/kanister/pkg/kopia/command" |
errkit package should be considered as external package and put it into separate group
pkg/kopia/snapshot/snapshot.go
Outdated
"github.com/kanisterio/errkit" | ||
"github.com/kanisterio/kanister/pkg/kopia" | ||
"github.com/kanisterio/kanister/pkg/kopia/repository" | ||
"github.com/kopia/kopia/fs" | ||
"github.com/kopia/kopia/repo" | ||
"github.com/kopia/kopia/repo/manifest" | ||
"github.com/kopia/kopia/snapshot" | ||
"github.com/kopia/kopia/snapshot/policy" | ||
"github.com/kopia/kopia/snapshot/snapshotfs" | ||
"github.com/pkg/errors" | ||
|
||
"github.com/kanisterio/kanister/pkg/kopia" | ||
"github.com/kanisterio/kanister/pkg/kopia/repository" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github.com/kanisterio/kanister
are internal packages, so should be kept in a separate group
pkg/kopia/snapshot/stream.go
Outdated
"github.com/kanisterio/kanister/pkg/kopia" | ||
"github.com/kanisterio/kanister/pkg/kopia/repository" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
has been applied, so now our packages are properly sorted |
Change Overview
In accordance with decision to migrate to our own errors package (look here #1838), this PR contains migration of pkg/kopia to errkit package.
Further migration PRs are expected.
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan