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

layer: clarify that layers are gzipped #316

Merged
merged 1 commit into from
Sep 27, 2016

Conversation

jonboulle
Copy link
Contributor

Signed-off-by: Jonathan Boulle jonathanboulle@gmail.com

@vbatts
Copy link
Member

vbatts commented Sep 16, 2016

Yes. I wonder if we worded in such a way that allows that to be optional? The mimetype having the +gzip suffix means it's an option. But will always at least a tar.
Does that make sense?

@jonboulle
Copy link
Contributor Author

The mimetype having the +gzip suffix means it's an option.

does it? I sort of also had that intuition but can't seem to find a reference stipulating that.
https://en.wikipedia.org/wiki/Media_type#Suffix
https://tools.ietf.org/html/rfc6839

@wking
Copy link
Contributor

wking commented Sep 16, 2016

On Fri, Sep 16, 2016 at 06:39:58AM -0700, Jonathan Boulle wrote:

The mimetype having the +gzip suffix means it's an option.

does it? I sort of also had that intuition but can't seem to find a
reference stipulating that.
https://en.wikipedia.org/wiki/Media_type#Suffix
https://tools.ietf.org/html/rfc6839

I don't think +gzip is defined anywhere, but I think we could define
+gzip and refernce RFC 6839 as the pattern we're following.

@vbatts
Copy link
Member

vbatts commented Sep 20, 2016

does it? I sort of also had that intuition but can't seem to find a reference stipulating that.
https://en.wikipedia.org/wiki/Media_type#Suffix
https://tools.ietf.org/html/rfc6839

You're right. Perhaps we ought to describe a Structured Syntax Suffix
for +gzip, in likeness to rfc6839, but for this use-case

@wking
Copy link
Contributor

wking commented Sep 20, 2016

On Tue, Sep 20, 2016 at 06:45:39AM -0700, Vincent Batts wrote:

does it? I sort of also had that intuition but can't seem to find a reference stipulating that.
https://en.wikipedia.org/wiki/Media_type#Suffix
https://tools.ietf.org/html/rfc6839

You're right. Perhaps we ought to describe a Structured Syntax Suffix
for +gzip, in likeness to rfc6839, but for this use-case

Already filed as #332 ;).

@stevvooe
Copy link
Contributor

stevvooe commented Sep 23, 2016

LGTM

In general, the suffixes aren't really optional (Apache HTTPD gets this very wrong, on occasion). In practice, it is much better to treat these as string constants. In Docker, we have them as .gzip to prevent this mucking. I changed this spec to use +gzip to keep it consistent with other media types, thinking that we had the discipline to avoid these problems, but #332 proves I was mistaken.

Approved with PullApprove

@jonboulle
Copy link
Contributor Author

can we land this and move on to #332 et al? needs another LGTM

@stevvooe
Copy link
Contributor

@jonboulle I'd like to see this in but #332 goes way too far. I hope this PR isn't considered a pre-cursor.

@wking
Copy link
Contributor

wking commented Sep 26, 2016

On Mon, Sep 26, 2016 at 11:44:44AM -0700, Stephen Day wrote:

@jonboulle I'd like to see this in but #332 goes way too far. I hope
this PR isn't considered a pre-cursor.

I think requiring application/vnd.oci.image.layer.tar+gzip to be
compressed with gzip makes sense regardless of whether we define
application/vnd.oci.image.layer.tar or a +gzip structured syntax
suffix. So I'm fine landing d58f4c7 as it stands (although I'd
rather be linking to 1, which is the same doc but in a more
obviously versioned location). Whether #332 or other optional
gzipping lands later is orthogonal.

Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>
@vbatts
Copy link
Member

vbatts commented Sep 27, 2016

LGTM

Approved with PullApprove

1 similar comment
@philips
Copy link
Contributor

philips commented Sep 27, 2016

LGTM

Approved with PullApprove

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.

5 participants