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.

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

image-layout: fix inconsistent description about external blob store #656

Merged
merged 1 commit into from
May 18, 2017
Merged

image-layout: fix inconsistent description about external blob store #656

merged 1 commit into from
May 18, 2017

Conversation

AkihiroSuda
Copy link
Member

Following sentences were conflicting:

  • A blob, referenced with digest <alg>:<hex> (per descriptor), MUST have its content stored in a file under blobs/<alg>/<hex>.
  • The blobs directory MAY be missing referenced blobs, in which case the missing blobs SHOULD be fulfilled by an external blob store.

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

wking
wking reviewed Apr 26, 2017
View reviewed changes
image-layout.md Outdated
@@ -53,7 +53,7 @@ afff3924849e458c5ef237db5f89539274d5e609db5db935ed3959c90f1f2d51 ./blobs/sha256/
## Blobs

* Object names in the `blobs` subdirectories are composed of a directory for each hash algorithm, the children of which will contain the actual content.
* A blob, referenced with digest `<alg>:<hex>` (per [descriptor](descriptor.md#digests-and-verification)), MUST have its content stored in a file under `blobs/<alg>/<hex>`.
* A blob, referenced with digest `<alg>:<hex>` (per [descriptor](descriptor.md#digests-and-verification)), SHOULD have its content stored in a file under `blobs/<alg>/<hex>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's weakening this too much, since it allows you to store <alg1>:<hex1> at blobs/<alg2>/<hex2> (or anywhere really). How about:

The content of blobs/<alg>/<hex> MUST match the digest <alg>:<hex>.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

wking
wking reviewed Apr 27, 2017
View reviewed changes
image-layout.md Outdated
* A blob, referenced with digest `<alg>:<hex>` (per [descriptor](descriptor.md#digests-and-verification)), MUST have its content stored in a file under `blobs/<alg>/<hex>`.
* The character set of the entry name for `<hex>` and `<alg>` MUST match the respective grammar elements described in [descriptor](descriptor.md#digests-and-verification).
* For example `sha256:5b` will map to the layout `blobs/sha256/5b`.
* The content of `blobs/<alg>/<encoded>` MUST match the digest `<alg>:<encoded>` (referenced per [descriptor](descriptor.md#digests-and-verification)). For example, the content of `blobs/sha256/5b` MUST match the digest `sha256:5b`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not use <encoded> here, since we currently require hex there. If/when #654 lands, it can update the <hex> reference.

It would also be nice to use a valid descriptor like sha256:da39a3ee5e6b4b0d3255bfef95601890afd80709, although since the current master uses sha256:5b, that would be a somewhat orthogonal fixup (but it seems like a fairly straightforward fix, so I thought I'd mention it in case you want to pick it up here).

Copy link
Member Author

Choose a reason for hiding this comment

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

#654 will land soon.
Even if #654 gets rejected, I don't think using <encoded> hurts here

wking
wking reviewed Apr 27, 2017
View reviewed changes
image-layout.md Outdated
* The character set of the entry name for `<hex>` and `<alg>` MUST match the respective grammar elements described in [descriptor](descriptor.md#digests-and-verification).
* For example `sha256:5b` will map to the layout `blobs/sha256/5b`.
* The content of `blobs/<alg>/<encoded>` MUST match the digest `<alg>:<encoded>` (referenced per [descriptor](descriptor.md#digests-and-verification)). For example, the content of `blobs/sha256/5b` MUST match the digest `sha256:5b`.
* The character set of the entry name for `<hex>` and `<encoded>` MUST match the respective grammar elements described in [descriptor](descriptor.md#digests-and-verification).
Copy link
Contributor

Choose a reason for hiding this comment

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

“<hex> and <encoded>” should be “<alg> and <hex>” (or “<alg> and <encoded>” in the post-#654 world).

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@AkihiroSuda
image-layout: fix inconsistent description about external blob store
f3cadcd 
Following sentences were conflicting:

 * A blob, referenced with digest `<alg>:<hex>` (per [descriptor](descriptor.md#digests-and-verification)), MUST have its content stored in a file under `blobs/<alg>/<hex>`.
 * The blobs directory MAY be missing referenced blobs, in which case the missing blobs SHOULD be fulfilled by an external blob store.

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda
Copy link
Member Author

rebased PTAL @stevvooe

@stevvooe
Copy link
Contributor

stevvooe commented May 10, 2017 •
edited by caniszczyk
Loading

LGTM

@AkihiroSuda Thank you, again!

Approved with PullApprove

@AkihiroSuda
Copy link
Member Author

@vbatts PTAL?

vbatts
vbatts reviewed May 16, 2017
View reviewed changes
image-layout.md
* A blob, referenced with digest `<alg>:<encoded>` (per [descriptor](descriptor.md#digests-and-verification)), MUST have its content stored in a file under `blobs/<alg>/<encoded>`.
* The character set of the entry name for `<encoded>` and `<alg>` MUST match the respective grammar elements described in [descriptor](descriptor.md#digests-and-verification).
* For example `sha256:5b` will map to the layout `blobs/sha256/5b`.
* The content of `blobs/<alg>/<encoded>` MUST match the digest `<alg>:<encoded>` (referenced per [descriptor](descriptor.md#digests-and-verification)). For example, the content of `blobs/sha256/da39a3ee5e6b4b0d3255bfef95601890afd80709` MUST match the digest `sha256:da39a3ee5e6b4b0d3255bfef95601890afd80709`.
Copy link
Member

Choose a reason for hiding this comment

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

oh interesting. So this will have to match the character set being discussed for URI charset

Copy link
Contributor

@wking wking May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So this will have to match the character set being discussed for URI charset

I think you're talking about #671 for “URI charset”, but #671 is about org.opencontainers.image.ref.name while this PR is about digests. The digest charset is still fairly restricted.

@vbatts
Copy link
Member

vbatts commented May 16, 2017 via email

@AkihiroSuda
Copy link
Member Author

LGTY? 😄

@vbatts
Copy link
Member

vbatts commented May 18, 2017 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

@vbatts vbatts merged commit b2b9246 into opencontainers:master May 18, 2017
@vbatts vbatts mentioned this pull request May 19, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbatts vbatts vbatts left review comments

@wking wking wking left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AkihiroSuda @stevvooe @vbatts @wking