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
images: Move hubble-proto into cilium-builder #16217
images: Move hubble-proto into cilium-builder #16217
Conversation
6743e5d
to
924e7f0
Compare
This commit consolidates the `hubble-proto` image (used to build the Hubble protobuf definitions) and the `cilium-builder` image. While this increases the download size for developers just needing to regenerate the Hubble API definitions, removing `hubble-proto` reduces the maintenance overhead of our developer images (for example, `hubble-proto` currently lacks the required GitHub Action to build and push it to docker.io). The new api/v1/Makefile using this image will is added in a subsequent commit. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
This replaces the current use of `hubble-proto` with `cilium-builder`. The generated files remain functionaly the same as the used plugin versions have not changed. The only difference in the output comes from the fact that `cilium-builder` installs the plugins via `go install` instead of `go get`. Therefore, the dependency set of the three built protoc plugins are not merged as they were in `hubble proto` due to it building them in a ad-hoc go module. Therefore `protoc-gen-go-grpc` is using the `protoc-gen-go` version specified in its go.mod file, which is slightly older than what was used before, but this has no effect on the generated Go lang source as the two versions are compatible. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
924e7f0
to
f79c5f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, LGTM 👍
cilium/hubble-proto:${version} | ||
$(QUIET)$(CONTAINER_ENGINE) container run --rm \ | ||
--volume $(CURDIR):/src \ | ||
--user "$(shell id -u):$(shell id -g)" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, running as a regular user is definitely something we want (it was also part of my old draft PR #13405).
Based on cilium/cilium#16217 Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 🎉
test-me-please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried locally, worked for me.
test-1.19-5.4 hit https://jenkins.cilium.io/job/Cilium-PR-K8s-1.19-kernel-5.4/143/ #16122 |
test-runtime hit https://jenkins.cilium.io/job/Cilium-PR-Runtime-4.9/4752/ #15604 |
test-1.16-netnext hit https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-net-next/564/ #15775 |
All required tests have passed. Marking as ready for merge. GH Actions seems stuck in the non-required ones, so the bot is unable to add the label itself. The |
Based on cilium/cilium#16217 Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
This commit consolidates the
hubble-proto
image (used to build theHubble protobuf definitions) and the
cilium-builder
image. While thisincreases the download size for developers just needing to regenerate
the Hubble API definitions, removing
hubble-proto
reduces themaintenance overhead of our developer images (for example, it currently
lacks the required GitHub Action to build and push it to docker.io).