-
Notifications
You must be signed in to change notification settings - Fork 744
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
dockerize controller-gen #958
dockerize controller-gen #958
Conversation
7231730
to
bebcfc9
Compare
Codecov Report
@@ Coverage Diff @@
## master #958 +/- ##
==========================================
- Coverage 43.62% 43.46% -0.16%
==========================================
Files 52 52
Lines 3200 3200
==========================================
- Hits 1396 1391 -5
- Misses 1610 1614 +4
- Partials 194 195 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
controller-gen: __tooling-image | ||
CONTROLLER_GEN=docker run -it -v $(shell pwd):/gatekeeper gatekeeper-tooling controller-gen | ||
|
||
__tooling-image: |
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.
is the double-underscore intended to signify non-user-facing targets? If so, I like it.
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.
yes, exactly. I like having tab-completion be a way of leading people through the makefile. At least on my machine, a __
-prefixed target doesn't even show up in tab-completion.
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.
Very nice, I could see us using this to hide a bunch of our current Makefile targets
CONTROLLER_GEN=docker run -it -v $(shell pwd):/gatekeeper gatekeeper-tooling controller-gen | ||
|
||
__tooling-image: | ||
docker build . \ |
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.
We should figure out how to avoid rebuilding the docker image if it already exists.
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.
There's some tradeoffs here worth discussing:
-
docker
will cache pre-built layers, though even cached it turns a 1.34s command into a 7.513s command -
We can separate this into two discrete steps (build to an actual version tag like
1.0.0
instead oflatest
) and then reference this version in the other command. This will result in speedup but will interrupt existing users' experience. It will also inject additional complexity into the makefiles as there will need to be targets that a: build and run, b: run only. The targets that usecontroller-gen
as a dependency (manifests
,generate
) could then either have both "a" and "b" versions, or just always use "a". If that's the case, then we're back where we started with "a" as the default.
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.
Max and I talked offline and decided we're going to keep this the way it is.
d18de3d
to
4a083fa
Compare
Signed-off-by: juliankatz <juliankatz@google.com>
4a083fa
to
bb9a1e3
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.
build/tooling/Dockerfile
Outdated
@@ -0,0 +1,6 @@ | |||
FROM golang:1.15.4 |
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.
Can we go with 1.15-alpine
here to reduce the size?
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.
Done.
I went with the regular image to start because of my past painful experiences with apk
, and because this image will never be run in parallel on a machine where size might matter.
Signed-off-by: juliankatz <juliankatz@google.com>
What this PR does / why we need it:
The
make manifests
target currently relies on the developer having the correct version (0.3.0
) of thecontroller-gen
CLI tool. This broke for me, as I had the old2.5.0
installed.By wrapping a docker container around
controller-gen
, I've locked this call to a specific version of the tool. A gatekeeper developer need not even install this tool on their system, as long as they havedocker
.This will also allow us to upgrade this library in the future with zero coordination among gatekeeper developers.