make the image-tool work with -rc5 #144

zhouhao3 · 2017-05-08T02:54:41Z

This is a #121 follow-up PR, make the image-tool work with -rc5. Sorry so late update.

Signed-off-by: zhouhao [email protected]

vbatts · 2017-05-08T14:10:34Z

image/descriptor.go

@@ -48,19 +50,36 @@ func (d *descriptor) hash() string {
 	return pts[1]
 }

+// Index implementation
+type Index v1.ImageIndex


This is not really needed. Is there a particular reason to make a local type for this struct? I could end up complicating golang type assertions

I need this struct to carry the contents of index.json.

But why not just use v1.ImageIndex? You're not defining any methods so there should be no need to add this extra level of type indirection.

Oh I see, updated, thanks.

vbatts · 2017-05-08T14:12:13Z

image/descriptor.go

 			return nil
 		}

-		var d descriptor
-		if err := json.NewDecoder(r).Decode(&d); err != nil {
+		buf, err := ioutil.ReadAll(r)


doing a .ReadAll is a bit more expensive, where the previous version is a streaming decoder. I'd prefer a streaming decoder.

updated. PTAL.

vbatts · 2017-05-08T14:12:38Z

image/descriptor.go

 			return nil
 		}

-		if err := json.NewDecoder(r).Decode(&d); err != nil {
+		buf, err := ioutil.ReadAll(r)


runcom · 2017-05-10T08:46:21Z

@cyphar PTAL (need this for skopeo tests in containers/skopeo#313)

We need opencontainers/image-tools#144 first Signed-off-by: Antonio Murdaca <[email protected]>

zhouhao3 · 2017-05-23T08:37:04Z

What do you think of this PR? Some of the improvements in image-tools need to be done on this basis. @vbatts @runcom @cyphar @stevvooe @coolljt0725 @xiekeyang PTAL

cyphar · 2017-05-23T13:14:11Z

image/descriptor.go

+				d.MediaType = index.Manifests[i].Descriptor.MediaType
+				d.Digest = string(index.Manifests[i].Descriptor.Digest)
+				d.Size = index.Manifests[i].Descriptor.Size
+				refs[index.Manifests[i].Descriptor.Annotations["org.opencontainers.ref.name"]] = &d


This will completely flatten the set of references (silently ignoring duplicates). I've spent quite a while trying to come up with a sane API for umoci and I think I've decided the only way to implement this is to remove the concept of a reference in the lower-level libraries and have it as a higher-level construct.

In umoci this split was quite easy to do with oci/cas vs oci/casext which adds implementation-agnostic extensions to oci/cas. Maybe that's something you can do so you can remove the old concepts of a reference being a simple mapping.

Why are we making this so complicated? Just return a slice of descriptors.

yeah, and just append here.

@stevvooe ... because first-class references aren't a property of the underlying storage anymore ...

Whatever, I'm not going to get into another argument over this.

I try to use type descriptor v1.Descriptor instead of descriptor, but the following error occurs:

/tmp/go-build341304265/github.com/opencontainers/image-tools/image/_test/_obj_test/descriptor.go:47:cannot use d.Digest (type digest.Digest) as type string in argument to strings.SplitN
/tmp/go-build341304265/github.com/opencontainers/image-tools/image/_test/_obj_test/descriptor.go:81: cannot use &index.Manifests[i].Descriptor (type *v1.Descriptor) as type *descriptor in assignment

What I am using is what I can think of.As for the kind of ideas you say I want to do, but can not solve the problem after the amendment.

Whatever, I'm not going to get into another argument over this.

Please avoid the passive aggressive tone.

@q384566678 You need to convert to string: dgst.String() should do the trick. However, it also looks like this local descriptor type is no longer needed.

@stevvooe Sorry to disturb, I have solved other problems, only the problem can not be resolved.

descriptor.go:64: cannot use index.Manifests[i].Descriptor (type v1.Descriptor) as type descriptor in assignment

Or is there a problem with my method. Can we discuss this question? If can't solve the problem I think I still use the method in ea52a59. （ I think this method is feasible.）
I hope to also try to promote the development of image-tool, but sometimes some technical problems need your help.Thanks.

@q384566678 The problem is that there is a local type descriptor and a common type, v1.Descriptor. You either need to convert the types or just use the v1.Descriptor type.

Signed-off-by: zhouhao <[email protected]>

zhouhao3 · 2017-06-06T02:46:45Z

@runcom @stevvooe @vbatts @cyphar updated.PTAL

zhouhao3 · 2017-06-09T01:56:24Z

@opencontainers/image-tools-maintainers PTAL

zhouhao3 · 2017-06-13T01:11:49Z

@opencontainers/image-tools-maintainers I think this can be merged.PTAL, thanks.

stevvooe · 2017-06-13T18:56:55Z

image/descriptor.go

-	Digest    string `json:"digest"`
-	Size      int64  `json:"size"`
-}
+type descriptor v1.Descriptor


Why make this type definition? Just use the type.

Remove the type def please.

updated,I used to define this type because of this function,

func (d *descriptor) validate(w walker, mts []string) error {

and now I changed it to

func ValidateDescriptor(d *v1.Descriptor, w walker, mts []string) error {

PTAL

stevvooe · 2017-06-13T18:57:11Z

image/descriptor.go

-	Digest    string `json:"digest"`
-	Size      int64  `json:"size"`
-}
+type descriptor v1.Descriptor

 func (d *descriptor) algo() string {


Lost this code: this just go-digest to do this properly.

stevvooe · 2017-06-13T18:59:00Z

@q384566678 Rather than defining a new descriptor type, just use the existing one.

zhouhao3 · 2017-06-14T05:53:57Z

@stevvooe
If not, two descriptor cannot be assigned. （Type conversion is not ok. ）

descriptor.go:67: cannot convert &index.Manifests[i].Descriptor (type *v1.Descriptor) to type *descriptor

If you want to use the original definition as you say, you can only assign each structure separately in the original way:

d.MediaType = index.Manifests[i].Descriptor.MediaType
d.Digest = string(index.Manifests[i].Descriptor.Digest)
d.Size = index.Manifests[i].Descriptor.Size

stevvooe · 2017-06-14T20:12:44Z

@q384566678 The type definition is not needed. Remove the extra type and it will work. You may need to update a few to make it work.

stevvooe · 2017-06-15T21:56:19Z

image/descriptor.go

 	case errEOW:
 		return &d, nil
 	default:
 		return nil, err
 	}
 }

-func (d *descriptor) validate(w walker, mts []string) error {
+//ValidateDescriptor validate descriptor type
+func ValidateDescriptor(d *v1.Descriptor, w walker, mts []string) error {


No need to export this. It is use unexported interfaces, so it is pointless, anyways.

stevvooe · 2017-06-15T21:56:59Z

image/descriptor.go

@@ -105,7 +96,7 @@ func (d *descriptor) validate(w walker, mts []string) error {
 		return fmt.Errorf("invalid descriptor MediaType %q", d.MediaType)
 	}

-	parsed, err := digest.Parse(d.Digest)
+	parsed, err := digest.Parse(string(d.Digest))


No need to parse a digest that is already a digest. Just call d.Digest.Validate().

updated, PTAL, thanks.

stevvooe · 2017-06-16T20:28:45Z

image/image_test.go

 	}

-	parsed, err := digest.Parse(desc.Digest)
+	parsed, err := digest.Parse(string(desc.Digest))


desc.Digest.Validate()

updated, PTAL.
ping @opencontainers/image-tools-maintainers

Signed-off-by: zhouhao <[email protected]>

stevvooe · 2017-06-20T20:13:01Z

LGTM

zhouhao3 · 2017-06-21T01:08:12Z

@xiekeyang @coolljt0725 @vbatts PTAL.

cyphar · 2017-06-21T01:27:26Z

Do we want to add explicit validation for index.json?

LGTM otherwise, we can do that in a follow-up.

cyphar · 2017-06-21T01:31:40Z

@q384566678 Can you answer this question please?

Do we want to add explicit validation for index.json?

zhouhao3 · 2017-06-21T01:41:16Z

@cyphar I wrote a validation that contained the existence of index.json in #84 and the validation of index.json content in #51 (I will convert the validation of the manifest list to index.json.).

cyphar · 2017-06-21T06:10:57Z

👍

zhouhao3 force-pushed the up-validate branch 4 times, most recently from ad644c4 to 29d31ee Compare May 8, 2017 05:28

vbatts reviewed May 8, 2017

View reviewed changes

zhouhao3 force-pushed the up-validate branch 2 times, most recently from b8c8494 to ea52a59 Compare May 9, 2017 09:02

runcom added a commit to runcom/skopeo-1 that referenced this pull request May 10, 2017

integration: disable check with image-tools for image-spec RC5

55144fd

We need opencontainers/image-tools#144 first Signed-off-by: Antonio Murdaca <[email protected]>

runcom mentioned this pull request May 10, 2017

re-enable image-tools validation in tests containers/skopeo#338

Closed

runcom added a commit to runcom/skopeo-1 that referenced this pull request May 10, 2017

integration: disable check with image-tools for image-spec RC5

ff5117c

We need opencontainers/image-tools#144 first Signed-off-by: Antonio Murdaca <[email protected]>

runcom added a commit to runcom/skopeo-1 that referenced this pull request May 10, 2017

integration: disable check with image-tools for image-spec RC5

f4a44f0

We need opencontainers/image-tools#144 first Signed-off-by: Antonio Murdaca <[email protected]>

cyphar reviewed May 23, 2017

View reviewed changes

zhouhao3 force-pushed the up-validate branch 2 times, most recently from 4b1b706 to 06d86d2 Compare June 6, 2017 02:36

make the image-tool work with -rc5

6ff2eb1

Signed-off-by: zhouhao <[email protected]>

zhouhao3 force-pushed the up-validate branch from 06d86d2 to 6ff2eb1 Compare June 6, 2017 02:42

stevvooe reviewed Jun 13, 2017

View reviewed changes

zhouhao3 force-pushed the up-validate branch 2 times, most recently from fdb2505 to ba46dee Compare June 15, 2017 06:33

stevvooe reviewed Jun 15, 2017

View reviewed changes

zhouhao3 force-pushed the up-validate branch from ba46dee to 7913288 Compare June 16, 2017 01:24

stevvooe reviewed Jun 16, 2017

View reviewed changes

image: replace the extra function

017b697

Signed-off-by: zhouhao <[email protected]>

zhouhao3 force-pushed the up-validate branch from a7861dd to 017b697 Compare June 20, 2017 02:01

zhouhao3 merged commit 2d16afd into opencontainers:master Jun 21, 2017

zhouhao3 deleted the up-validate branch June 21, 2017 01:29

xiekeyang mentioned this pull request Jun 29, 2017

Bump version v0.2.0 #155

Merged

zhouhao3 mentioned this pull request Jan 9, 2018

image: code optimization #153

Merged

make the image-tool work with -rc5 #144

make the image-tool work with -rc5 #144

Conversation

zhouhao3 commented May 8, 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

Choose a reason for hiding this comment

runcom commented May 10, 2017

zhouhao3 commented May 23, 2017

cyphar May 23, 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

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 commented Jun 6, 2017

zhouhao3 commented Jun 9, 2017

zhouhao3 commented Jun 13, 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

stevvooe commented Jun 13, 2017

zhouhao3 commented Jun 14, 2017

stevvooe commented Jun 14, 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

stevvooe commented Jun 20, 2017 • edited by caniszczyk Loading

zhouhao3 commented Jun 21, 2017

cyphar commented Jun 21, 2017 • edited by caniszczyk Loading

cyphar commented Jun 21, 2017

zhouhao3 commented Jun 21, 2017

cyphar commented Jun 21, 2017

cyphar May 23, 2017 •

edited

Loading

stevvooe commented Jun 20, 2017 •

edited by caniszczyk

Loading

cyphar commented Jun 21, 2017 •

edited by caniszczyk

Loading