Do dockerized builds for minikube and localkube #1656
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
cncf-cla: yes
Codecov Report
@@ Coverage Diff @@
## master #1656 +/- ##
=======================================
Coverage 38.65% 38.65%
=======================================
Files 51 51
Lines 2667 2667
=======================================
Hits 1031 1031
Misses 1455 1455
Partials 181 181Continue to review full report at Codecov.
|
|
Nice, this looks really good. |
dlorenc
approved these changes
Jun 28, 2017
Makefile
Outdated
| LOCALKUBE_BUILD_IN_DOCKER=y | ||
| endif | ||
|
|
||
| ifeq ($(IN_DOCKER),1) |
There was a problem hiding this comment.
Maybe just a quick comment here explaining that this prevents infinite recursion, at first this reads incorrect.
r2d4
commented
Jun 28, 2017
Makefile
Outdated
|
|
||
| # $(call MINIKUBE_GO_BUILD_CMD, output file, OS) | ||
| define MINIKUBE_GO_BUILD_CMD | ||
| $($(shell echo MINIKUBE_ENV_$(2) | tr a-z A-Z)) go build --installsuffix cgo -ldflags="$(MINIKUBE_LDFLAGS) $(K8S_VERSION_LDFLAGS)" -o $(1) k8s.io/minikube/cmd/minikube |
There was a problem hiding this comment.
Quick question: I accidently dropped the -a flag to go build here, but I don't think we need it. What was the reasoning initially?
-a
force rebuilding of packages that are already up-to-date.
BUILD_IN_DOCKER=y will build both localkube and minikube in docker LOCALKUBE_BUILD_IN_DOCKER=y, MINIKUBE_BUILD_IN_DOCKER=y will toggle docker builds for each respective binary Regardless of the options provided, if you attempt to build localkube on a non-linux platform, it will run in docker. This ensures that `make` still works as expected on darwin.
minikube-linux-amd64 and minikube-darwin-amd64 cannot be cross built natively on different platforms and must be built in the container.
|
@minikube-bot retest this please |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BUILD_IN_DOCKER=y will build both localkube and minikube in docker
LOCALKUBE_BUILD_IN_DOCKER=y, MINIKUBE_BUILD_IN_DOCKER=y will toggle docker builds for each
respective binary
Regardless of the options provided, if you attempt to build localkube on a
non-linux platform, it will run in docker. This ensures that
makestill works as expected on darwin.
The only thing that I couldn't get working was the extra platform detection when cross building. So BUILD_IN_DOCKER=y is required when
make cross. Any suggestions on how to avoid that would be greatI originally planned something like
ifneq(,$(or $(filter $(MINIKUBE_BUILD_IN_DOCKER),y),$(filter-out $(BUILD_OS),$*))), but automatic variables like$*aren't expanded in GNU make conditionals.We might want to just move a lot of this to bash for simplicity and debugging.