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: wordsmith non-distributable section #483

Merged
merged 2 commits into from
Jan 18, 2017

Conversation

jonboulle
Copy link
Contributor

Attempt to clarify when non-distributable policies should be enforced
(only on upload, never on download); as discussed in + fixes #475

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

[Descriptors](descriptor.md) referencing these layers MAY include `urls` for downloading these layers.
It is implementation-defined whether or not implementations upload layers tagged with this media type.
Non-distributable layers SHOULD be tagged with an alternative mediatype of `application/vnd.oci.image.layer.nondistributable.v1.tar+gzip`.
Implementations SHOULD NOT upload layers tagged with this media type; however, such a media type SHOULD NOT affect whether an implementation downloads the layer.
Copy link
Contributor

Choose a reason for hiding this comment

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

In a number of situations (e.g. dealing with a local cache or proxy) “downloading” and “uploading” become hard to distinguish from “internal handling” (which is why we went with the less specific approach in the first place). Do we want to provide guidelines on how we define those terms? Or is the existing “never uploaded” (which I still don't like) line sufficient precedence for “folks dealing with local caches and proxies are allowed to pick their own definition of ‘uploaded’ and we'll never actually attempt to validate this SHOULD NOT”?

@wking
Copy link
Contributor

wking commented Dec 8, 2016 via email

@jonboulle
Copy link
Contributor Author

I'm not sure what you're suggesting - just strike the line here for now?

@wking
Copy link
Contributor

wking commented Dec 8, 2016

Yeah, drop your new "Implementations SHOULD NOT upload..." line and keep the old "It is implementation-defined..." line. Then we land this one quick and easy, and you open a new PR with just that "implementation-defined" -> "SHOULD NOT" line swap.

Non-distributable layers SHOULD be tagged with an alternative mediatype of `application/vnd.oci.image.layer.nondistributable.v1.tar+gzip`.
Implementations SHOULD NOT upload layers tagged with this media type; however, such a media type SHOULD NOT affect whether an implementation downloads the layer.

[Descriptors](descriptor.md) referencing non-distributable layers MAY include `urls` for downloading these layers directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the urls field is present, they SHOULD be favored for retrieval over resolving through the digest identifiers.
The presence of the urls field MUST NOT be used to determine whether or not a layer is non-distributable.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the urls field is present, they SHOULD be favored for retrieval over resolving through the digest identifiers.

I don't think we want this, since it sounds like “don't cache these locally”. I don't think we can make generic calls about whether a digest-based retrieval will be faster or slower than urls based retrieval, and I don't see any user-side metrics for deciding between them other than speed.

The presence of the urls field MUST NOT be used to determine whether or not a layer is non-distributable.

I'm fine with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want this, since it sounds like “don't cache these locally”.

Only if you read into it too much and skip words like "retrieval" and "resolving" when trying to understand the meaning. It is also a SHOULD.

Copy link
Contributor

@wking wking Dec 13, 2016

Choose a reason for hiding this comment

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

Does “resolving through the digest identifiers” somehow not include fetching from a local cache? If so, I think it's worth spelling that out explicitly (but defining “a local cache” might be tricky).

And I don't think the SHOULD-ness means we can be sloppy about defining the relevant terms. SHOULD just means compliant implementations can skip the requirement; it shouldn't mean that the requirement itself is ambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wking More "precision" here is going to infer a hierarchy, which actually sacrifices accuracy. The term "retrieval" implies relative displaced locality. In this context, since we are talking about urls, the term is both accurate and precise, since the relative locality is urls, provided in the first clause, which implies network access. Local cache clearly does not fall into the locality level of urls.

Adding more here will just obscure the intent of the statement. It is neither sloppy nor imprecise and ultimately accurate in context.

But let's see what the contributor has to say, as he might have an opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why we need to call out urls as being preferential - what's wrong with the digest resolving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The presence of the urls field MUST NOT be used to determine whether or not a layer is non-distributable.

Mixing SHOULDs and MUSTs here is awkward. With the current wording I really don't think there's any reason to think a reader or implementer would infer the opposite of this; in the descriptor definition, urls is mentioned as a common property, so clearly its presence doesn't imply anything about distributability.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonboulle How so? There has already been confusion that urls == nondistributable, on both docker PRs and OCI PRs. Making this distinction clear will ensure that urls will be useful without being tied to nondistributable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still don't think it's necessary but in the interests of moving forward I added that line, ptal

@stevvooe
Copy link
Contributor

stevvooe commented Dec 8, 2016

Added a few lines, otherwise, LGTM.

You must have a fine forge to be smithing at such a level.

@stevvooe
Copy link
Contributor

@jonboulle Ping.

@stevvooe
Copy link
Contributor

stevvooe commented Jan 6, 2017

LGTM

Approved with PullApprove

@stevvooe
Copy link
Contributor

stevvooe commented Jan 6, 2017

@jonboulle Looks like the DCO check failed.

Attempt to clarify when non-distributable policies should be enforced
(only on upload, never on download); as discussed in + fixes opencontainers#475

Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>
Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>
@jonboulle
Copy link
Contributor Author

le sigh. now with fixed DCO

@stevvooe
Copy link
Contributor

stevvooe commented Jan 9, 2017

LGTM

Approved with PullApprove

1 similar comment
@philips
Copy link
Contributor

philips commented Jan 18, 2017

LGTM

Approved with PullApprove

@philips philips merged commit bccd72a into opencontainers:master Jan 18, 2017
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.

Clarify when non-distributable layer policies should be enforced
4 participants