Skip to content

specs-go: add missing urls field of descriptor #431

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.

Sign up for GitHub

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

Jump to bottom
Merged
jonboulle merged 1 commit into from
Nov 4, 2016
Merged

specs-go: add missing urls field of descriptor #431

jonboulle merged 1 commit into from
Nov 4, 2016

Conversation

@coolljt0725
Copy link
Member

@coolljt0725 coolljt0725 commented Nov 1, 2016

Signed-off-by: Lei Jitang leijitang@huawei.com

@wking
Copy link
Contributor

wking commented Nov 1, 2016 via email

@coolljt0725
Copy link
Member Author

coolljt0725 commented Nov 2, 2016

@wking thanks for your reminder, will do that

@coolljt0725
Copy link
Member Author

coolljt0725 commented Nov 2, 2016

@wking added a commit to update the JSON schema

@coolljt0725 coolljt0725 force-pushed the urls branch 2 times, most recently from 054b6c3 to 9233d34 Compare November 2, 2016 08:05
@runcom
Copy link
Member

runcom commented Nov 2, 2016

LGTM, ping @vbatts @philips can we get this in so tooling can start using this?

jonboulle
jonboulle reviewed Nov 2, 2016
View reviewed changes
schema/content-descriptor.json
"digest": {
"$ref": "defs-image.json#definitions/digest"
},
"urls": {
Copy link
Contributor

@jonboulle jonboulle Nov 2, 2016

Choose a reason for hiding this comment

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

description field?

@jonboulle
Copy link
Contributor

jonboulle commented Nov 2, 2016

commit message/title has a typo (filed -> field) and could be squashed with the other commit

@wking
Copy link
Contributor

wking commented Nov 2, 2016

manifestDescriptor in defs-image.json should be updated too.

@coolljt0725 coolljt0725 changed the title specs-go: add missing urls filed of desciptor specs-go: add missing urls field of desciptor Nov 3, 2016
@coolljt0725 coolljt0725 changed the title specs-go: add missing urls field of desciptor specs-go: add missing urls field of descriptor Nov 3, 2016
@coolljt0725 coolljt0725 force-pushed the urls branch 2 times, most recently from a220643 to b8d624f Compare November 3, 2016 01:37
@coolljt0725
Copy link
Member Author

coolljt0725 commented Nov 3, 2016

@wking @jonboulle
The commit message and title has updated, also squash to a commit.
Add description field.
Update manifestDescriptor in defs-image.json

wking
wking reviewed Nov 3, 2016
View reviewed changes
schema/content-descriptor.json Outdated
"description": "a list of urls from which this object may be downloaded",
"type": "array",
"items": {
"type": "string"
Copy link
Contributor

@wking wking Nov 3, 2016

Choose a reason for hiding this comment

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

These should use the uri format:

"type": "string",
"format": "uri"

@coolljt0725
Copy link
Member Author

coolljt0725 commented Nov 3, 2016

@wking updated per your suggestions

wking
wking reviewed Nov 3, 2016
View reviewed changes
schema/content-descriptor.json Outdated
"type": "array",
"items": {
"type": "string",
"format": "uri"
Copy link
Contributor

@wking wking Nov 3, 2016

Choose a reason for hiding this comment

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

Inconsistent indent?

Copy link
Member Author

@coolljt0725 coolljt0725 Nov 3, 2016

Choose a reason for hiding this comment

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

Oh, sorry..

@coolljt0725 coolljt0725 force-pushed the urls branch 2 times, most recently from f88df27 to 6975241 Compare November 3, 2016 02:57
@coolljt0725
Copy link
Member Author

coolljt0725 commented Nov 3, 2016

@wking updated

@wking
Copy link
Contributor

wking commented Nov 3, 2016

Indents are still a bit off. And we do have a rule to fix them for you:

$ git describe --always
v1.0.0-rc1-99-g6975241
$ make fmt
$ git diff
diff --git a/schema/defs-image.json b/schema/defs-image.json
index 2a029ab..b0046a0 100644
--- a/schema/defs-image.json
+++ b/schema/defs-image.json
@@ -32,13 +32,13 @@
"digest": {
"$ref": "#definitions/digest"
},

  •   "urls": {

  •    "urls": {
     "description": "a list of urls from which this object may be downloaded",
     "type": "array",
     "items": {

  •       "type": "string",

  •       "format": "uri"

  •     }

  •        "type": "string",

  •        "format": "uri"

  •      }
   },
   "platform": {
     "id": "https://opencontainers.org/schema/image/platform",

@coolljt0725
specs-go: add missing field urls of descriptor
9208274 
Signed-off-by: Lei Jitang <leijitang@huawei.com>
@coolljt0725
Copy link
Member Author

coolljt0725 commented Nov 3, 2016

@wking updated, make fmt to fix the indents

@wking
Copy link
Contributor

wking commented Nov 3, 2016 via email

@wking wking mentioned this pull request Nov 3, 2016
Closed
@vbatts
Copy link
Member

vbatts commented Nov 3, 2016 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

wking added a commit to wking/image-spec that referenced this pull request Nov 3, 2016
@wking
descriptor: MUST URIs and SHOULD http(s) for descriptor's 'urls'
8729ba4 
The in-flight opencontainers#431 is using JSON Schema's uri format [1], which
requires RFC 3986.  This commit makes that an official spec
requirement by adding a MUST to the Markdown.  It also SHOULDs the
http(s) schemes, because they're widely supported and are deployed in
existing Docker images.  And Vincent and Antonio both like SHOULDing
them [2] ;).

[1]: https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-7.3.6
[2]: opencontainers#436

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking mentioned this pull request Nov 3, 2016
Merged
@jonboulle
Copy link
Contributor

jonboulle commented Nov 4, 2016 •
edited by caniszczyk
Loading

lgtm

Approved with PullApprove

@jonboulle jonboulle merged commit 334ad37 into opencontainers:master Nov 4, 2016
@coolljt0725 coolljt0725 deleted the urls branch November 5, 2016 01:18
wking added a commit to wking/image-spec that referenced this pull request Nov 5, 2016
@wking
descriptor: MUST URIs and SHOULD http(s) for descriptor's 'urls'
876489a 
The in-flight opencontainers#431 is using JSON Schema's uri format [1], which
requires RFC 3986.  This commit makes that an official spec
requirement by adding a MUST to the Markdown.  It also SHOULDs the
http(s) schemes, because they're widely supported and are deployed in
existing Docker images.  And Vincent and Antonio both like SHOULDing
them [2] ;).

[1]: https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-7.3.6
[2]: opencontainers#436

Signed-off-by: W. Trevor King <wking@tremily.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@wking wking wking left review comments

@jonboulle jonboulle jonboulle left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@coolljt0725 @wking @runcom @jonboulle @vbatts