Usability improvements to build steps #2890
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
@antoineco we removed the local build in #2868 |
| @@ -41,7 +41,8 @@ fi | |||
|
|
|||
| export CGO_ENABLED=0 | |||
|
|
|||
| go build -a -installsuffix cgo \ | |||
| go build \ | |||
| ${GOBUILD_FLAGS} \ | |||
There was a problem hiding this comment.
you need to set a default in case this env value is not set (that's why this is failing CI)
|
I was working on an older master and the rebase didn't pick that up, thanks for the pointer ;) |
|
@aledbf I addressed your comments and added some more minor fixes/improvements. Each commit should be self-documented. Putting this on hold for now because I want to squash these commits. |
k8s-ci-robot
added
the
do-not-merge/hold
Makefile
Outdated
| @@ -48,7 +48,7 @@ ARCH ?= $(shell go env GOARCH) | |||
| GOARCH = ${ARCH} | |||
| DUMB_ARCH = ${ARCH} | |||
|
|
|||
| GOBUILD_FLAGS ?= -v | |||
| GOBUILD_FLAGS := | |||
There was a problem hiding this comment.
Tested with both make build and make build GOBUILD_FLAGS='-v -a'.
Does what's expected in both scenarios using a pretty old make version (3.81, mac OS).
Makefile
Outdated
| @@ -161,7 +161,7 @@ endif | |||
| build: build-image | |||
| @echo "+ Building bin/$(ARCH)/nginx-ingress-controller" | |||
| @$(DEF_VARS) \ | |||
| GOBUILD_FLAGS=$(GOBUILD_FLAGS) \ | |||
| GOBUILD_FLAGS="$(GOBUILD_FLAGS)" \ | |||
There was a problem hiding this comment.
Quotes needed to avoid /bin/sh: -a: command not found when passing custom flags.
Makefile
Outdated
| @@ -164,9 +168,6 @@ build: | |||
| GOBUILD_FLAGS="$(GOBUILD_FLAGS)" \ | |||
| build/go-in-docker.sh build/build.sh | |||
|
|
|||
| @echo "+ Copying artifact to temporary directory" | |||
There was a problem hiding this comment.
I figured this didn't make sense here, in isolation, because make build and make container could not be invoked separately.
Makefile
* Make Go build flags parameterizable
* Verbose by default
* Do not force complete rebuild
* Remove unnecessary '-installsuffix' flag
https://plus.google.com/117192131596509381660/posts/eNnNePihYnK
* Log build steps
* Rename 'clean' target to 'clean-container'
* Move artifact copy to '.container' target
* Add 'clean' target
Shell script
* Fix shellcheck SC2068
https://github.com/koalaman/shellcheck/wiki/SC2068
* Reject mandatory vars with empty values
Codecov Report
@@ Coverage Diff @@
## master #2890 +/- ##
==========================================
- Coverage 47.56% 47.54% -0.02%
==========================================
Files 76 76
Lines 5483 5483
==========================================
- Hits 2608 2607 -1
- Misses 2540 2542 +2
+ Partials 335 334 -1
Continue to review full report at Codecov.
|
|
Squashed (I hope the thumb up reaction meant 'ok' 😉). |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, antoineco The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Suggestions of tiny improvements for things I run often. I though some may make sense for the rest of us as well:
-aflag). Speeds up builds quite a lot on modest hardware.GOBUILD_FLAGSvariable, defaults to-vto show the build progress. Also makes the wait more meaningful than a long blank on modest hardware.Move build ofingress-nginx:buildimage to abuild-imagemake target, mainly to be able to log that step in the right order.cleantarget toclean-container, because it's really what it does.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged):Does not introduce any feature or fix any bug.