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.

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

vendor: update dependencies #312

Closed
wants to merge 1 commit into from
Closed

Conversation

erikh
Copy link
Contributor

@erikh erikh commented Feb 21, 2017

This updates all the dependencies, but in particular fixes a dep for go-digest so it and containers/image can coexist.

Signed-off-by: Erik Hollensbe <github@hollensbe.org>
@runcom
Copy link
Member

runcom commented Feb 21, 2017

Looks good to me @erikh

@mtrmac
Copy link
Contributor

mtrmac commented Feb 21, 2017

I don’t think vendoring any version in skopeo can fix the vendor-caused type mismatch breaking #240EDIT containers/image#240.

We do need to figure out how to keep containers/image and skopeo’s vendor.conf in sync (and how to make sure they don’t stagnate on years-old versions), but that’s not quite the same thing I guess.

@erikh
Copy link
Contributor Author

erikh commented Feb 22, 2017

part of the big problem is that we're not using SHAs and instead tracking master. This causes trouble because we can't look back and see what master was (easily, at least) based on a revision history because it's been wiped out by vndr.

Another option here would be to eliminate the vendor directory before running skopeo tests; this might eliminate some problems, but not all.

I think the better solution is -- like you said -- to keep vendor.conf in sync. I wonder if we could source it from a gist or something?

@erikh
Copy link
Contributor Author

erikh commented Feb 22, 2017

Seems pretty lousy but it'd work! :D

@mtrmac
Copy link
Contributor

mtrmac commented Feb 22, 2017

part of the big problem is that we're not using SHAs and instead tracking master. This causes trouble because we can't look back and see what master was (easily, at least) based on a revision history because it's been wiped out by vndr.

We already have #304 for the digests vs. master discussion, let’s not duplicate it here please.

Another option here would be to eliminate the vendor directory before running skopeo tests; this might eliminate some problems, but not all.

Like, test skopeo against whatever happens to be in $GOPATH? I am probably misunderstanding.

I think the better solution is -- like you said -- to keep vendor.conf in sync. I wonder if we could source it from a gist or something?

Repeating myself from above, I don’t think that using the exact same version of go-digest fixes the mismatched type names between containers/image and skopeo. That can only be fixed by containers/image, when used in skopeo, not having its own vendored copy of go-digest, of any version of go-digest.

@erikh
Copy link
Contributor Author

erikh commented Feb 22, 2017 via email

@mtrmac
Copy link
Contributor

mtrmac commented Feb 22, 2017

I can confirm this patch fixes it.

To be clear, fixes what? I am referring to the recent failure in containers/image#240 :

cmd/skopeo/inspect.go:86: cannot assign "github.com/projectatomic/skopeo/vendor/github.com/containers/image/vendor/github.com/opencontainers/go-digest".Digest to outputData.Digest (type "github.com/projectatomic/skopeo/vendor/github.com/opencontainers/go-digest".Digest) in multiple assignment

This is a simple type Digest string in both packages / versions, this is not an interface mismatch which can be solved by bumping either of the dependencies; the issue is that the type command has created a named type incompatible with any other named type, and both projects having independent vendoring causes them to create two different named types.

And I have posted the suggestion twice in containers/image#240:

When test-skopeo copies containers/image into skopeo’s vendor subdirectory, it must not copy containers/image/vendor in there as well.

@erikh
Copy link
Contributor Author

erikh commented Feb 25, 2017

I think this is resolved; do we need to keep this one around?

@mtrmac
Copy link
Contributor

mtrmac commented Feb 27, 2017

If this was intended to fix the digest.Digest type mismatches in #240, I think #240 has fixed it the right way and this is not necessary. Or was this intended to fix something else?

@erikh
Copy link
Contributor Author

erikh commented Feb 27, 2017

Will close for now, we can always re-open if necessary.

@erikh erikh closed this Feb 27, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants