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

image-layout: clarification of oci-layout #434

Conversation

vbatts
Copy link
Member

@vbatts vbatts commented Nov 1, 2016

The section for this file was too crammed together. Breaking it out into
individual items allows for more straightforward validation.

Signed-off-by: Vincent Batts vbatts@hashbangbash.com

@vbatts vbatts force-pushed the image-layout_oci-layout_clarification branch 2 times, most recently from d3f520e to daac7a3 Compare November 1, 2016 16:54

- "oci-layout" MUST contain a JSON object with a version field `{"imageLayoutVersion": "1.0.0"}` and MAY include additional fields.
- It MUST be a JSON object
- It MUST contain a `imageLayoutVersion` field for determing the version of the layout used
Copy link
Contributor

Choose a reason for hiding this comment

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

“a imageLayoutVersion” → “an imageLayoutVersion” and “determing” → “determining”.

I'm also fine with the shorter:

  • It MUST contain an imageLayoutVersion field.

since the purpose of that field seems fairly obvious.

@@ -15,9 +15,12 @@ The image layout has two top level directories:
- "refs" contains [descriptors][descriptors]. Commonly pointing to an [image manifest](manifest.md#image-manifest) or an [image manifest list](manifest-list.md#oci-image-manifest-list-specification).


It also contains a file that is used to identify the layout version:
The image layout MUST contain a file "oci-layout":
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of quoting, can we use backticks (oci-layout)?

- "oci-layout" MUST contain a JSON object with a version field `{"imageLayoutVersion": "1.0.0"}` and MAY include additional fields.
- It MUST be a JSON object
- It MUST contain a `imageLayoutVersion` field for determing the version of the layout used
- The `imageLayoutVersion` value will align with the `specs.Version` at the time changes to the layout are made, and will pin a given version until changes to the layout are required
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth pointing out that this means imageLayoutVersion is not semantically versioned. I'm ok with that, since I expect most image-layout bumps will break backwards compat anyway, but my personal preference would be to have a semantically versioned imageLayoutVersion that is more independent of the spec version. You'd bump the spec version if any constituent component added a new feature (minor bump) or a backwards-incompatible feature (major bump). So:

  • A major imageLayoutVersion bump would require a major spec version bump.
  • A minor imageLayoutVersion bump would require at least a minor spec version bump.
  • A new layer media type (e.g. for Any chance of changing the whiteout file approach? #24) would require a minor spec version bump but no imageLayoutVersion bump.
  • Dropping an old media type (e.g. after types based on .wh.* fall out of use) would require a major spec version bump but no imageLayoutVersion bump.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'd rather change “specs.Version” → “OCI Image Format Specification version” to avoid referencing the informative Go from Markdown spec.

@vbatts vbatts force-pushed the image-layout_oci-layout_clarification branch from daac7a3 to 538e518 Compare November 1, 2016 18:24
- "oci-layout" MUST contain a JSON object with a version field `{"imageLayoutVersion": "1.0.0"}` and MAY include additional fields.
- It MUST be a JSON object
- It MUST contain an `imageLayoutVersion` field
- The `imageLayoutVersion` value will align with the OCI Image Specification version at the time changes to the layout are made, and will pin a given version until changes to the layout are required
Copy link
Contributor

Choose a reason for hiding this comment

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

You've gone with “OCI Image Specification”, but the title for this spec is “Open Container Initiative Image Format Specification”. I'm fine changing “Open Container Initiative” → “OCI”, but don't think we should be dropping “Format” here unless we also drop it from the spec title. I don't mind which way we go, but I think we should stay consistent.

I also still think we should either semantically version imageLayoutVersion or point out that this is not semantically versioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with this because the image-layout.md doc is titled "OCI Image Layout Specification".

Copy link
Contributor

Choose a reason for hiding this comment

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

This file is about the OCI Image Layout Specification, and if imageLayoutVersion semantically versions that, I'm happy. However, it seems like the “pin” language was adding a relationship to the project-wide OCI Image Format Specification version.

To distinguish the two cases, pretend we had already cut 1.0.0, and say there is a backwards-compatible change to the image layout (e.g. you add an optional index, #438) and a backwards-incompatible change to something else (e.g. we drop support for .wh.*, #24). The backwards-incompatible change means the project as a whole goes to 2.0.0. With the independently-SemVer'ed approach, the backwards-compatible image-layout change moves imageLayoutVersion to 1.1.0. With the pinned-to-the-project-version approach, imageLayoutVersion moves to 2.0.0. Which of those is your intended result?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dare I say that some kind of specific-feature based compatibility would most closely be like the bittorrent BEP process, where specific features or enhancements are assigned an indicator (i.e. BEP0001). Rather than rev'ing a version that requires compliance of all features, an implementation can specify the specific features they accommodate.
Your pretending may certainly find benefits and drawbacks to both approaches. Neither is perfect. What is your intended result?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with tagged features (opencontainers/runtime-spec#15), but the OCI specs seemed to be leaning towards linearized versioning. With linearized versioning, my preferred result is 1.1.0 (because it avoids confusing implementers and layout authors who are interested in image-layout but not with the other spec features). I don't expect much feature fragmentation in image-layout, so shifting to tagged features would be a small gain. However, just because I don't foresee fragmentation doesn't mean we won't have it ;). So I'm also fine switching to tagged features proactively.

wking referenced this pull request in opencontainers/umoci Nov 6, 2016
Signed-off-by: Aleksa Sarai <asarai@suse.com>
@philips
Copy link
Contributor

philips commented Nov 17, 2016

LGTM

Approved with PullApprove

The section for this file was too crammed together. Breaking it out into
individual items allows for more straightforward validation.

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@vbatts vbatts force-pushed the image-layout_oci-layout_clarification branch from 1c4720b to 9c333aa Compare November 21, 2016 18:53
@vbatts
Copy link
Member Author

vbatts commented Nov 21, 2016

updated. PTAL.

@jonboulle
Copy link
Contributor

jonboulle commented Nov 28, 2016

lgtm

Approved with PullApprove

@stevvooe
Copy link
Contributor

stevvooe commented Dec 7, 2016

LGTM

Do we have any language about how implementations must error if the Major is different, but must be able to process future minors/patch?

Approved with PullApprove

@jonboulle jonboulle merged commit 656fb2f into opencontainers:master Dec 7, 2016
@wking
Copy link
Contributor

wking commented Dec 7, 2016 via email

@vbatts vbatts deleted the image-layout_oci-layout_clarification branch December 9, 2016 14:29
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