docker / buildx

Docker CLI plugin for extended build capabilities with BuildKit
Apache License 2.0
3.33k stars 448 forks source link

[v0.15 backport] remove use of deprecated containerd/containerd/errdefs #2517

Closed thaJeztah closed 2 weeks ago

thaJeztah commented 3 weeks ago

This package has moved to a separate module. Also added linting rules to prevent accidental reintroduction.

(cherry picked from commit c116af7b82b7f9af8bb73ff84483d2224405a959)

thompson-shaun commented 3 weeks ago

Since it's moving to a new package, is this something we need to backport to the v0.15 line?

Already in that new and super-awesome v0.16 😄

thaJeztah commented 3 weeks ago

@thompson-shaun we already updated all code to the new module, but this one import must have sneaked back in; containerd currently provides an alias for it, but we should not depend on that.

The updated linter rules should prevent these from sneaking back in.

thompson-shaun commented 3 weeks ago

Thank you for clarifying @thaJeztah - definitely sounds backport-y 😄 LGTM

thaJeztah commented 3 weeks ago

@thompson-shaun so for a bit of context; the "trick" with Go when code is moved elsewhere is to provide an alias; say, I have a type named Foo and a func Bar in my awesome package;

package awesome

type Foo string

func Bar() Foo {
    return "hello"
}

And maybe those must be moved elsewhere, which can be either in the same repository, but could even be (such as in this case) to a separate module. Go is strongly typed, so newlocation.Foo != oldlocation.Foo (even if they're the same). An alias (literally just =) means that they're the same type with a different name. This helps transitioning projects that depend on your module in cases where some code already moved to the new location, but some code (which could be in indirect dependencies) still has to make the move; without the alias, things would break.

But usually you'd also deprecated the old location; code will still compile (and work), but adding the Deprecated: comment will make linters shout, and with a proper comment, users can learn what they need replace the deprecated code with. For example;

package awesome // Deprecated: package awesome moved to the example.com/my/newmodule module.

import "example.com/my/newmodule"

// Deprecated: use [newmodule.FooNew] instead.
type Foo = newmodule.FooNew

// Deprecated: use [newmodule.BarNew] instead.
func Bar() newmodule.FooNew {
    return newModule.BarNew()
}

But ideally you'd get rid of depending on those aliases, so that WHEN those aliases are removed by the dependency you're using (which may be out of your control if some indirect dependency forces you to update), you're prepared for that to happen, and things don't break; in this case buildx was still using the old location of code that moved elsewhere, and it was depending on aliases; https://github.com/docker/buildx/blob/d3a53189f7e9c917eeff851c895b9aad5a66b108/vendor/github.com/containerd/containerd/errdefs/errdefs_deprecated.go#L47-L53

tonistiigi commented 2 weeks ago

image