Skip to content
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

chore: move tools in a module to reduce root module bloat #559

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jooola
Copy link
Member

@jooola jooola commented Nov 20, 2024

This moves the tools dependencies in a dedicated module to reduce the bloat of the root module.

This should also reduce the amount of failed check generated CI jobs in renovate PRs, by not sharing the deps between our library and the tools that we are using.

The mix of make files and go generate seems unnecessarily complex, we might want to move everything to make files if this turns out to be painful.

@jooola jooola requested a review from a team as a code owner November 20, 2024 11:31
@jooola jooola force-pushed the extract-tools-in-sperate-module branch from 2f25a28 to f2d15d8 Compare November 20, 2024 11:31
@apricote
Copy link
Member

While I personally like the division in "prod" and "dev" dependencies, I also appreciate the simplicity of using go run ... in the generate comments, it removes a bunch of steps from the pipeline.

In addition, while these dependencies are in our go.mod, they wont end up in downstream users go.mod because go is smart enough to only add the dependencies that are actually imported in the code you use.

@jooola
Copy link
Member Author

jooola commented Nov 20, 2024

Yes, I agree that the simplicity of go run is nice.

Part of the reason why I did this is that we have those jobs failing, a bit too often, for me to tidy the files manually every time:

https://github.com/hetznercloud/hcloud-go/actions/runs/11889479028/job/33126124091?pr=550#step:4:28

We could simplify this by using a makefile, and skipping go generate all together, would that work for you?

@apricote
Copy link
Member

At that point we could just let renovate run goModTidy and merge !530.

We could simplify this by using a makefile, and skipping go generate all together, would that work for you?

That would also work, personally not that much of a fan of Makefiles if it can also be done in a language-native way, but I would not block it.

@phm07
Copy link
Contributor

phm07 commented Nov 22, 2024

Why do we need a Makefile instead of doing it like in hetznercloud/cli#901 with go run -C ./tools?

@jooola
Copy link
Member Author

jooola commented Nov 22, 2024

I am stepping back from this PR, @phm07 will take a look at this to see if its worth it.

@apricote
Copy link
Member

Go 1.24 will add a tool directive to go.mod to specify these external binaries: https://tip.golang.org/doc/go1.24#go-command

@phm07
Copy link
Contributor

phm07 commented Dec 19, 2024

Go 1.24 will add a tool directive to go.mod to specify these external binaries: https://tip.golang.org/doc/go1.24#go-command

Wouldn't we need to update the module to 1.24 then as well? Since the new directive goes into the go.mod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants