Skip to content

Conversation

@runcom
Copy link
Member

@runcom runcom commented Jun 29, 2016

This is an attempt to fix containers/skopeo#102 (comment) so we can move on with #23 w/o breaking skopeo copy when working with the Atomic Registry.

@mtrmac PTAL

Signed-off-by: Antonio Murdaca runcom@redhat.com

image/image.go Outdated
// private cache for Signatures(); nil if not yet known.
cachedSignatures [][]byte
cachedSignatures [][]byte
supportedDestMIMEType []string
Copy link
Member Author

Choose a reason for hiding this comment

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

note to myself, should be plural

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually isn’t this more requestedManifestMIMETypes? the Image has no concept of a destination, and the MIME types mostly affect what we read and serve.

Copy link
Member Author

Choose a reason for hiding this comment

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

it should be requestedManifestMIMETypes indeed

@runcom
Copy link
Member Author

runcom commented Jun 29, 2016

I'm sure design needs a better look - I integrated this code in containers/skopeo#102 in commit containers/skopeo@7624571 but I'm still unsure about the missing or nil arg in image.FromSource.

I'm wondering if the SupportedDestinationMIMETypes() should be static in the given package and not called from an instance of an ImageDestination because otherwise I can't use FromSource w/o first getting a given ImageDestination

@runcom
Copy link
Member Author

runcom commented Jun 29, 2016

EDIT: the above patch fixed the issue in skopeo :) - now, on design review here :)

types/types.go Outdated
PutSignatures(signatures [][]byte) error
// SupportedImageDestinationMIMEType tells which mime types the destination support when uploading
// If an empty slice or nil it's returned, then any mime type can be tried to upload
SupportedImageDestinationMIMEType() []string
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Destination is redundant, and so is Image (and are we talking about manifests only, or blobs as well?)
  • Return a slice, so this should be plural.

{Supported,Accepted}{Manifest,}MIMETypes ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Return a slice, so this should be plural.

#26 (comment)

SupportedMIMETypes() is ok for me

Copy link
Member Author

Choose a reason for hiding this comment

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

it's valid only for manifests also

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 29, 2016

I'm wondering if the SupportedDestinationMIMETypes() should be static in the given package and not called from an instance of an ImageDestination because otherwise I can't use FromSource w/o first getting a given ImageDestination.

It would be nice to have class methods instead of plain interfaces in Go.

… but in this case, actually it is desirable to have it as a method on ImageDestination. Just because a remote HTTP service speaks the docker/distribution protocol does not automatically mean that it accepts all future MIME types, getting the list of supported types may (or even should?) involve an API call to that service.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom runcom force-pushed the supported-mime-types branch from 96d970a to aa53132 Compare June 29, 2016 15:21
@runcom
Copy link
Member Author

runcom commented Jun 29, 2016

Updated PTAL @mtrmac

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 29, 2016

👍

Approved with PullApprove

@runcom
Copy link
Member Author

runcom commented Jun 29, 2016

lgtm

Approved with PullApprove

@runcom
Copy link
Member Author

runcom commented Jun 29, 2016

I'm going to rebase #23 and then go for the containers/skopeo#102 rebase as the final step

@runcom runcom merged commit 65d69b5 into containers:master Jun 29, 2016
@runcom runcom deleted the supported-mime-types branch June 29, 2016 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants