Skip to content

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

Details
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())
}

@thaJeztah thaJeztah force-pushed the containerd_distribution branch from 3dc29b9 to 0d5356b Compare February 22, 2022 15:50
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. 🙈

@thaJeztah thaJeztah force-pushed the containerd_distribution branch from 0d5356b to b7f49f2 Compare October 18, 2022 14:43
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

area/distribution Image Distribution kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants