Make update-all & ensure bazel-only targets are runnable #5251
Conversation
jetstack-bot
added
kind/cleanup
| bin | ||
| _bin |
There was a problem hiding this comment.
This stops bazel from trying to look in these directories for files!
| sh_binary( | ||
| name = "update-deps", | ||
| srcs = ["update-deps.sh"], | ||
| args = [ | ||
| "$(location %s)" % GO, | ||
| "$(location %s)" % GAZELLE, | ||
| "$(location %s)" % KAZEL, | ||
| "$(location :update-bazel)", | ||
| "$(location :update-deps-licenses)", | ||
| ], | ||
| data = [ | ||
| GAZELLE, | ||
| GO, | ||
| KAZEL, | ||
| ":update-bazel", | ||
| ":update-deps-licenses", | ||
| ], | ||
| tags = ["manual"], | ||
| ) |
There was a problem hiding this comment.
This is re-added from an older commit but marked manual so it won't be automatically run anywhere.
| tags = ["manual"], | ||
| ) | ||
|
|
||
| sh_binary( | ||
| name = "update-deps-licenses", | ||
| srcs = ["update-deps-licenses.sh"], | ||
| args = [ | ||
| "$(location %s)" % GO, | ||
| ], | ||
| data = [ | ||
| GO, | ||
| ], | ||
| tags = ["manual"], |
There was a problem hiding this comment.
update-deps-licenses is re-added from an older commit but marked manual so it won't be automatically run anywhere.
update-bazel and the licenses target become manual
| echo -e "\033[0;33mThis script is preserved for legacy reasons, and as such will also update bazel | ||
| You shouldn't need to run this script or install bazel for normal development. | ||
| Use 'make update-all' to do everything this script does without touching bazel\033[0m" |
There was a problem hiding this comment.
I felt like removing update-bazel here would be a more disruptive change for people who use these hack scripts due to muscle memory, so I kept it here but added make update-all for more general use.
| if [[ ! -f go.mod ]]; then | ||
| echo "No module defined, see https://github.com/golang/go/wiki/Modules#how-to-define-a-module" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
This just seems redundant, although I recognise it's an unnecessary change!
| export GO111MODULE=on | ||
| export GOPROXY=https://proxy.golang.org | ||
| export GOSUMDB=sum.golang.org | ||
| mode="${1:-}" | ||
| shift || true | ||
| case "$mode" in | ||
| --minor) | ||
| if [[ -z "$@" ]]; then | ||
| "$go" get -u ./... | ||
| else | ||
| "$go" get -u "$@" | ||
| fi | ||
| ;; | ||
| --patch) | ||
| if [[ -z "$@" ]]; then | ||
| "$go" get -u=patch ./... | ||
| else | ||
| "$go" get -u=patch "$@" | ||
| fi | ||
| ;; | ||
| "") | ||
| # Just validate, or maybe manual go.mod edit | ||
| ;; | ||
| *) | ||
| echo "Usage: $(basename "$0") [--patch|--minor] [packages]" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
|
|
||
| rm -rf vendor | ||
| "$go" mod tidy | ||
| unset GOROOT |
There was a problem hiding this comment.
I removed this because it doesn't do anything when update-deps is called via bazel (i.e. without an argument) and I don't really see a need to wrap these commands in make.
If someone needs a helper script to update our dependencies (as in, they don't understand how to update the dependencies without a helper) then they probably shouldn't be updating our dependencies (an action which has huge supply-chain implications and needs careful review in any case).
If someone doesn't need this helper script, surely they'd just use go get -u themselves?
| args=$(ls "$(pwd)" | grep -v 'bazel-' | grep -v 'external/' | grep -v 'bin' ) | ||
| args=$(ls "$(pwd)" | grep -v 'bazel-' | grep -v 'external/' | grep -v 'bin' | grep -v '_bin' ) |
There was a problem hiding this comment.
unrelated but I think this is an obvious thing to do!
| echo "Please run 'bazel run //hack:update-gofmt'" | ||
| exit 1 | ||
| fi | ||
| make verify-imports |
There was a problem hiding this comment.
goimports isn't the same as gofmt but I think it's fine to replace gofmt entirely here. We already run goimports on every PR.
| # NB: This script requires bazel, and is no longer supported since we no longer support bazel | ||
| # We want to add something like this to make, but since this script was never part of any CI | ||
| # pipeline it's not a priority. The script is kept for backwards compatibility for now but may | ||
| # change or be removed in the future. | ||
|
|
||
| # See https://github.com/cert-manager/cert-manager/pull/3037#issue-440523030 | ||
|
|
||
| # Currently only works on linux/amd64, darwin/amd64. |
There was a problem hiding this comment.
I initially just removed this script and I think that'd probably be fine, but might as well leave it in this PR.
| ## Update all CRDs to the latest version based on the current checkout | ||
| ## | ||
| ## @category Development |
There was a problem hiding this comment.
It seems pointless to document this when most people will want to run both update-crds and update-codegen together at the same time. make update-all is probably preferable to document for most uses.
this allows us to maintain the bazel build files until they're removed, but tries to avoid accidentally encouraging their use `make update-all` implementes a non-bazel version of `hack/update-all.sh`, with `hack/update-all.sh` now calling make but also doing the bazel stuff it used to. Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
|
/test pull-cert-manager-make-test |
There was a problem hiding this comment.
Thanks @SgtCoDFish agree that one target to udpate everything seems like the most straightforward thing and easiest for contributors 👍🏼
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irbekrm, SgtCoDFish 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 |
|
/test pull-cert-manager-make-test ❄️ |
This allows us to maintain the bazel build files until they're removed, but tries to avoid accidentally encouraging their use
make update-allimplementes a non-bazel version ofhack/update-all.sh, withhack/update-all.shnow calling make but also doing the bazel stuff it used to.Kind
/kind cleanup
Release Note