Skip to content

daemon/c8d: Fix duplicate containerd/images import#49140

Merged
thaJeztah merged 3 commits intomoby:masterfrom
vvoland:c8d-fix-duplicate-containerdimages-import
Dec 19, 2024
Merged

daemon/c8d: Fix duplicate containerd/images import#49140
thaJeztah merged 3 commits intomoby:masterfrom
vvoland:c8d-fix-duplicate-containerdimages-import

Conversation

@vvoland
Copy link
Copy Markdown
Contributor

@vvoland vvoland commented Dec 19, 2024 •
edited
Loading

daemon/c8d: Fix duplicate containerd/images import

Remove duplicate imports under different aliases

c8d/delete: Consistent method receiver

imageDeleteConflict is always returned via a reference, so adjust the
method receiver of Conflict to make it consistent with Error.

daemon/c8d: Force c8dimages alias for containerd/images

@vvoland vvoland added status/2-code-review kind/refactor PR's that refactor, or clean-up code containerd-integration Issues and PRs related to containerd integration labels Dec 19, 2024
@vvoland vvoland added this to the 28.0.0 milestone Dec 19, 2024
@vvoland vvoland self-assigned this Dec 19, 2024
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 19, 2024

GitHub having a bad time, or is it related to that cache deprecation they were working on?

Downloading single artifact
Error: Unable to download artifact(s): Artifact not found for name: windows-2022-graphdriver-unit-reports
        Please ensure that your artifact is not expired and the artifact was uploaded using a compatible version of toolkit/upload-artifact.
        For more information, visit the GitHub Artifacts FAQ: https://github.com/actions/toolkit/blob/main/packages/artifact/docs/faq.md

thaJeztah
thaJeztah reviewed Dec 19, 2024
View reviewed changes
Comment thread daemon/containerd/image_delete.go Outdated
"time"

"github.com/containerd/containerd/images"
containerdimages "github.com/containerd/containerd/images"
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Dec 19, 2024

Choose a reason for hiding this comment

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

We could add a linter rule for this import; either require to use an alias, or "when aliasing it MUST be xyz";

moby/.golangci.yml

Lines 90 to 100 in ce9c163

importas:
# Do not allow unaliased imports of aliased packages.
no-unaliased: true
alias:
# Enforce alias to prevent it accidentally being used instead of our
# own errdefs package (or vice-versa).
- pkg: github.com/containerd/errdefs
alias: cerrdefs
- pkg: github.com/opencontainers/image-spec/specs-go/v1
alias: ocispec

I went looking what we used elsewhere, and at least we're consistent; but was slightly considering if we could come up with a shorter alias that's still clear (containerdimages is quite long!!);

git grep ' "github.com/containerd/containerd/images"' -- . ':!vendor'
daemon/containerd/handlers.go:  containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image.go:     containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_builder.go:     containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_children.go:    containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_delete.go:      containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_exporter.go:    containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_history.go:     containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_inspect.go:     containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_manifest.go:    containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_prune.go:       containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_push.go:        containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_push_test.go:   containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_snapshot.go:    containerdimages "github.com/containerd/containerd/images"
daemon/containerd/image_tag.go: containerdimages "github.com/containerd/containerd/images"
daemon/containerd/soft_delete.go:       containerdimages "github.com/containerd/containerd/images"
git grep '\t"github.com/containerd/containerd/images"' -- . ':!vendor'
builder/builder-next/adapters/containerimage/pull.go:   "github.com/containerd/containerd/images"
builder/builder-next/adapters/localinlinecache/inlinecache.go:  "github.com/containerd/containerd/images"
builder/builder-next/worker/worker.go:  "github.com/containerd/containerd/images"
daemon/containerd/image_delete.go:      "github.com/containerd/containerd/images"
daemon/containerd/image_delete_test.go: "github.com/containerd/containerd/images"
daemon/containerd/image_events.go:      "github.com/containerd/containerd/images"
daemon/containerd/image_import.go:      "github.com/containerd/containerd/images"
daemon/containerd/image_list.go:        "github.com/containerd/containerd/images"
daemon/containerd/image_list_test.go:   "github.com/containerd/containerd/images"
daemon/containerd/image_manifest.go:    "github.com/containerd/containerd/images"
daemon/containerd/image_pull.go:        "github.com/containerd/containerd/images"
daemon/containerd/image_push.go:        "github.com/containerd/containerd/images"
daemon/containerd/image_test.go:        "github.com/containerd/containerd/images"
daemon/containerd/progress.go:  "github.com/containerd/containerd/images"
daemon/containerd/service.go:   "github.com/containerd/containerd/images"
daemon/images/image.go: "github.com/containerd/containerd/images"
image/tarexport/save.go:        "github.com/containerd/containerd/images"
integration/image/pull_test.go: "github.com/containerd/containerd/images"
integration/plugin/common/plugin_test.go:       "github.com/containerd/containerd/images"
libcontainerd/remote/client.go: "github.com/containerd/containerd/images"
plugin/backend_linux.go:        "github.com/containerd/containerd/images"
plugin/fetch_linux.go:  "github.com/containerd/containerd/images"

Copy link
Copy Markdown
Contributor Author

@vvoland vvoland Dec 19, 2024

Choose a reason for hiding this comment

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

Switched to explicit c8dimages and added a golangci rule

thaJeztah
thaJeztah approved these changes Dec 19, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM if CI is happy, thanks!

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 19, 2024

The good news; the linter works
The bad news; looks like there's some imports that need updating 😅

@vvoland
Copy link
Copy Markdown
Contributor Author

vvoland commented Dec 19, 2024

Right, missed another grep pass 😅

@vvoland vvoland force-pushed the c8d-fix-duplicate-containerdimages-import branch from 1771d25 to d55702b Compare December 19, 2024 17:31
@vvoland
daemon/c8d: Fix duplicate containerd/images import
1ad78f0 
Remove duplicate imports under different aliases

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland
c8d/delete: Consistent method receiver
8c30e11 
`imageDeleteConflict` is always returned via a reference, so adjust the
method receiver of `Conflict` to make it consistent with `Error`.

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland
daemon/c8d: Force c8dimages alias for containerd/images
90fef06 
Change all github.com/containerd/containerd/images imports to be
imported as `c8dimages`.

Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland vvoland force-pushed the c8d-fix-duplicate-containerdimages-import branch from d55702b to 90fef06 Compare December 19, 2024 17:39
thaJeztah
thaJeztah approved these changes Dec 19, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 19, 2024

Windows CI doesn't want to cooperate; keep getting some (seemingly unrelated) failures; I restarted "all" tasks instead now, sometimes that helps. Last failure;

=== Failed
=== FAIL: github.com/docker/docker/integration/networking TestNatNetworkICC/User_defined_nat_network (8.60s)
    nat_windows_test.go:62: assertion failed: error is not nil: Post "http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.48/containers/5d5a551260df960ea1ca0daf3ced1d0fd20bd0e73ffb330a1b3526278b5be0a8/start": context deadline exceeded
    panic.go:629: assertion failed: error is not nil: Error response from daemon: error while removing network: network mynat id 2827d72a9da6a7910a416069d6286d17a784276b7cfa37dcf624cbe44661692c has active endpoints

=== FAIL: github.com/docker/docker/integration/networking TestNatNetworkICC (16.30s)

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Dec 19, 2024

Yay, green now 🎉

@thaJeztah thaJeztah merged commit ec0dba0 into moby:master Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@cpuguy83 cpuguy83 Awaiting requested review from cpuguy83 cpuguy83 is a code owner

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

Assignees

@vvoland vvoland

Labels

containerd-integration Issues and PRs related to containerd integration kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Milestone

28.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@vvoland @thaJeztah