LGTM

LGTM

@philips Please re-LGTM, thanks

@vbatts @stevvooe PTAL

Rejected.

The urls field is not exemplary of descriptors.

We could add another section explaining the handling of the field, when encountered.

On Mon, Nov 21, 2016 at 12:02:09PM -0800, Stephen Day wrote:

The urls field is not exemplary of descriptors.

It might be exemplary for a nondistributable layer? Maybe we could
show one of those?

It might be exemplary for a nondistributable layer? Maybe we could
show one of those?

Yes, I would keep the example narrow. Basically, "if you see one like this, try to grab the content from the urls before resolving remotely".

On Mon, Nov 21, 2016 at 01:07:49PM -0800, Stephen Day wrote:

It might be exemplary for a nondistributable layer? Maybe we could
show one of those?

Yes, I would keep the example narrow. Basically, "if you see one
like this, try to grab the content from the urls before resolving
remotely".

Do we even need explanatory text? That should already be covered by
the property spec. I was thinking this section would look like:

Examples

The following example describes a Manifest…[snip same as now]

  {
    "mediaType": "application/vnd.oci.image.manifest.v1+json",
    "size": 7682,
    "digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"
  }

The following example describes a non-distributable layer:

  {
    "mediaType": "application/vnd.oci.image.layer.nondistributable.v1.tar+gzip",
    "size": 10815,
    "digest": "sha256:67877ffadef141079c71284ce7d8302e4d05fedc4be397f32f99bc6e97a00cb4",
    "urls": [
      "https://example.com/base-layer.tar.gz
    ]
  }

@wking Yes, we need explanatory text, because a lot of people won't understand that a layer with urls but without the media type isn't nondistributable. Again, as I have said about a million times, context is important.

On Mon, Nov 21, 2016 at 01:45:46PM -0800, Stephen Day wrote:

@wking Yes, we need explanatory text, because a lot of people won't
understand that a layer with urls but without the media type is
nondistributable.

So make it a minimal pivot from the “common case” example:

Examples

The following example describes a Manifest…[snip same as now]

  {
    "mediaType": "application/vnd.oci.image.manifest.v1+json",
    "size": 7682,
    "digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"
  }

If the manifest was available outside of CAS, you could add locations to urls:

  {
    "mediaType": "application/vnd.oci.image.manifest.v1+json",
    "size": 7682,
    "digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"
    "urls": [
      "https://example.com/example-manifest"
    ]
  }

So make it a minimal pivot from the “common case” example:

Common sense isn't so common.

It also makes assumptions about the reader's prior knowledge and their understanding of when urls should be used and when it should not be used. For such a powerful feature, we need to make sure that users can infer implications and apply it to the right scenarios such that it will work in a plurality of environments.

On Mon, Nov 21, 2016 at 02:14:09PM -0800, Stephen Day wrote:

It also makes assumptions about the reader's prior knowledge and
their understanding of when urls should be used and when it should
not be used. For such a powerful feature, we need to make sure that
users can infer implications and apply it to the right scenarios
such that it will work in a plurality of environments.

As it stands, the ‘urls’ property definition has no warnings at all
[1](and the spec portion landed in #169 without warnings either). If
you feel the field needs warnings or SHOULDs and SHOULD NOTs, it seems
like we should start by adding them where the field is specified.
Then any ‘urls’ examples can refer readers to that canonical
description.

Then any ‘urls’ examples can refer readers to that canonical
description.

Or, we can use examples to demonstrate exemplary use cases, with appropriate context, like I requested here.

So what you mean is that in the examples to add urls, there must be relevant instructions？

Is this discussion blocking this from being merged? or can it happen in a follow-up?

The problem here is that the urls field, which isn't supposed to be used in the common case, is part of the only example.

Make a separate example showing the use of urls with the example in a context that helps explains its role.

@stevvooe @wking You mean this change? I feel it does not need to modify, urls and other optional attributes on the same, can appear in the example, if according to you mean, whether to all options in the example to illustrate?

LGTM

@q384566678 Sure, I guess this works. I cannot get the original revision.

@jonboulle @philips PTAL

LGTM