Skip to content

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

Merged
vbatts merged 1 commit into
opencontainers:masterfrom
zhouhao3:test-url
Dec 6, 2016
Merged

descriptor.md: Add the urls attribute instance in the Examples#437
vbatts merged 1 commit into
opencontainers:masterfrom
zhouhao3:test-url

Conversation

@zhouhao3

@zhouhao3 zhouhao3 commented Nov 3, 2016

Copy link
Copy Markdown

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

Comment thread 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"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

missing , at the end of this line

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated

Comment thread descriptor.md Outdated
"digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270",
"urls": {
"https://example.com/example"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@philips

philips commented Nov 17, 2016

Copy link
Copy Markdown
Contributor

LGTM

Approved with PullApprove

Comment thread descriptor.md Outdated
"digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270",
"urls": {
"https://example.com/example"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

urls should be an array, not an object.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated,thanks

@wking

wking commented Nov 17, 2016 via email

Copy link
Copy Markdown
Contributor

@jonboulle

jonboulle commented Nov 17, 2016

Copy link
Copy Markdown
Contributor

LGTM

Approved with PullApprove

@zhouhao3

Copy link
Copy Markdown
Author

@philips Please re-LGTM, thanks

@zhouhao3

Copy link
Copy Markdown
Author

@vbatts @stevvooe PTAL

@stevvooe

Copy link
Copy Markdown
Contributor

Rejected.

The urls field is not exemplary of descriptors.

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

@wking

wking commented Nov 21, 2016

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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

wking commented Nov 21, 2016

Copy link
Copy Markdown
Contributor

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

stevvooe commented Nov 21, 2016

Copy link
Copy Markdown
Contributor

@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

wking commented Nov 21, 2016

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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

wking commented Nov 22, 2016

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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.

@zhouhao3

Copy link
Copy Markdown
Author

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

@vbatts

vbatts commented Nov 30, 2016

Copy link
Copy Markdown
Member

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

@stevvooe

Copy link
Copy Markdown
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.

@zhouhao3

zhouhao3 commented Dec 1, 2016

Copy link
Copy Markdown
Author

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

Comment thread descriptor.md Outdated
"digest": "sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270"
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated

Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
@stevvooe

stevvooe commented Dec 6, 2016

Copy link
Copy Markdown
Contributor

LGTM

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

Approved with PullApprove

@zhouhao3

zhouhao3 commented Dec 6, 2016

Copy link
Copy Markdown
Author

@jonboulle @philips PTAL

@wking

wking commented Dec 6, 2016 via email

Copy link
Copy Markdown
Contributor

@vbatts

vbatts commented Dec 6, 2016

Copy link
Copy Markdown
Member

LGTM

Approved with PullApprove

@vbatts vbatts merged commit c2ba0e3 into opencontainers:master Dec 6, 2016
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.

7 participants