-
Notifications
You must be signed in to change notification settings - Fork 419
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
All go install
commands in kube_codegen.sh
fail because the version is not specified
#184
All go install
commands in kube_codegen.sh
fail because the version is not specified
#184
Comments
Yes i also face the same issue. The current script mandates polluting the go.mod from which the script picks up the versions. Alternatively we have to define empty imports in |
Perhaps a simple thing to do would be ask for the k8s API version. The versioning seems to follow the same semantic versioning as the k8s API. If not specified assume the latest as default version. This also maintains backward compatibility. |
/assign |
What are the circumstances that lead to you having this script, which lives in the What I really mean is that this script is fairly tightly coupled to the tools it executes, and if you give it some back-rev version of the tools, the script may not work, or vice-versa. |
I did not want to bring in the dependency of version_gomod = $(shell go list -mod=mod -f '{{ .Version }}' -m $(1)) I use the k8s.io/api dependency to extract the version and use that version for CODE_GENERATOR_VERSION ?= $(call version_gomod,k8s.io/api) Assumption is that code-generator tags/versions are in-line with k8s API versions |
Where does the script come from? Did you copy it I to your own repo? |
in the same I have also seen in other repositories developers are just building what they need via |
Why is the rest of this repo needed? The script:
Maybe the tool is intended to be ran from a copy of this repo, but it's not documented anywhere and it works anywhere outside of what appears to be a logic bug/bad assumption with the script.
In my case, I'm using it to generate a client that has bindings for this tool, but doesn't actually provide a client itself (CloudNativePG). I have tooling that clones a specific version of that repo, downloads this script, and does essentially |
So you are running I am just trying to be understand - this script is coupled to the specific version of the tools. While neither are particularly fast-changing, they are not meant to be interchangeable. I understand that we have a non-trivial deps tree, but you ACTUALLY DO depend on them. Have you considered using a 'tools' directory, with its own go.mod, to isolate those deps without breaking them? The script doesn't use "./" notation mostly for clarity. That doesn't make it less coupled. My concern here, again, is that the tools may change or the script may change, and unless you are using the same versions, you may have a bumpy ride. I don't INTEND to change all the CLI flags, but I didn't intend to last time, either. And in fact, I DO have some budding ideas on how we might make these tools more robust, and that might necessitate CLI changes. If you are cloning a specific version of this repo, then you really want the same version of the tools. Why is that version not encoded in a go.mod? Even a child go mod in a subdir (and look, I agree that go tooling here is clunky but it exists). |
I think this is a great argument for setting the installed tool versions. With the current behavior, when the script runs It'd be great if the script was updated to instead do Even better would be
This wouldn't solve the versioning issue, as it would allow the script and go tool versions to drift (for example, when |
I think maybe there's a disconnect between how I assume people use this and how you actually use it. I assume people use Go's tooling, even if the only reason is so you are not swimming against the current. Make a fresh module:
Get a back-rev version of code-generator, to emulate an older repo:
Vendor your deps at the pinned versions (note, Go's vendoring includes related scripts because this pattern is what the tooling expects people to do, too).
Run the tool that we pinned. Note the
Now update the dep and re-run the tool:
Note the flags are different. If you don't like all of this in your main go.mod, you can make a This is what I assumed people are doing. Clearly I was not quite correct. The question is whether to convince you that you should do it more like this, or whether I should adapt to accomodate a different pattern. What I don't want to see is people cloning Any version drift between the script and the tools is a liability. |
I see. Most projects I work on usually avoid vendoring due to it's drawbacks (larger repos, larger dep update diffs, multiple sources of truth, licensing issues, etc.) so I hadn't considered it as an option. I'd prefer to avoid this as possible - I don't need (or want) all the sources files of this repo checked into my repo(s). The tool is a dev dependency as well, so I'd prefer to keep it out of my go.mod direct dependencies, too. The ideal path (for me) would be to be able to download just a single file (which today would be the The only thing missing this today is that the scripts do not include a release version number in them anywhere, so when they do |
Opinion time: NOT vendoring is insanity. Upstream repos DO disappear and the module cache is a free public service with a best-effort SLO. None of the alleged downsides of vendoring have really manifested in an project I work with. But I am not here to convince you to vendor. Given that the script in question doesn't change very often, embedding a version in it is my least favorite idea. If you already know exactly which version you want ( I really wanted to understand how that came to be, and I guess I do, now. |
That is also true for all repositories in the Gardener project as well. This move was made perhaps last year. The reasoning forced us to look at our dependencies and ensure that we are only dependent upon projects that are a) well maintained b) active c) have contributors from multiple organisations and there are other criteria. No good project will disappear all of a sudden, there is usually an announcement and there is sufficient time for us to either clone the project and maintain it ourselves or move to another alternative project. So projects disappearing is not a strong argument to have vendoring. But as you have said above its an opinion and something that everyone has to decide for themselves. But to assume that people will use vendoring (when the language does not mandate it) is IMHO not a fair assumption. |
That's not my experience, but OK. I'm not trying to convince you.
That's fair, though the script pretty clearly assumes a local copy of the whole code-generator repo:
|
When the
kube_codegen.sh
script exists outside a go module directory, automatic installation commands from this repo (i.e. defaulter-gen, client-gen, etc.) fails because the tool version is not specified. For example, thegen_client
function attempts to install the tools here essentially just runs:At the top of the subshell the working directory is changed to the
kube_codegen.sh
, which the logic assumes is a Go module directory. If it isn't, the install command fails because thego
command requires a version to be specified when running outside of a module directory.To reproduce:
. /tmp/kube_codegen.sh; kube::codegen::gen_client --output-dir /tmp/generated /tmp/src
go: 'go install' requires a version when current directory is not in a module [...]
message loggedAlternatively:
docker run --rm --env GO111MODULE=on golang:1.23.2 go install k8s.io/code-generator/cmd/defaulter-gen
go: 'go install' requires a version when current directory is not in a module [...]
message loggedIf somebody can tell me how to plumb the release version number into the script then I can file a PR to fix this.
By the way, this line and related install lines in the script can be simplified to:
GO111MODULE=on go install "${BINS[@]/#/k8s.io/code-generator/cmd/}"
which would also fix the quoting issue, and remove the need for the linter exception.
The text was updated successfully, but these errors were encountered: