Support protocol buffers as a transport medium in the cluster registry #178
Conversation
k8s-ci-robot
added
sig/multicluster
perotinus
force-pushed
the
protos
branch
3 times, most recently
from
5072827
to
12826af
Compare
There was a problem hiding this comment.
Cool to see this!
Would you be able to explain the difference between the vendoring changes and the third_party changes? I think the latter is the C++ implementation of protocol buffers (protoc compiler), while the former is the changes to vendor in the protobuf Golang generator protoc-gen-gogo from k8s.io/code-generator and the specific Golang compiler plugin from github.com/gogo/protobuf. Is that right?
hack/update-codegen.sh
Outdated
| //vendor/k8s.io/code-generator/cmd/go-to-protobuf/protoc-gen-gogo \ | ||
| //third_party/protobuf:protoc | ||
|
|
||
| PATH="$(bazel info bazel-bin)/vendor/k8s.io/code-generator/cmd/go-to-protobuf/protoc-gen-gogo:$(bazel info bazel-bin)/third_party/protobuf:${PATH}" \ |
There was a problem hiding this comment.
This is an artifact of how the protocol buffer compiler works: it expects to be able to call plugins from the PATH. A bit of a pain, but able to be worked around like so.
Though, with the latest changes to rules_go, this has gotten a bit more complex.
|
Also looks like this needs a rebase and retesting. |
|
@font Rebased, and I'll fix any additional test failures. I removed the |
|
Looks like |
|
@font In any case, this should be ready to review; that infrastructure issue will need to be fixed before submission, but it shouldn't be a review blocker. |
There was a problem hiding this comment.
This looks good and simpler now without third_party!
Curious, how does the bazel workspace contain a version of protobuf?
For the infrastructure test failure, you should be able to use the same image kubekins-e2e we're using for the pull-cluster-registry-verify-gosrc job, as that one contains the Go toolchain whereas the bazelbuild image used by the pull-cluster-registry-verify-gensrc job does not. It also contains bazel as I understand. So we may want to eventually convert all of the jobs to use kubekins-e2e.
hack/update-codegen.sh
Outdated
| PROTOC_GEN_GOGO_PATH="$(dirname "$(bazel build //vendor/k8s.io/code-generator/cmd/go-to-protobuf/protoc-gen-gogo:protoc-gen-gogo 2>&1 | grep bazel-bin | xargs)")" | ||
| PROTOC_PATH="$(dirname "$(bazel build @com_google_protobuf//:protoc 2>&1 | grep bazel-bin | xargs)")" | ||
|
|
||
| PATH="$(bazel info workspace)/${PROTOC_PATH}:$(bazel info workspace)/${PROTOC_GEN_GOGO_PATH}:${PATH}" \ |
There was a problem hiding this comment.
Could we add a comment explaining that it's for the compiler to find the plugins?
Gopkg.toml
Outdated
| ignored = ["github.com/kubernetes/repo-infra/kazel"] | ||
| ignored = [ | ||
| "github.com/kubernetes/repo-infra/kazel", | ||
| "github.com/google/protobuf/*", |
There was a problem hiding this comment.
Is there a transitive dependency requiring dep to ignore?
There was a problem hiding this comment.
Hmm. I thought I had removed this, but I'll make sure it's actually gone. Perhaps it slipped into another commit as I was rebasing.
|
/test pull-cluster-registry-verify-gensrc |
1 similar comment
|
/test pull-cluster-registry-verify-gensrc |
|
@font I expect that some other dependency that's in the workspace requires the protobuf library, and so it's pulled in. My guess would be that |
|
/test pull-cluster-registry-verify-gensrc |
|
/retest |
1 similar comment
|
/retest |
|
@font It looks like the |
|
@font OK, I fixed the issue with |
|
@perotinus /lgtm |
|
/meow |
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: font, perotinus The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/sig multicluster
/cc @font @madhusudancs @pmorie
Fixes #106.