descriptor.md: Add the urls attribute instance in the Examples #437

Merged
merged 1 commit into from Dec 6, 2016

Projects

None yet

7 participants

@q384566678
Contributor

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

descriptor.md
@@ -115,5 +115,8 @@ The following example describes a [_Manifest_](manifest.md#image-manifest) with
"mediaType": "application/vnd.oci.image.manifest.v1+json",
"size": 7682,
"digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"
@coolljt0725
coolljt0725 Nov 3, 2016 Contributor

missing , at the end of this line

@q384566678
q384566678 Nov 3, 2016 Contributor

updated

descriptor.md
+ "digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270",
+ "urls": {
+ "https://example.com/example"
+ }
@philips
Contributor
philips commented Nov 17, 2016 edited

LGTM

Approved with PullApprove

descriptor.md
+ "digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270",
+ "urls": {
+ "https://example.com/example"
+ }
@wking
wking Nov 17, 2016 Contributor

urls should be an array, not an object.

@q384566678
q384566678 Nov 17, 2016 Contributor

updated,thanks

@wking
Contributor
wking commented Nov 17, 2016
@jonboulle
Contributor
jonboulle commented Nov 17, 2016 edited

LGTM

Approved with PullApprove

@q384566678
Contributor

@philips Please re-LGTM, thanks

@q384566678
Contributor
@stevvooe
Contributor

Rejected.

The urls field is not exemplary of descriptors.

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

@wking
Contributor
wking commented Nov 21, 2016

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?

@stevvooe
Contributor

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".

@wking
Contributor
wking commented Nov 21, 2016

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
    ]
  }
@stevvooe
Contributor
stevvooe commented Nov 21, 2016 edited

@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.

@wking
Contributor
wking commented Nov 21, 2016

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"
    ]
  }
@stevvooe
Contributor

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.

@wking
Contributor
wking commented Nov 22, 2016

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.

@stevvooe
Contributor

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.

@q384566678
Contributor

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

@vbatts
Member
vbatts commented Nov 30, 2016

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

@stevvooe
Contributor

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.

@q384566678
Contributor

@stevvooe @wking You mean this change? I feel it does not need to modify, urlsand 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?

descriptor.md
@@ -116,4 +116,15 @@ The following example describes a [_Manifest_](manifest.md#image-manifest) with
"size": 7682,
"digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"
}
+
+If the manifest was available outside of CAS, you could add locations to urls:
@jonboulle
jonboulle Dec 1, 2016 Contributor

In the following example, the descriptor indicates that the referenced manifest is retrievable from a particular URL:

@q384566678
q384566678 Dec 2, 2016 Contributor

updated

@q384566678 q384566678 descriptor.md: Add the urls attribute instance in the Examples
Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
bc78dcc
@stevvooe
Contributor
stevvooe commented Dec 6, 2016 edited

LGTM

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

Approved with PullApprove

@q384566678
Contributor
@wking
Contributor
wking commented Dec 6, 2016
@vbatts
Member
vbatts commented Dec 6, 2016 edited

LGTM

Approved with PullApprove

@vbatts vbatts merged commit c2ba0e3 into opencontainers:master Dec 6, 2016

2 checks passed

code-review/pullapprove Approved by stevvooe, vbatts
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment