reference: upstreams additions from forked docker/reference package by stevvooe · Pull Request #1277 · distribution/distribution

stevvooe · 2015-12-17T02:33:23Z

Rather than fork, we have now upstreamed some of the additions required in
https://github.com/docker/docker/pull/18586. Of upmost importance is the
addition of a useful WithName function.

While did attempt to restrict Reference implementations to being from the
reference package, making Reference a proper algebraic data type, we ran into
issues with the usage in the main docker project. We will re-examine this
approach in the future.

This change will be followed up with some other additions from the daemon.

Signed-off-by: Stephen J Day stephen.day@docker.com

aaronlehmann · 2015-12-17T02:58:06Z

Typo: should be NamedOnly

Thanks. Fixed.

stevvooe · 2015-12-18T01:22:08Z

Consider moving the "docker default" behavior into this package, as well.

aaronlehmann · 2015-12-18T19:37:28Z

I see what you're going for here but I don't think we should make this interface change yet, because it will make it impossible to vendor an updated distribution repository in the Docker Engine until all the reference changes are upstreamed. We will need to vendor an updated distribution repository soon to get the updated manifest code. So I would prefer to wait until the Engine is ported over to a unified reference package before adding a internal() method to this interface.

I am sorry, but I want to force that requirement. I am not to thrilled about the forking of the reference package and have put in measures to ensure that doesn't happen again. This also let's us write helper functions with stronger guarantees.

I understand that you want to force the requirement, but I think it needs to come after the package is unified. I don't want to merge something that will make it impossible to vendor an updated distribution package without further big changes, especially when I need to vendor an updated distribution package soon.

Per our conversation, this has been removed.

aaronlehmann · 2015-12-18T19:53:29Z

Do you plan to address this before merging, or in a followup?

This will come in a follow up.

dmcgowan · 2016-01-12T19:22:00Z

Consider moving the "docker default" behavior into this package, as well.

I would prefer to add ...WithDefault functions which specify the default behavior. In that case we can provide what docker needs without polluting the package with legacy default behavior.

aaronlehmann · 2016-01-13T19:42:46Z

There is a compile error in reference.go.

I think the changes to WithName and WithTag should be reverted now that internal() is not part of the method set, since they assume the reference is one of the concrete types defined in this file.

stevvooe · 2016-01-13T21:16:49Z

@aaronlehmann Should be fixed now.

aaronlehmann · 2016-01-13T21:20:39Z

Could this function be preserved, perhaps under the name ParseNameOnly? It is used in several places in the engine.

Is this not identical to ParseNamed?

It seems there are two paths here: one path uses ParseNamed + NamedOnly/IsNameOnly and the other uses this function (or what would be ParseNameOnly). Do we need to ensure that we only use the repository name or do need to ensure that the input is strictly the repository name?

It's not identical to ParseNamed because this function won't accept tags or digests. There are some cases where an item being parsed is strictly a name only, and tags or digests shouldn't be accepted. It would be useful to keep this convenience function to support this case. It's fine with me if it's implemented as ParseNamed + NamedOnly.

Ok, so I'll add something with the strict behavior.

Rather than fork, we have now upstreamed some of the additions required in moby/moby#18586. Of upmost importance is the addition of a useful WithName function. While did attempt to restrict Reference implementations to being from the reference package, making Reference a proper algebraic data type, we ran into issues with the usage in the main docker project. We will re-examine this approach in the future. This change will be followed up with some other additions from the daemon. Signed-off-by: Stephen J Day <stephen.day@docker.com>

aaronlehmann · 2016-01-13T23:33:34Z

LGTM - but please hold off on merging to avoid interfering with cross-repository push vendoring

Signed-off-by: Stephen J Day <stephen.day@docker.com>

aaronlehmann · 2016-01-14T02:07:46Z

It looks like this won't play nicely without backing types defined outside this package. This seems like an oversight, because the godoc comment says implies it works with other types.

Was that not covered by the comment?

+// If the target of named is from another package, any data on the backing +// type will be lost and replaced with an instance from this package. Simply +// put, the resulting type will only include information available on the +// interface.

I may be misreading the code, but it looks like if you call it with a type that's not handled by the switch statement, it returns ErrNameDisallowed.

So the comment says the extra data from a foreign type will be lost, but in reality you just get an error.

The cases for Canonical, Named and Tagged are handled. What else is missing?

I don't see those. I only see concrete types being handled.

Er, looking at the wrong one. Are you implying it should look like WithTag below?

Yes, I think that would make sense.

dmp42 · 2016-01-25T19:27:17Z

What's the status on this one?

stevvooe · 2016-01-25T20:53:32Z

@dmp42 We are still working towards a balance here. The engine PR will drive this one.

dmcgowan · 2017-01-10T00:59:02Z

Merged alternate with #1778

GordonTheTurtle added the status/0-triage label Dec 17, 2015

stevvooe mentioned this pull request Dec 17, 2015

Reference package should not be forked moby/moby#18731

Closed

aaronlehmann reviewed Dec 17, 2015
View reviewed changes

stevvooe force-pushed the reference-package-additions branch from cad87a1 to adbf469 Compare December 17, 2015 21:52

aaronlehmann reviewed Dec 18, 2015
View reviewed changes

stevvooe force-pushed the reference-package-additions branch from adbf469 to b27eda9 Compare December 18, 2015 19:43

aaronlehmann reviewed Dec 18, 2015
View reviewed changes

stevvooe force-pushed the reference-package-additions branch 3 times, most recently from a78e63a to 637b365 Compare January 13, 2016 02:51

stevvooe force-pushed the reference-package-additions branch from 637b365 to 16657ab Compare January 13, 2016 21:16

aaronlehmann reviewed Jan 13, 2016
View reviewed changes

stevvooe force-pushed the reference-package-additions branch from 16657ab to 6664e6d Compare January 13, 2016 22:00

reference: add a strict version of ParseNamed

1e44b42

Signed-off-by: Stephen J Day <stephen.day@docker.com>

aaronlehmann reviewed Jan 14, 2016
View reviewed changes

amylindburg added this to the Registry/2.4 milestone Jan 26, 2016

amylindburg assigned aaronlehmann Jan 26, 2016

This was referenced Mar 23, 2016

Make required arguments required. docker-archive-public/docker.engine-api#162

Merged

Update engine api with required arguments moby/moby#21657

Merged

RichardScothern modified the milestones: Registry/2.5, Registry/2.4 Jun 1, 2016

RichardScothern modified the milestones: Registry/2.6, Registry/2.5 Aug 19, 2016

dmcgowan modified the milestones: Registry/2.7, Registry/2.6 Nov 14, 2016

dmcgowan added status/1-design-review and removed status/0-triage labels Nov 23, 2016

dmcgowan closed this Jan 10, 2017

dmcgowan removed this from the Registry/2.7 milestone Mar 21, 2017

Conversation

stevvooe commented Dec 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Dec 18, 2015

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!

dmcgowan commented Jan 12, 2016

Uh oh!

aaronlehmann commented Jan 13, 2016

Uh oh!

stevvooe commented Jan 13, 2016

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!

aaronlehmann commented Jan 13, 2016

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!

dmp42 commented Jan 25, 2016

Uh oh!

stevvooe commented Jan 25, 2016

Uh oh!

dmcgowan commented Jan 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants