reformat with goimports -local by samuelkarp · Pull Request #7533 · containerd/containerd

samuelkarp · 2022-10-14T18:40:24Z

The -local flag allows us to separate imports that originate from github.com/containerd/containerd from other imports, making it more clear where various imports come from.

Files can be formatted using the following command:

goimports -w -local github.com/containerd/containerd

To reformat all Go source files (excluding vendor and .pb.go):

goimports -w -local github.com/containerd/containerd \
  $(find . -type f -name '*.go' -not -path "./vendor/*" -not -path "*.pb.go")

The -local flag allows us to separate imports that originate from github.com/containerd/containerd from other imports, making it more clear where various imports come from. Files can be formatted using the following command: goimports -w -local github.com/containerd/containerd To reformat all Go source files (excluding vendor and .pb.go): goimports -w -local github.com/containerd/containerd \ $(find . -type f -name '*.go' -not -path "./vendor/*" -not -path "*.pb.go") Signed-off-by: Samuel Karp <[email protected]>

Signed-off-by: Samuel Karp <[email protected]>

thaJeztah

Left comments inline

Some thoughts in general;

This will be disruptive for some time
- cause quite some merge conflicts on open PRs
- switching between branches (main <--> 1.6 LTS) will require switching tooling settings to use the expected convention
The linter apparently doesn't do a great job on checking these, and doesn't enforce "3 groups (stdlib, external, local)"
- Perhaps the gci linter does handle this? (haven't checked) https://golangci-lint.run/usage/linters/
If we want this format to be enforced, perhaps we need a make fmt target, otherwise contributors must type goimports -w --local github.com/containerd/containerd . (which is a lot to type)

I'm also wondering (if we go ahead with this) would it make sense to change "local" to github.com/containerd/ (all imports from within the containerd org)?

Packages may move around within the org, and perhaps we want to make that less disruptive
How should separate modules be treated? e.g. if github.com/containerd/containerd/api becomes its own module, is it still "local"? It's in the same repo, but effectively "non-local"

thaJeztah · 2022-10-15T10:27:08Z

cmd/containerd-stress/cri_worker.go

@@ -25,9 +25,10 @@ import (

 	"github.com/sirupsen/logrus"


Looks like this one needs to be together with the one below

thaJeztah · 2022-10-15T10:32:40Z

integration/remote/remote_image.go

 	"k8s.io/klog/v2"

-	internalapi "github.com/containerd/containerd/integration/cri-api/pkg/apis"
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1"


These should go together

thaJeztah · 2022-10-15T10:32:52Z

integration/remote/remote_image.go

+	internalapi "github.com/containerd/containerd/integration/cri-api/pkg/apis"
+
 	"github.com/containerd/containerd/integration/remote/util"


These should go together

thaJeztah · 2022-10-15T10:33:18Z

integration/remote/remote_runtime.go

 	"k8s.io/klog/v2"

-	internalapi "github.com/containerd/containerd/integration/cri-api/pkg/apis"
 	"k8s.io/component-base/logs/logreduction"


These should to together

thaJeztah · 2022-10-15T10:33:26Z

integration/remote/remote_runtime.go

+	internalapi "github.com/containerd/containerd/integration/cri-api/pkg/apis"
+
 	"github.com/containerd/containerd/integration/remote/util"


These should to together

thaJeztah · 2022-10-15T10:56:48Z

pkg/cri/server/snapshots.go


 	snapshotstore "github.com/containerd/containerd/pkg/cri/store/snapshot"
 	ctrdutil "github.com/containerd/containerd/pkg/cri/util"


Move these together with the containerd group above

thaJeztah · 2022-10-15T10:57:08Z

pkg/cri/server/version.go

+	"github.com/containerd/containerd/version"
+
 	"github.com/containerd/containerd/pkg/cri/constants"


Move these together

thaJeztah · 2022-10-15T10:57:55Z

pkg/process/exec.go

 	"golang.org/x/sys/unix"

 	"github.com/containerd/console"


Move these together

thaJeztah · 2022-10-15T11:00:05Z

runtime/v2/runc/pause/sandbox.go

+	"github.com/containerd/containerd/pkg/shutdown"
+
 	api "github.com/containerd/containerd/api/runtime/sandbox/v1"


Move these together

thaJeztah · 2022-10-15T11:02:37Z

services/sandbox/store_local.go

+	"github.com/containerd/containerd/services"
+
 	api "github.com/containerd/containerd/api/services/sandbox/v1"


Move these together

thaJeztah · 2022-10-15T11:20:36Z

ARF, GITHUB! Looks like you need to "expand" to see all review comments (they should really fix this)

samuelkarp · 2022-10-16T07:50:11Z

Thanks for the thorough review @thaJeztah!

This will be disruptive for some time

cause quite some merge conflicts on open PRs

Yep, I want to see what everyone else thinks about this. It'll definitely be disruptive, but I think it makes the imports clearer (and it'll reduce the chance of me/my IDE changing them in other unrelated PRs 😆).

switching between branches (main <--> 1.6 LTS) will require switching tooling settings to use the expected convention

If we're onboard with this change, I think we can do the same thing on the 1.5 and 1.6 branches as well.

The linter apparently doesn't do a great job on checking these, and doesn't enforce "3 groups (stdlib, external, local)"

Perhaps the gci linter does handle this? (haven't checked) golangci-lint.run/usage/linters

My preference would be to do this with goimports since it's more of a standard tool, but I'm open to other opinions.

If we want this format to be enforced, perhaps we need a make fmt target, otherwise contributors must type goimports -w --local github.com/containerd/containerd . (which is a lot to type)

I can do this.

I'm also wondering (if we go ahead with this) would it make sense to change "local" to github.com/containerd/ (all imports from within the containerd org)?

Packages may move around within the org, and perhaps we want to make that less disruptive

How should separate modules be treated? e.g. if github.com/containerd/containerd/api becomes its own module, is it still "local"? It's in the same repo, but effectively "non-local"

I thought about these too and landed on github.com/containerd/containerd as the grouping because I think the same-repository indication was the most meaningful to me; I can see that changes within the same repo are coordinated while cross-repo changes require additional updates.

github-actions · 2024-03-28T00:09:10Z

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 7 days unless new comments are made or the stale label is removed.

github-actions · 2024-04-04T00:25:38Z

This PR was closed because it has been stalled for 7 days with no activity.

samuelkarp added 2 commits October 14, 2022 11:37

golangci-lint: enforce local-prefixes

1e95bdf

Signed-off-by: Samuel Karp <[email protected]>

samuelkarp mentioned this pull request Oct 14, 2022

Vagrant BOX:rockylinux/8 workflow is flaky (broken?): mount.nfs: requested NFS version or transport protocol is not supported #7536

Closed

mxpv approved these changes Oct 15, 2022

View reviewed changes

thaJeztah requested changes Oct 15, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase label Nov 29, 2023

github-actions bot added the Stale label Mar 28, 2024

github-actions bot closed this Apr 4, 2024

samuelkarp mentioned this pull request Jul 25, 2024

introspection: regenerate UUID if state is empty #10503

Merged

		internalapi "github.com/containerd/containerd/integration/cri-api/pkg/apis"

		"github.com/containerd/containerd/integration/remote/util"


		snapshotstore "github.com/containerd/containerd/pkg/cri/store/snapshot"
		ctrdutil "github.com/containerd/containerd/pkg/cri/util"

		"github.com/containerd/containerd/version"

		"github.com/containerd/containerd/pkg/cri/constants"

		"github.com/containerd/containerd/pkg/shutdown"

		api "github.com/containerd/containerd/api/runtime/sandbox/v1"

		"github.com/containerd/containerd/services"

		api "github.com/containerd/containerd/api/services/sandbox/v1"

Conversation

samuelkarp commented Oct 14, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 15, 2022

Uh oh!

samuelkarp commented Oct 16, 2022

Uh oh!

github-actions bot commented Mar 28, 2024

Uh oh!

github-actions bot commented Apr 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants