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

serialization: remove extra "hierarchy" from mediatypes #212

Merged

Conversation

stevvooe
Copy link
Contributor

The fact that we are serializing is implied. This PR removes the extra
bits in the media type that only serve to confuse and waste bytes. The
new media types better reflect the reality for the usage of each type
and how they relate.

This must be followed up by a split of the serialization document, but
this change should be applied first.

This change is backwards incompatible with previous versions of the OCI
specification. Since we are before 1.0, this is acceptable.

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

Related to #206.

@stevvooe stevvooe added this to the v1.0.0 milestone Aug 26, 2016
"size": 7023,
"digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"
},
"layers": [
{
"mediaType": "application/vnd.oci.image.serialization.rootfs.tar.gzip",
"mediaType": "application/vnd.oci.image.layer.tar+gzip",
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the +{suffix} format is from RFC 6839. That doesn't have any examples of nested formats, but it doesn't specify a formal syntax for suffixes either. Since the layer idea doesn't seem particularly tied to tar or gzip, I'd rather use application/vnd.oci.image.layer+tar+gzip to make it easy to also support +zip or +tar+xz, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just +tar (with no compression, see here).

@vbatts
Copy link
Member

vbatts commented Aug 29, 2016

OHMAN GPG SIGNED COMMIT

@vbatts
Copy link
Member

vbatts commented Aug 29, 2016

While i'm not terribly opposed, I'm not sure what harm or confusion having the name stay is?
The biggest beneficial piece of this seems to be the +gzip such that it's an optional representation of the artifact.

@stevvooe
Copy link
Contributor Author

stevvooe commented Aug 29, 2016

@vbatts I'd like to see cruft remove, if we can.

It would be great to get an LGTM here so I can carry on with the split.

@stevvooe stevvooe modified the milestones: v0.5.0, v1.0.0 Aug 31, 2016
@philips
Copy link
Contributor

philips commented Aug 31, 2016

On the call no one really objected to making this change on the call. Saving a few bytes isn't a huge deal in json land but I don't object.

LGTM
(rebase needed, I will LGTM again)

Approved with PullApprove

@stevvooe stevvooe force-pushed the remove-serialization-from-mediatype branch from 432c134 to dd8eaff Compare August 31, 2016 23:09
@stevvooe
Copy link
Contributor Author

@philips @vbatts rebase, PTAL

The fact that we are serializing is implied. This PR removes the extra
bits in the media type that only serve to confuse and waste bytes. The
new media types better reflect the reality for the usage of each type
and how they relate.

This must be followed up by a split of the serialization document, but
this change _should_ be applied first.

This change is backwards incompatible with previous versions of the OCI
specification. Since we are before 1.0, this is acceptable.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
@stevvooe stevvooe force-pushed the remove-serialization-from-mediatype branch from dd8eaff to 5861fdb Compare September 1, 2016 00:40
@vbatts
Copy link
Member

vbatts commented Sep 1, 2016

LGTM

Approved with PullApprove

1 similar comment
@philips
Copy link
Contributor

philips commented Sep 1, 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.

4 participants