image: Port to go-digest #104

wking · 2017-01-20T00:36:08Z

This is the smallest pivot I could think up (replacing old sha256 imports with go-digest). But I want the go-digest vendoring to land so I can rebase #5 and #40 on top of it. And I expect folks who currently prefer other methods for cleaning up the internal image/* code are also interested in go-digest, so I'm filing this PR to get a base that we hopefully all agree on ;).

Mashimiao · 2017-01-20T09:58:00Z

Except need reabase, looks good to me

wking · 2017-01-20T15:54:45Z

On Fri, Jan 20, 2017 at 01:58:01AM -0800, Ma Shimiao wrote: Except need reabase, looks good to me

Rebased around #102 with 2a40e7b → c94e7cb.

coolljt0725 · 2017-01-22T02:49:12Z

@wking should we also use digest type in descriptor ?

diff --git a/image/descriptor.go b/image/descriptor.go
index 341b65c..eac9279 100644
--- a/image/descriptor.go
+++ b/image/descriptor.go
@@ -24,13 +24,14 @@ import (
        "path/filepath"
        "strings"

+       digest "github.com/opencontainers/go-digest"
        "github.com/pkg/errors"
 )

 type descriptor struct {
-       MediaType string `json:"mediaType"`
-       Digest    string `json:"digest"`
-       Size      int64  `json:"size"`
+       MediaType string        `json:"mediaType"`
+       Digest    digest.Digest `json:"digest"`
+       Size      int64         `json:"size"`
 }

wking · 2017-01-22T03:16:46Z

should we also use digest type in descriptor ?

I think we should be using the image-spec type directly (and opencontainers/image-spec#514 is in flight to make that change there). But either way, that sounds like something we can do in a follow-up PR.

xiekeyang · 2017-01-22T08:34:26Z

@wking you might had better to adjust the commits order, and squash ad0de71, 6cdd607 and c94e7cb to one. That make sure the building passed after resetting to each commit.

wking · 2017-01-22T15:28:38Z

you might had better to adjust the commits order, and squash ad0de71, 6cdd607 and c94e7cb to one.

I can't get the go-digest vendor to stick before I add the consuming code, even with the entry in glide.yaml. Is there a glide invocation I'm missing? I'd rather not mix hand-written and auto-generated changes in a single commit.

xiekeyang · 2017-01-23T01:24:41Z

It should be compile-able after reset to 37624b0.

wking · 2017-01-23T04:52:44Z

On Sun, Jan 22, 2017 at 05:24:42PM -0800, xiekeyang wrote: It should be compile-able after reset to 37624b0.

I agree on the goal. I'm just not clear on how to accomplish that without squashing this PR down into a single commit (after which it would be harder to distinguish the code I wrote (which probably needs manual review) from the code glide injected (which can be reviewed by re-running glide and checking for differences). Looking at past updates, handling everthing in a single commit seems to be the established procedure (#69, #102), but I'd rather avoid that if possible. However, I have found no way to install a single package with glide(-vc) before we have local code that consumes the package. Maybe this is impossible without Masterminds/glide#252? I'll leave this as it stands for a day or two to see if any glide gurus know the proper incantation, and then squash it down to a single commit if nobody has a better idea. If maintainers want to land this PR in the meantime, they can always use GitHub's squash-and-merge [1]. [1]: https://help.github.com/articles/about-merge-methods-on-github/#squashing-your-merge-commits

xiekeyang · 2017-01-23T11:46:24Z

@wking Please take account my proposal, to squash glide and dependency code to one commit, and keep your writing code as one. That may be better for rolling back. Anyway it is just an alternative.

I debug the PR, it works correctly. The PR mostly looks good to me.

I haven't been able to find a glide invocation to add an unused dependency, so I generated this commit by committing the consuming code (image: Port to go-digest), running update-deps, and then using rebase to swap the order of the two commits: ...add go-digest conumers... $ git commit -sm 'image: Port to go-digest...' $ make update-deps $ git add vendor/github.com/opencontainers/go-digest $ git commit -sm 'vendor/github.com/opencontainers/go-digest: Add dependency...' $ git rebase -i HEAD^^ ...swap the order of the two commits... That way we have the vendored library if someone checks out the consuming commit directly [1]. [1]: opencontainers#104 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2017-01-23T17:32:08Z

On Mon, Jan 23, 2017 at 03:46:24AM -0800, xiekeyang wrote: @wking Please take account my proposal, to squash glide and dependency code to one commit, and keep your writing code as one.

This is a bit awkward: 1. Add your consuming code and commit. 2. Run update-deps to get glide to vendor the new dependency and commit. 3. Rebase to put the commit from 2 in front of the commit from 1. but I guess it's a workable way to get glide to do what I need. I've rebased this commit to do that with c94e7cb → 4cf2903. It still doesn't squash the three glide commits together (because each of them is doing a separate glide task), but it does compile successfully at each commit.

xiekeyang · 2017-01-24T01:31:40Z

LGTM

cc @opencontainers/image-tools-maintainers

xiekeyang · 2017-02-07T02:10:03Z

@vbatts @philips @stevvooe Is this PR OK?

coolljt0725 · 2017-02-07T02:11:48Z

@wking let's wait #107 to bump the image-spec and then rebase this pr?

stevvooe · 2017-02-07T04:01:43Z

image/descriptor.go

@@ -116,15 +115,18 @@ func (d *descriptor) validate(w walker, mts []string) error {
 }

 func (d *descriptor) validateContent(r io.Reader) error {
-	h := sha256.New()
-	n, err := io.Copy(h, r)
+	parsed, err := digest.Parse(d.Digest)


Just use the OCI descriptor type.

I'd love to (and I do that in #5). However, as I pointed out earlier, that would require making this PR larger (and therefore harder to review). I'll gladly file a follow up PR with that change if we can land this initial go-digest consuming code first.

wking · 2017-02-07T05:28:13Z

On Mon, Feb 06, 2017 at 06:11:48PM -0800, Lei Jitang wrote: @wking let's wait #107 to bump the image-spec and then rebase this pr?

#107 landed, so I've rebased around it. Interestingly, ff477b3 was able to magically vendor go-digest without writing any consuming code, although I had been unable to get that working myself [1]. @q384566678, do you remember how you accomplished it? [1]: #104 (comment)

zhouhao3 · 2017-02-07T05:47:50Z

do you remember how you accomplished it?

@wking I thought maybe because #537 of merged, then I modified version: v1.0.0-rc4.
Then I executed the make update-deps command

vbatts · 2017-02-08T19:01:40Z

LGTM

stevvooe · 2017-02-08T22:08:08Z

LGTM

vbatts · 2017-02-08T22:14:54Z

bugger. Needs a rebase.

wking · 2017-02-08T23:17:43Z

On Wed, Feb 08, 2017 at 02:14:55PM -0800, Vincent Batts wrote: Needs a rebase.

Rebased around #65 with d2241d5 → 83f63c7.

coolljt0725 · 2017-02-09T01:11:09Z

ping @wking CI failed

Outsource this stuff to avoid duplication of effort. newDescriptor was returning just the hex and createHashedBlob (its only consumer) was fixing that (by the "Normalize the hashed digest" comment) so it didn't have to split the hash (Hex) back off. But that seems confusing to me, so I've fixed newDescriptor to create a non-busted digest which we split back apart in createHashedBlob. Signed-off-by: W. Trevor King <wking@tremily.us>

By editing glide.yaml and then running: $ make update-deps to update the hash. This way we can bump go-digest intentionally when we feel so inclined. Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2017-02-09T05:52:50Z

On Wed, Feb 08, 2017 at 05:11:09PM -0800, Lei Jitang wrote: ping @wking CI failed

Thanks for the ping, it was a sloppy rebase. Should be fixed with 83f63c7 → adb0766.

xiekeyang · 2017-02-09T06:02:38Z

LGTM

coolljt0725 · 2017-02-09T06:22:48Z

LGTM

wking force-pushed the go-digest branch from ec4e7c2 to 2a40e7b Compare January 20, 2017 00:51

wking force-pushed the go-digest branch from 2a40e7b to c94e7cb Compare January 20, 2017 15:52

wking force-pushed the go-digest branch from c94e7cb to 4cf2903 Compare January 23, 2017 17:28

stevvooe reviewed Feb 7, 2017

View reviewed changes

wking force-pushed the go-digest branch from 4cf2903 to d2241d5 Compare February 7, 2017 05:23

wking force-pushed the go-digest branch from d2241d5 to 83f63c7 Compare February 8, 2017 23:14

wking added 2 commits February 8, 2017 21:50

glide.yaml: Lock go-digest to v1.0.0-rc0

adb0766

By editing glide.yaml and then running: $ make update-deps to update the hash. This way we can bump go-digest intentionally when we feel so inclined. Signed-off-by: W. Trevor King <wking@tremily.us>

wking force-pushed the go-digest branch from 83f63c7 to adb0766 Compare February 9, 2017 05:50

coolljt0725 merged commit 9386a7f into opencontainers:master Feb 9, 2017

xiekeyang mentioned this pull request Feb 16, 2017

Bump version for v0.1.0 #116

Merged

wking mentioned this pull request Feb 16, 2017

image/cas: Add a generic CAS interface #40

Open

wking deleted the go-digest branch February 16, 2017 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image: Port to go-digest #104

image: Port to go-digest #104

wking commented Jan 20, 2017

Mashimiao commented Jan 20, 2017

wking commented Jan 20, 2017 via email

coolljt0725 commented Jan 22, 2017 •

edited

Loading

wking commented Jan 22, 2017

xiekeyang commented Jan 22, 2017 •

edited

Loading

wking commented Jan 22, 2017 •

edited

Loading

xiekeyang commented Jan 23, 2017

wking commented Jan 23, 2017 via email

xiekeyang commented Jan 23, 2017

wking commented Jan 23, 2017 via email

xiekeyang commented Jan 24, 2017 •

edited by caniszczyk

Loading

xiekeyang commented Feb 7, 2017

coolljt0725 commented Feb 7, 2017

stevvooe Feb 7, 2017

wking Feb 7, 2017

wking commented Feb 7, 2017 via email

zhouhao3 commented Feb 7, 2017

vbatts commented Feb 8, 2017 •

edited by caniszczyk

Loading

stevvooe commented Feb 8, 2017 •

edited by caniszczyk

Loading

vbatts commented Feb 8, 2017

wking commented Feb 8, 2017 via email

coolljt0725 commented Feb 9, 2017

wking commented Feb 9, 2017 via email

xiekeyang commented Feb 9, 2017 •

edited by caniszczyk

Loading

coolljt0725 commented Feb 9, 2017 •

edited by caniszczyk

Loading

image: Port to go-digest #104

image: Port to go-digest #104

Conversation

wking commented Jan 20, 2017

Mashimiao commented Jan 20, 2017

wking commented Jan 20, 2017 via email

coolljt0725 commented Jan 22, 2017 • edited Loading

wking commented Jan 22, 2017

xiekeyang commented Jan 22, 2017 • edited Loading

wking commented Jan 22, 2017 • edited Loading

xiekeyang commented Jan 23, 2017

wking commented Jan 23, 2017 via email

xiekeyang commented Jan 23, 2017

wking commented Jan 23, 2017 via email

xiekeyang commented Jan 24, 2017 • edited by caniszczyk Loading

xiekeyang commented Feb 7, 2017

coolljt0725 commented Feb 7, 2017

stevvooe Feb 7, 2017

Choose a reason for hiding this comment

wking Feb 7, 2017

Choose a reason for hiding this comment

wking commented Feb 7, 2017 via email

zhouhao3 commented Feb 7, 2017

vbatts commented Feb 8, 2017 • edited by caniszczyk Loading

stevvooe commented Feb 8, 2017 • edited by caniszczyk Loading

vbatts commented Feb 8, 2017

wking commented Feb 8, 2017 via email

coolljt0725 commented Feb 9, 2017

wking commented Feb 9, 2017 via email

xiekeyang commented Feb 9, 2017 • edited by caniszczyk Loading

coolljt0725 commented Feb 9, 2017 • edited by caniszczyk Loading

coolljt0725 commented Jan 22, 2017 •

edited

Loading

xiekeyang commented Jan 22, 2017 •

edited

Loading

wking commented Jan 22, 2017 •

edited

Loading

xiekeyang commented Jan 24, 2017 •

edited by caniszczyk

Loading

vbatts commented Feb 8, 2017 •

edited by caniszczyk

Loading

stevvooe commented Feb 8, 2017 •

edited by caniszczyk

Loading

xiekeyang commented Feb 9, 2017 •

edited by caniszczyk

Loading

coolljt0725 commented Feb 9, 2017 •

edited by caniszczyk

Loading