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

use containerd/reference/docker instead of docker/distribution #43278

Closed
wants to merge 1 commit into from

Conversation

thaJeztah
Copy link
Member

splitting this off from #40961

similar PR can be found in moby/buildkit#2666, which removes the dependency on this package from BuildKit (only remaining use in the code now)

containerd now maintains a copy of the docker/distribution code under the
github.com/containerd/containerd/reference/docker package.

Let's switch to using that package, to remove the (direct) dependency on
github.com/docker/distribution.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

Ah, some fixes needed in a mock;

distribution/push_v2_test.go:404:4: cannot use repo (type *mockRepo) as type distribution.Repository in field value:
    *mockRepo does not implement distribution.Repository (wrong type for Named method)
        have Named() docker.Named
        want Named() "github.com/docker/docker/vendor/github.com/docker/distribution/reference".Named
distribution/push_v2_test.go:607:3: cannot use repo (type *mockRepoWithBlob) as type distribution.Repository in field value:
    *mockRepoWithBlob does not implement distribution.Repository (wrong type for Named method)
        have Named() docker.Named
        want Named() "github.com/docker/docker/vendor/github.com/docker/distribution/reference".Named
distribution/push_v2_test.go:638:5: cannot use &mockRepo{} (type *mockRepo) as type distribution.Repository in assignment:
    *mockRepo does not implement distribution.Repository (wrong type for Named method)
        have Named() docker.Named
        want Named() "github.com/docker/docker/vendor/github.com/docker/distribution/reference".Named

@thaJeztah
Copy link
Member Author

thaJeztah commented Feb 22, 2022

So, the error is because the mockRepos do not implement this interface;

// Repository is a named collection of manifests and layers.
type Repository interface {
// Named returns the name of the repository.
Named() reference.Named

But I feel silly now; not sure I understand; so the failure is because I swapped the imports. Now, that makes perfect sense if the returned type was a "type", but it's an interface, so this line:

func (m *mockRepo) Named() reference.Named {
m.t.Fatalf("Named() not implemented")
return nil
}

  • Previously returned github.com/docker/distribution/reference.Named
  • But now returns github.com/containerd/containerd/reference/docker.Named

So.. how do they differ? Well.. they don't. The containerd reference/docker package is a copy of distribution's reference package;

Here's the docker/distribution definition:

// Reference is an opaque object reference identifier that may include
// modifiers such as a hostname, name, tag, and digest.
type Reference interface {
// String returns the full reference
String() string
}

// Named is an object with a full name
type Named interface {
Reference
Name() string
}

And here's the containerd definition:

// Reference is an opaque object reference identifier that may include
// modifiers such as a hostname, name, tag, and digest.
type Reference interface {
// String returns the full reference
String() string
}

// Named is an object with a full name
type Named interface {
Reference
Name() string
}

So, I would expect things to fail if those definitions depended on some type, but they're defining an interface that returns a generic (string) type.

A smaller example; https://go.dev/play/p/cRwQGtWCZi9

package main

import "fmt"

type Named interface {
	Name() string
}

type NamedTwo interface {
	Name() string
}

type Registry interface {
	Named() Named
}

// implementNamed implements Named, NamedTwo interface.
type implementNamed struct{ name string }

func (n *implementNamed) Name() string {
	return n.name
}

type oneRegistry struct{}

func (r *oneRegistry) Named() Named {
	return &implementNamed{"one"}
}

type twoRegistry struct{}

func (r *twoRegistry) Named() NamedTwo {
	return &implementNamed{"two"}
}

func main() {
	oneReg := oneRegistry{}
	twoReg := twoRegistry{}

	expectNamed(oneReg.Named())
	expectNamed(twoReg.Named())
	expectNamedTwo(oneReg.Named())
	expectNamedTwo(twoReg.Named())

	type thingWithNamed struct {
		named Named
	}

	// This works; even though twoReg.Named() returns a NamedTwo
	foo := thingWithNamed{}
	foo.named = oneReg.Named()
	fmt.Println(foo.named.Name())

	foo.named = twoReg.Named()
	fmt.Println(foo.named.Name())

	type thingWithRegistry struct {
		registry Registry
	}

	bar := thingWithRegistry{}
	bar.registry = &oneReg

	// But this doesn't work:
	// ./main.go:60:15: cannot use &twoReg (type *twoRegistry) as type Registry in assignment:
	// *twoRegistry does not implement Registry (wrong type for Named method)
	// have Named() NamedTwo
	// want Named() Named

	// bar.registry = &twoReg
}

func expectNamed(in Named) {
	fmt.Println("expectNamed:    " + in.Name())
}

func expectNamedTwo(in NamedTwo) {
	fmt.Println("expectNamedTwo: " + in.Name())
}

Comment on lines 641 to 649
// Named() must return a distref.Named here, not containerd's Named interface;
// while both interfaces are identical, Golang considers it an incompatible
// type otherwise:
//
// distribution/push_v2_test.go:404:4: cannot use repo (type *mockRepo) as type distribution.Repository in field value:
// *mockRepo does not implement distribution.Repository (wrong type for Named method)
// have Named() docker.Named
// want Named() "github.com/docker/docker/vendor/github.com/docker/distribution/reference".Named
func (m *mockRepo) Named() distref.Named {
Copy link
Member Author

Choose a reason for hiding this comment

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

As it looks like the "interface" issues really only affects this Mock, I decided to import the "old" package here. "production" code doesn't need the old interface

Not sure if there's a better solution for this; perhaps we should replace the distribution.Repository interface with something local if possible (haven't dug too deeply into that yet)

@thaJeztah thaJeztah marked this pull request as ready for review February 22, 2022 18:28
@tianon
Copy link
Member

tianon commented Feb 26, 2022

Relevant to what we discussed in the maintainers meeting, it appears this was copied into containerd specifically for containerd/containerd#3554 (via containerd/containerd#3728).

Was there ever any communication with either party about maintaining it in a separate (smaller) place? Now there are two copies that have diverged slightly and it'd be neat if we could help get them back in sync. 🙈

containerd now maintains a copy of the docker/distribution code under the
github.com/containerd/containerd/reference/docker package.

Let's switch to using that package, to remove the (direct) dependency on
github.com/docker/distribution.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@thaJeztah thaJeztah closed this Sep 16, 2023
@thaJeztah thaJeztah deleted the containerd_distribution branch September 16, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants