-
Notifications
You must be signed in to change notification settings - Fork 416
replace go-install.sh with a new script that focuses on downloading binaries #3431
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
base: main
Are you sure you want to change the base?
Conversation
…inaries There is IMHO no point in forcing everyone, especially every single CI job, to download *and compile* each and every tool being used to build kcp. Especially getting and building golangci-lint takes forever. Even worse, newer golangci-lint versions might use Go versions that kcp does not yet support, forcing us to hold off updating the linter until we also update Go. I entirely removed compiling apigen, instead I replaced it with `go run`. I know of at least one other person than me who also runs into the issue of working on the kcp API and forgetting to `make clean` before doing codegen again. There is simply no need to build a binary for this thing. On-behalf-of: @SAP [email protected]
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.
/lgtm
/approve
LGTM label has been added. Git tree hash: 924a86dbef495511ad268533e1ea3c2109d35538
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xmudrii The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Did we never move this repo over to |
Even Go's In general I am worried about mixing a project's dependencies with those of the tools it uses. |
@xrstf sure, both the Aren't all the tools in this PR Go-based? |
verify_boilerplate isn't 😁 Also some of our scripts already use jq, which is also not Go. |
Can you link to this .bingo solution/software? I am not familiar with it yet. :) Or is it just the spirit of setting $GOBIN? |
It's been a long time since I touched |
It also looks like they have a Go-based runner for the verify-boilerplate stuff: https://github.com/kubernetes-sigs/release-utils/blob/main/mage/boilerplate.go |
I've toyed around with bingo and I quite like it. It solves all the usual problems and even generates files for Makefiles and shell scripts to source: incude .bingo/Variables.mk
.PHONY: lint
lint:
$(GOLANGCI_LINT) run ./... It's also very unintrusive in how it installs the dependencies - it's really just a binary dropped into GOBIN. And with the tool directory Also this approach allows automatic dependency updates with e.g. with renovate with very little customManager magic. The only drawback is that it doesn't solve the problem for non-go tools - but how many non-go tools are there really that this would apply to? On jq - next to the jq in Go Steve linked there is also Edit: Even better - the makefiles are bingo-less and just use incude .bingo/Variables.mk
.PHONY: lint
lint: $(GOLANGCI_LINT)
$(GOLANGCI_LINT) run ./... e.g.:
And that will cause the tool to be build in exactly the version as pinned by bingo. Only whoever updates bingo will need to have bingo installed technically. Not sure how the prow runners would cache this though - but there's probably a way to have the prow runners have a persistent/cached GOBIN. |
Indeed, bingo dependencies can be handled by renovate: Although whats missing is updating the tool-specific go.sum file, that would still require a human to go and update and add the commit, but at least a notification for an update is coming in automatically. Edit: That also wasn't too hard to automate with a workflow. |
I'm -1 for Anything that we might need more from |
That is true - but bingo itself isn't injecting itself in the execution, the recipes run The bingo approach also only has a single dependency - namely bingo is just a wrapper around some
A tool doesn't need to be constantly changed to be useful. GNU coreutils have a handful releases every year (usually once per quarter) with most tools not being touched at all, and it's still software running everywhere. As for dependencies - the tool is mostly using the stdlib/xtra with some exceptions for semver parsing, shell execution and cobra/pflag. As for issues - many of those are suggestions. If you filter for |
And that the curl approach only requires curl isn't quite true - |
This is technically incorrect -- anything that you run in CI/CD has a risk of supply chain attacks (and there's a very solid chance that we'll be using
If we're paranoid, we could add checksum checks to the bash script, that should be very trivial.
That's again technically incorrect. First, number of dependencies doesn't matter (e.g. the project itself could be compromised), second, there's actually a couple of dependencies used by that project: https://github.com/bwplotka/bingo/blob/eebab197b1b510268e8658e9526b907dd009ff96/go.mod On top of that, given that releases are infrequent, the project is built using a non-supported version of Go, so it could be affected by the Go vulnerabilities as well (at some point).
We can't compare any Go tool with GNU There's a very big difference between not needing to add new features constantly and project being effectively unmaintained. For Go tools, it's clear what are the minimal requirements to consider it maintained: keep dependencies up to date and built semi-often with a supported Go toolchain version. None of that is happening in case of I have to make it very clear, I have a lot of respect for the author of that tool. But dependency management is something very critical to me and I don't feel confident in using something that is not maintained for that, especially for the project such as kcp. As I said above, anything that needs to be fixed, is it a vulnerability, bug, or even a new feature that we need, there's an extremely high chance that we would be on our own (i.e. fork and maintain the tool ourselves). The shell scripts that @xrstf proposed don't need any of that, they pose very little risk (much less than
In my books, |
That is what I wanted to express, yes - anything that isn't 100% in our control will yield some risk. Be it through libraries, tools or infrastructure we use.
I said as much a bit further down - but that is quintessentially what bingo is; a templater for the go.mod, Makefile and shell script; and the dependencies don't matter once the files are generated.
That only matters if prebuilt binaries are used - we still decide which Go version we use to build and run the tools.
Imho calling a project unmaintained when it isn't implementing more outside of it's intended feature scope is a bit harsh - but I agree on the dependencies.
The coreutils comparison is playing devils advocate - though the base comparison still stands with corutils being dynamically linked just like bingo can be built/run with a current go version (ignoring the dependencies that could bear updating). But putting With binary releases we rely on the tool authors a) publishing releases and b) providing binaries (and c) these binaries not being manipulated during the builds) - and even if a feature or fix that is needed for kcp is merged a new release might be weeks out unless it is crucial. I don't think bingo is inherently better than the shell script, please don't misunderstand me - I just think the approach of With the module _
tool github.com/golangci/golangci-lint/cmd/golangci-lint
require github.com/golangci/golangci-lint/v2 v2.1.6 go mod tidy -modfile=golangci-lint.mod As said - I just really like the approach. |
And hence we can reduce this risk by putting things in our control and ownership. This is not always worth, but if it requires effectively no effort to maintain (as in this example), we should just do it.
I think these are great examples of the premature optimization. We're now talking about things that we don't have a problem with at all, but that might hypothetically happen. However, that always costs something, and in this case it's about using yet another tool. Not only that there are concerns that I outlined above, but everyone working with kcp (dependencies) will have to fiddle with Also, we'll still have to keep this, because my understanding (and please correct me if I'm wrong), is that That said, I'm strongly in favor of keeping it simple and rolling with this approach for now. It's already fully implemented, it's super easy to use, and any benefits that we might gain from any other solution are marginal. |
I don't know if I would call that premature optimization :D
To which the argument again would be that the question is how many non-go tools we do have, because I only see
I agree, I think we have discussed the pros and cons enough - also api-syncagent and kcp-operator already have the same script as @xrstf had pointed out; the PR was just a cleanup to update the kcp repository. We'd just need an owner to approve the PR @mjudeikis @embik |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I dont mind either way. Either bingo, this or something else. Any improvments to the system are welcome. lgtm from my side. |
@xrstf merge? what is the consensus here? |
Merge after rebase and after adding some sort of checksum validation. I was blocked by the workspace auth feature PR and will now go back to this PR here and kube-bind and stuff. |
This PR is still on my backburner, right now I am struggeling coming up with a nice ay to incorporate checksums into this. Even if I had a file with the checksums for golangci-lint in 8 different [arch,os] combos, updating these whenever someone bumps golangci-lint is nontrivial, mostly because the download script has no idea about the possible URL patterns it is called with. |
Summary
There is IMHO no point in forcing everyone, especially every single CI job, to download and compile each and every tool being used to build kcp. Especially getting and building golangci-lint takes forever. Even worse, newer golangci-lint versions might use Go versions that kcp does not yet support, forcing us to hold off updating the linter until we also update Go. And non-Go tools (like yq) were impossible to sanely install (we already cheated with verify_boilerplate.py, which skips the whole hack/tools versioning approach).
I replaced the go-install.sh with a script that we're already using to great effect in the api-syncagent and kcp-operator repos. Applications for which no binaries are available will still be installed using
go install
.I entirely removed compiling apigen, instead I replaced it with
go run
. I know of at least one other person than me who also runs into the issue of working on the kcp API and forgetting tomake clean
before doing codegen again. There is simply no need to build a binary for this thing.What Type of PR Is This?
/kind cleanup
Release Notes