Clean overridden dependencies #150
Conversation
|
Not sure what Azure Pipelines is doing, cancelled the build and reran it, it's green now - https://dev.azure.com/deislabs/cnab-go/_build/results?buildId=3919 |
|
Also, we should probably consider adding the |
There was a problem hiding this comment.
Thanks for doing this @radu-matei ! Looks good for me on go version go1.12.9 darwin/amd64 after a rm -rf vendor/ && dep ensure && make build test cycle.
Had one proposal, but definitely understand if we wish it to be a follow-up.
| go-tests = true | ||
| [[override]] | ||
| name = "github.com/google/go-containerregistry" | ||
| revision = "ef12d49c8daf6a6d72978966d5945e39bb898b4a" |
There was a problem hiding this comment.
I'm wondering if we can override with a more recent commit for this repo? Though, perhaps this was pinned at the inherited commit on master?
google/go-containerregistry@0e086ae brings in a fix, courtesy @glyn, related to supporting pushing images (or indexes) with differing media types, which Porter hit with a use case mentioned in vmware-archive/image-relocation#34
Add osusergo build tag for static compilation Override x/sys because of Windows unsafe pointer bug Override go-containerregistry Signed-off-by: Radu M <root@radu.sh>
radu-matei
changed the title
|
|
||
| [[override]] | ||
| name = "golang.org/x/sys" | ||
| revision = "f49334f85ddcf0f08d7fb6dd7363e9e6d6b777eb" | ||
|
|
||
| [prune] |
There was a problem hiding this comment.
Why were the prune options removed? If you are thinking about checking in vendor eventually, then we should be keeping the prune options.
| [[constraint]] | ||
| name = "github.com/docker/cli" | ||
| revision = "f95ca8e1ba6c22c9abcdbf65e8dcc39c53958bba" | ||
|
|
||
| [[override]] |
There was a problem hiding this comment.
overrides without constraints are not communicated to people who depend on your library. So other people who depend upon cnab-go won't get any help from dep on these dependencies, and won't know that containerd for example should be at version 1.3.0.
Each override needs to be paired with a matching constraint for downstream consumers of the library.
There was a problem hiding this comment.
Sorry I forgot to mention the other part of the trick to let you constrain a transitive dependency with dep:
https://github.com/deislabs/porter/blob/master/pkg/dep/dep.go
It's a total hack, sorry you can blame google for killing the project before we got a chance to finish...
Anyhoo, you add a file like that in an out of the way package to get dep to consider it direct dependency. Now you can add constraints, and communicate your dependencies to your downstream consumers properly.
There was a problem hiding this comment.
I think you can get away with removing the overrides entirely with that this workaround. Give it a try.
Then in the downstream consumer's Gopkg.toml they can reference cnab-go without having to add a bunch of overrides and constraints because cnab-go did all the proper work up-front. That's what porter has been doing with its mixin repos.
| [[override]] | ||
| name = "k8s.io/api" | ||
| revision = "kubernetes-1.11.2" | ||
| #version = "v2.7.1" |
There was a problem hiding this comment.
Why are we using that revision instead of v2.7.1? That commented out line is a bit misleading. Whenever we do pin to a specific revision it really helps to explain why in the file, so that people can understand when it's safe to move back to a tag, what issue they should be watching and waiting for it to be merged, etc.
|
I agree that a follow-up PR where we commit vendor would be appreciated. 👍 |
|
@radu-matei sorry for the churn, can you rebase this PR now that #161 is merged? |
|
Closing now that we use go mods. |
depends on cnabio/cnab-to-oci#77
ref #121
When that PR is merged, we should update this PR, do a release of
cnab-to-oci, then a release ofcnab-go.There are still some dependencies that are still overridden:
without the
x/sysoverride:github.com/docker/docker/pkg/system/filesys_windows.go:112:24: cannot use uintptr(unsafe.Pointer(&sd[0])) (type uintptr) as type *"github.com/deislabs/cnab-go/vendor/golang.org/x/sys/windows".SECURITY_DESCRIPTOR in assignmentgo-containerregistry- I got this fromduffle- I think it relates toimage-relocation- @glyn might be able to help`containerd,distribution,docker-credential-helpers- this helps pin compatible versions acrosscnab-to-oci,cli,client,docker/docker. Feel free to try turning these into constraints rather than overrides, I didn't have any luck so far.Please test this on all operating systems and supported Go versions - I also added the
osusergobuild tag (see golang/go#23265) - without it, cross compilation on Darwin fails.