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

ci: check for tidied go.mod #530

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

ci: check for tidied go.mod #530

wants to merge 3 commits into from

Conversation

phm07
Copy link
Contributor

@phm07 phm07 commented Sep 6, 2024

This PR adds a CI check for go mod tidy as we already do in the CLI.

@phm07 phm07 self-assigned this Sep 6, 2024
@phm07 phm07 requested a review from a team as a code owner September 6, 2024 12:28
@jooola
Copy link
Member

jooola commented Sep 6, 2024

I am against enforcing this, this will lead to git conflict that will either have to be retried with renovate or resolved manually.

I prefer to be able to merge multiple dependency upgrade and tidy every once in a while.

Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.46%. Comparing base (a90d4c2) to head (5b7ea05).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #530   +/-   ##
=======================================
  Coverage   71.46%   71.46%           
=======================================
  Files          46       46           
  Lines        3935     3935           
=======================================
  Hits         2812     2812           
  Misses        710      710           
  Partials      413      413           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phm07
Copy link
Contributor Author

phm07 commented Sep 6, 2024

I am against enforcing this, this will lead to git conflict that will either have to be retried with renovate or resolved manually.

No, this will prevent conflicts, because the go.sum will be strictly dependent on the go.mod if you run go mod tidy. For example, if you install a dependecy, then remove it (or update a dependency, etc.) there will still be an entry in go.sum and it will not be removed.

I prefer to be able to merge multiple dependency upgrade and tidy every once in a while.

Once a dependency is merged, renovate will rebase and re-run go mod tidy on the other PRs anyway.

@jooola
Copy link
Member

jooola commented Sep 6, 2024

Once a dependency is merged, renovate will rebase and re-run go mod tidy on the other PRs anyway.

That's one of my points, I don't want to wait for Renovate.

@apricote
Copy link
Member

apricote commented Sep 6, 2024

I am also against this, but for another reason. If we always run go mod tidy we force all dependents to upgrade their dependencies as well, to the versions we have defined here.

This feels unnecessary if we do not rely on any new features of a lib (at which point go should automatically detect that it does not work and remove the old version from go.sum).

@phm07
Copy link
Contributor Author

phm07 commented Sep 9, 2024

If we always run go mod tidy we force all dependents to upgrade their dependencies as well, to the versions we have defined here.

go mod tidy doesn't upgrade dependencies or change the Go version.
https://go.dev/ref/mod#go-mod-tidy

(at which point go should automatically detect that it does not work and remove the old version from go.sum).

Old library versions are kept in the go.sum file if you don't go mod tidy it. Just have a look at our go.sum:

...
golang.org/x/sys v0.19.0 h1:q5f1RH2jigJ1MoAWp2KTp3gm5zAGFUTarQZ5U386+4o=
golang.org/x/sys v0.19.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y=
golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws=
golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
golang.org/x/sys v0.22.0 h1:RI27ohtqKCnwULzJLqkv897zojh5/DwS/ENaMzUOaWI=
golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
...

@jooola
Copy link
Member

jooola commented Sep 9, 2024

I think the workflow in itself is also something we are against. Having this extra possibility of failing CI is annoying.

A solution in between could be to run a go mod tidy task every week and commit this. But I think a manual tidy can be done occasionally, when it needed.

Old library versions are kept in the go.sum file if you don't go mod tidy it. Just have a look at our go.sum:

I don't think we gain much by cleaning this every time, while I see a lose having to comply to the CI check every time.

@phm07
Copy link
Contributor Author

phm07 commented Sep 9, 2024

Having this extra possibility of failing CI is annoying

The only possibility for the CI run to fail would be if you add, remove or upgrade dependencies, in which case you would want to run go mod tidy anyway. And it's not annoying, it serves the purpose of following best practices. It's the same reason we use a linter.

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