Modify the ref option #169

zhouhao3 · 2017-07-31T08:22:05Z

Increase the search conditions, you can better retrieve descriptor.
Resolved when the search is not unique or non-existent.
cc/ @opencontainers/image-tools-maintainers

Fixes #164
Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

wking · 2017-07-31T13:38:13Z

image/descriptor.go

 			}
+		case "digest":


Walking the index to find an entry pointing at a given digest seems odd. Doing that in findDescriptor seems fine, but create and unpack seem like they could skip straight over index.json into CAS resolution when you know the digest and media type (although you'd need to use --type or a new option to get the media type).

I don't think so. when you get a given digest, we also need to verify that it conforms to the specification by validateDescriptor.

I don't think so. when you get a given digest, we also need to verify that it conforms to the specification by validateDescriptor.

Sure, if you're validating a ref. But if you're just trying to unpack an image, and you have the digest and media type (from --type), I don't see why you'd need to hit anything outside CAS.

What do you think about this change.

What do you think about this change.

My problem wasn't with validateDescriptor for digest== ref names. If we get a media type from --type and have a digest==… name, I think we can skip around all of this and never hit anything outside of CAS in unpack.

The type obtained by --type is only the file type of create or unpack ( imageLayout or image). And the type of digest obtained here is manifest or index. We can't get the media type corresponding to digest through --type, only through findDescriptor to confirm the media type.

Mashimiao · 2017-08-04T03:31:15Z

This PR is partly duplicated with #89

zhouhao3 · 2017-08-15T06:17:01Z

@coolljt0725 @stevvooe PTAL

zhouhao3 · 2017-08-18T03:27:21Z

ping @coolljt0725 @xiekeyang @stevvooe

stevvooe · 2017-08-22T00:35:43Z

cmd/oci-image-tool/create.go

@@ -85,8 +85,8 @@ var createCommand = cli.Command{
 		},
 		cli.StringFlag{
 			Name:  "ref",
-			Value: "v1.0",
-			Usage: "The ref pointing to the manifest of the OCI image. This must be present in the 'refs' subdirectory of the image.",
+			Value: "org.opencontainers.image.ref.name==v1.0",


Is this the default value? Why is the default value v1.0?

Is this the default value?

Yes.

Why is the default value v1.0?

Just follow the previous default value.

zhouhao3 · 2017-08-25T06:27:32Z

@wking @stevvooe @xiekeyang @coolljt0725 @vbatts PTAL

zhouhao3 · 2017-08-28T03:24:04Z

@wking @coolljt0725 @xiekeyang @stevvooe @cyphar PTAL

zhouhao3 · 2017-08-31T05:43:07Z

reping @stevvooe @wking @coolljt0725 @cyphar @xiekeyang @vbatts I need your advices.

zhouhao3 · 2017-09-07T01:44:41Z

Bump.

coolljt0725 · 2017-09-07T07:16:32Z

man/oci-image-tool-create.1.md

-  The ref pointing to the manifest of the OCI image. This must be present in the "refs" subdirectory of the image. (default "v1.0")
+  Specify the search criteria, format is A==B.
+  e.g. --ref org.opencontainers.image.ref.name==v1.0
+  Only support `org.opencontainers.image.ref.name`, `platform.os` and `digest` three cases.(default org.opencontainers.image.ref.name==v1.0)


use == seems odd to me, can we just use =? And another question, does it support the old way --ref=latest？ or we should use org.opencontainers.image.ref.name=latest to specify a ref ?

And if we have multiple images for a platform.os, does all these images will be unpack?

And I think in this way, we change the semantic meaning of the ref, the ref supposed to representing a tag of image.

use == seems odd to me, can we just use =?

updated.

And another question, does it support the old way --ref=latest？ or we should use org.opencontainers.image.ref.name=latest to specify a ref ?

use org.opencontainers.image.ref.name=lates

And if we have multiple images for a platform.os, does all these images will be unpack?

Yes, all images conforming to the platform. OS will be unpack.

Yes, all images conforming to the platform. OS will be unpack.

Is that a desired behavior ? Will these images be unpack to different rootfs ?

I think maybe something is not clear:
unpack at a time can only be an image or imageLayout unpack. When ref point to the index, then go to the platform.os to filter manifest, and then according to the manifests to unpack to the rootfs, I think your question is not that if the match of the manifest there are many, should not unpack to the same rootfs?
If this is the problem, I do not know is not able to unpack to the same file, if not, it should increase the screening of the manifests to determine.

There could be multiple images with different ref for a specific platform in an image or imageLayer, My question is if platform.os=xx match multiple image, when you do unpack or create bundle, Will these images will be unpacked to same rootfs? That's not a desired behavior.

Sorry. I did not understand it before.

My question is if platform.os=xx match multiple image, when you do unpack or create bundle, Will these images will be unpacked to same rootfs?

No. If platform.os=xx match multiple image, The program will be error.

zhouhao3 · 2017-09-18T01:11:02Z

ping @wking @xiekeyang @stevvooe @vbatts @cyphar

coolljt0725 · 2017-09-18T01:32:45Z

I just think org.opencontainers.image.ref.name is tool long for user to type it, can we make a short name like --ref name=xx? or just keep the old way --ref xxx to stand for --ref org.opencontainers.image.ref.name=xxx?

zhouhao3 · 2017-09-18T02:24:09Z

@coolljt0725 updated. Thanks for your advices.

coolljt0725 · 2017-09-18T03:51:04Z

with this patch when I validated a image, the output is

$ oci-image-tool validate busybox/
oci-image-tool: reference "latest": OK
oci-image-tool: reference "linux": OK
oci-image-tool: reference "sha256:40a114053d955a2b80ee2cf6e13410b28b59594ceee9036b41e12c42d3e16615": OK
busybox/: OK
Validation succeeded

I think it's a bit of redundant and make user confuse,

and I use oci-image-tool validate --ref name=latest busybox/ , it doesn't work,

oci-image-tool validate  --ref name=latest busybox/
1 errors detected:
busybox/: validation failed: reference name=latest not found

So the oci-image-tool validate has a different UI for --ref? It's better to keep consistent.

zhouhao3 · 2017-11-02T01:51:45Z

ping @opencontainers/image-tools-maintainers

zhouhao3 · 2017-11-11T07:44:49Z

@opencontainers/image-tools-maintainers @vbatts @coolljt0725 @stevvooe @Mashimiao
I updated this Pr, and now I change the following:

Find reference more accurately through multiple ref inputs

The following is the specific operation results:

oci-image-tool validate --ref name=latest --ref platform.os=linux ubuntu.tar 
oci-image-tool: reference [name=latest platform.os=linux]: OK
ubuntu.tar: OK
Validation succeeded

Mashimiao · 2017-11-13T06:28:04Z

image/descriptor.go

-				d = index.Manifests[i]
-				return errEOW
+		descs = index.Manifests
+		for _, name := range names {


The following code is totally duplicate with part code of validate() in image/image.go
Maybe we can create a common or utility function by taking advantage of it.

updated, PTAL.

Mashimiao · 2017-11-23T02:14:38Z

LGTM

coolljt0725 · 2017-11-24T02:53:46Z

ping @opencontainers/image-tools-maintainers PTAL. This is a change on UI, I think it needs more lgtm

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

stevvooe · 2018-01-02T22:55:40Z

It would be good to summarize the change. Does this now allow one to ignore parts of the layout by specifying a ref? What is the use for only validating one part of an image?

Is there an updated proposal?

zhouhao3 · 2018-01-03T02:20:41Z

Does this now allow one to ignore parts of the layout by specifying a ref?

I don't quite understand what you mean. What does parts of the layout mean?
I think the only thing that's been ignored is the manifest that doesn't match the lookup information when the validation is performed.

What is the use for only validating one part of an image?

For the image validation process, I just continued the previous practice: Verify the specified manifest according to the ref.

In my opinion, this patch is no problem and can be merged.

zhouhao3 · 2018-01-24T01:37:37Z

This patch dragged for a long time, I will merge it first, what problems can be amended later.

stevvooe · 2018-01-26T00:13:45Z

@q384566678 Did this patch have the requisite LGTMs from maintainers to proceed? Questions in #169 (comment) were not resolved.

zhouhao3 · 2018-01-26T03:18:13Z

Did this patch have the requisite LGTMs from maintainers to proceed?

According to the latest rules, a LGTM can merge patches. This patch waits for a long time, and no one else to review, so I can only merge it first.

Questions in #169 (comment) were not resolved.

As for the question you said, I have already given a reply below because I did not understand something you said and you did not reply to me. I think if there are other problems can be repaired in the next pr.

Please forgive me if there is anything wrong with it.

stevvooe · 2018-01-26T21:33:12Z

I reviewed it and asked for a summary of the changes: "It would be good to summarize the change." Just because it takes a long time doesn't mean it is okay to merge.

I have no clue what is wrong with this because there is no proposal describing the end result of what this PR is attempting to accomplish.

zhouhao3 · 2018-01-30T05:24:09Z

The purpose of this pr is to solve the problem that the descriptor is not unique when searching descriptor by the input conditions. After the change can enter multiple conditions to search, to ensure that the search descriptor is unique.
So what do you think there is something to change? If there is any problem, I will modify it.

zhouhao3 mentioned this pull request Jul 31, 2017

How to handle multiple manifests with the same ref.name #164

Closed

zhouhao3 force-pushed the select-descs branch from 64a1893 to d189863 Compare July 31, 2017 08:37

wking reviewed Jul 31, 2017

View reviewed changes

zhouhao3 force-pushed the select-descs branch 2 times, most recently from d549631 to 3cdbaa7 Compare August 1, 2017 09:14

zhouhao3 force-pushed the select-descs branch 8 times, most recently from 7d49b49 to 4aa4df9 Compare August 15, 2017 06:07

stevvooe reviewed Aug 22, 2017

View reviewed changes

zhouhao3 force-pushed the select-descs branch from 4aa4df9 to 74f5081 Compare August 25, 2017 06:15

zhouhao3 added the help wanted label Aug 31, 2017

coolljt0725 reviewed Sep 7, 2017

View reviewed changes

zhouhao3 force-pushed the select-descs branch from 74f5081 to 263330a Compare September 7, 2017 07:31

zhouhao3 force-pushed the select-descs branch from 263330a to f758639 Compare September 18, 2017 02:20

zhouhao3 force-pushed the select-descs branch 3 times, most recently from 9198a55 to 752e33a Compare November 11, 2017 07:12

zhouhao3 force-pushed the select-descs branch 5 times, most recently from 2a9a780 to 612d162 Compare November 13, 2017 01:47

Mashimiao reviewed Nov 13, 2017

View reviewed changes

zhouhao3 force-pushed the select-descs branch from 612d162 to 60a93f0 Compare November 13, 2017 08:20

Mashimiao added enhancement and removed help wanted labels Nov 23, 2017

Modify the ref optiont

c3ac69c

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

zhouhao3 force-pushed the select-descs branch 2 times, most recently from 43a1e42 to c3ac69c Compare December 27, 2017 01:41

zhouhao3 merged commit dab5ba5 into opencontainers:master Jan 24, 2018

zhouhao3 deleted the select-descs branch January 24, 2018 01:38

This was referenced Jan 24, 2018

Modify the ref information #89

Closed

Bump version to v1.0.0-rc1 #198

Merged

Modify the ref option #169

Modify the ref option #169

Conversation

zhouhao3 commented Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

zhouhao3 Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Aug 4, 2017

zhouhao3 commented Aug 15, 2017

zhouhao3 commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouhao3 commented Aug 25, 2017 • edited Loading

zhouhao3 commented Aug 28, 2017

zhouhao3 commented Aug 31, 2017

zhouhao3 commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouhao3 Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

zhouhao3 commented Sep 18, 2017

coolljt0725 commented Sep 18, 2017

zhouhao3 commented Sep 18, 2017 • edited Loading

coolljt0725 commented Sep 18, 2017

zhouhao3 commented Nov 2, 2017

zhouhao3 commented Nov 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Nov 23, 2017 • edited by caniszczyk Loading

coolljt0725 commented Nov 24, 2017

stevvooe commented Jan 2, 2018

zhouhao3 commented Jan 3, 2018

zhouhao3 commented Jan 24, 2018

stevvooe commented Jan 26, 2018

zhouhao3 commented Jan 26, 2018

stevvooe commented Jan 26, 2018

zhouhao3 commented Jan 30, 2018 • edited Loading

zhouhao3 commented Jul 31, 2017 •

edited

Loading

zhouhao3 Aug 1, 2017 •

edited

Loading

zhouhao3 commented Aug 25, 2017 •

edited

Loading

zhouhao3 Sep 7, 2017 •

edited

Loading

zhouhao3 commented Sep 18, 2017 •

edited

Loading

zhouhao3 commented Nov 11, 2017 •

edited

Loading

Mashimiao commented Nov 23, 2017 •

edited by caniszczyk

Loading

zhouhao3 commented Jan 30, 2018 •

edited

Loading