.circleci/config.yml vendor gnostic for k8s.io/client-go #585
Conversation
41c6e51 to
b3dd239
Compare
b3dd239 to
d244412
Compare
.circleci/config.yml
Outdated
| # k8s.io/client-go package, because it won't build with the | ||
| # current master. | ||
| command: > | ||
| git clone --branch v0.4.0 |
There was a problem hiding this comment.
It doesn't seem to matter in this case, but client-go uses 0.1.0 as their minimum link
There was a problem hiding this comment.
I chose the maximum, 0.4.0, that is before the breaking change. Since go get by default checks out the most recent, this is the closest version to what was already being built.
|
The "better ways" for having a stable CI involve modules or another form of dependency management instead of "temporary" cloning/vendoring. But a perfectly stable CI doesn't necessarily reflect how things are for users, who may have updated to newer things than the versions we've pinned. Since CI breakage regularly affects PRs from maintainers and contributors, it's worth considering a new approach to this. |
There was a problem hiding this comment.
The comments don't make it clear why we need to do this. Can you please explain (either here in the PR or in the comments) ? "vendor [this] because it won't build with the current master" doesn't seem like a good explanation.
Will this work for users checking things out? When someone does go get gopkg.in/DataDog/dd-trace-go.v1/... will things break? What is the actual problem here? I think we need a good thorough PR description.
|
Sorry about that, @gbbr. I've added a detailed description to the PR. |
There was a problem hiding this comment.
I'd like to request changes:
- The comments (in the
yamlfile) are vague and don't make it clear why the steps are there. - We have two steps when we only need one. Both are addressing the same underlying issue and having them separate can be confusing.
- We might not even need the second step and should use the suggested solution in Lowercase openapiv2 imports kubernetes/client-go#741.
8092bd5 to
35008b9
Compare
Co-Authored-By: Gabriel Aszalos <gabriel.aszalos@gmail.com>
…ring when the upstream issue is resolved.
k8s.io/client-go has github.com/googleapis/gnostic as a dependency. Recently, gnostic renamed some packages to be consistent with go style (google/gnostic#155) This change breaks client-go, which will no longer build when clients are checked out with e.g. go get k8s.io/client-go/kubernetes. This break in k8s.io/client-go is causing our CI to fail. This commit addresses the issue by checking out k8s.io/client-go manually and vendoring the last working version of gnostic in k8s.io/client-go/vendor.
k8s.io/client-gohasgithub.com/googleapis/gnosticas a dependency.Recently, gnostic renamed some packages to be consistent with go style (google/gnostic#155)
This change breaks
client-go, which will no longer build when clients are checked out with e.g.go get k8s.io/client-go/kubernetes.k8s.io/client-gohas not made it clear when/if they will fix their imports. They are closing issues opened against the repository with instructions on how to use go modules to circumvent the problem (kubernetes/client-go#741, kubernetes/client-go#743)go get gopkg.in/DataDog/dd-trace-go.v1/...still works as expected. Unit tests still run, and I am still able to build against dd-trace-go. I have not tried building a program using thecontrib/k8s.io/client-go/kubernetesintegration.This break in
k8s.io/client-gois causing our CI to fail. This PR addresses the issue by checking outk8s.io/client-gomanually and vendoring the last working version ofgnosticink8s.io/client-go/vendor.