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

Errkit migration 2 (pkg/kopia) #2537

Merged
merged 31 commits into from
Jun 25, 2024
Merged

Errkit migration 2 (pkg/kopia) #2537

merged 31 commits into from
Jun 25, 2024

Conversation

e-sumin
Copy link
Contributor

@e-sumin e-sumin commented Dec 14, 2023

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:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

Copy link
Contributor

@hairyhum hairyhum left a 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

pkg/kopia/repository/client.go Outdated Show resolved Hide resolved
pkg/kopia/repository/client.go Outdated Show resolved Hide resolved
pkg/kopia/repository/connect_or_create.go Outdated Show resolved Hide resolved
pkg/kopia/snapshot/stream.go Outdated Show resolved Hide resolved
pkg/kopia/snapshot/stream.go Outdated Show resolved Hide resolved
@e-sumin e-sumin force-pushed the errkit-migration-2 branch from 92cfbdc to 20d1867 Compare March 20, 2024 10:04
@e-sumin e-sumin force-pushed the errkit-migration-1 branch from e1c3136 to 2921b99 Compare March 20, 2024 10:35
@e-sumin e-sumin force-pushed the errkit-migration-2 branch 2 times, most recently from b1ba71b to 4b6c62f Compare March 20, 2024 16:49
@e-sumin e-sumin force-pushed the errkit-migration-1 branch from d5d5249 to b18e381 Compare March 21, 2024 11:18
@e-sumin e-sumin force-pushed the errkit-migration-2 branch from 4b6c62f to 32661a3 Compare March 21, 2024 11:38
@e-sumin e-sumin force-pushed the errkit-migration-2 branch from 32661a3 to 4e5ee80 Compare May 7, 2024 23:44
@e-sumin e-sumin force-pushed the errkit-migration-2 branch from 4e5ee80 to 107d323 Compare May 7, 2024 23:47
@e-sumin e-sumin requested a review from hairyhum May 7, 2024 23:50
Base automatically changed from errkit-migration-1 to master June 19, 2024 17:15
Copy link
Contributor

@PrasadG193 PrasadG193 left a 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

Comment on lines 27 to 28
"github.com/kanisterio/kanister/pkg/field"
"github.com/kanisterio/kanister/pkg/log"
Copy link
Contributor

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

Comment on lines 29 to 30
"github.com/kopia/kopia/repo"
"github.com/kopia/kopia/repo/content"
Copy link
Contributor

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

Comment on lines 20 to 23
"k8s.io/client-go/kubernetes"

"github.com/kanisterio/errkit"
"github.com/kanisterio/kanister/pkg/kopia/command"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"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

Comment on lines 24 to 33
"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"
Copy link
Contributor

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

Comment on lines 25 to 26
"github.com/kanisterio/kanister/pkg/kopia"
"github.com/kanisterio/kanister/pkg/kopia/repository"
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@e-sumin
Copy link
Contributor Author

e-sumin commented Jun 25, 2024

@PrasadG193

goimports -w -d -local github.com/kanisterio/kanister

has been applied, so now our packages are properly sorted

@e-sumin e-sumin added the kueue label Jun 25, 2024
@mergify mergify bot merged commit 766794e into master Jun 25, 2024
16 checks passed
@mergify mergify bot deleted the errkit-migration-2 branch June 25, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants