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

Include PR body note when Go implicitly increases the go mod directive #25047

Closed
rarkins opened this issue Oct 5, 2023 · 9 comments · Fixed by #28938
Closed

Include PR body note when Go implicitly increases the go mod directive #25047

rarkins opened this issue Oct 5, 2023 · 9 comments · Fixed by #28938
Assignees
Labels
manager:gomod Go Modules priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:in-progress Someone is working on implementation type:feature Feature (new functionality)

Comments

@rarkins
Copy link
Collaborator

rarkins commented Oct 5, 2023

Describe the proposed change(s).

As discussed in #24994, Go 1.21 can now implicitly increase the go mod directive in go.mod files if it detects that the current directive is wrong (too low), or if the upgrades in the PR require a newer Go version. In such cases the Go version change is not mentioned in the PR, but ideally it should be.

I propose that the updateArtifacts() function can return a prBodyNotes value for such cases as a quick way to warn that the Go version was bumped as part of this PR.

@rarkins rarkins added type:feature Feature (new functionality) priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others manager:gomod Go Modules priority-2-high Bugs impacting wide number of users or very important features and removed priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others labels Oct 5, 2023
@secustor
Copy link
Collaborator

secustor commented Oct 5, 2023

Counter proposal:

If updateArtifacts signals that updates have happened extract again and compare the result with the initial one.
That way we get all information we display currently on PRs.

@rarkins
Copy link
Collaborator Author

rarkins commented Oct 5, 2023

Agreed! I also created #25049

But even if #25049 is done, I think that a warning in the PR body notes would be beneficial because increasing the minimum Go version is very significant and not to be "lost" somewhere in the PR body table

@secustor
Copy link
Collaborator

secustor commented Oct 5, 2023

No disagreement there, my comment has been solely about the implementation method.

I propose that the updateArtifacts() function can return a prBodyNotes value for such cases as a quick way to warn that the Go version was bumped as part of this PR.

@rarkins
Copy link
Collaborator Author

rarkins commented Oct 5, 2023

I think only the manager knows if a particular change is worth a warning/note. So it should be able to return both indicators

@rarkins
Copy link
Collaborator Author

rarkins commented Oct 6, 2023

Reproduction repo: https://github.com/renovate-reproductions/25047

@rarkins rarkins added priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others and removed priority-2-high Bugs impacting wide number of users or very important features labels Oct 21, 2023
@mtardy
Copy link
Contributor

mtardy commented Nov 9, 2023

This is an issue we hit as well, it's annoying when maintaining stable branches with old Go versions like 1.18 for example, or even 1.20. We have PRs that are trying to upgrade just one dependency but the Go version constraint is ^directive (for example ^1.20), thus installing latest Go 1.21.x, thus updating the Go directive in the go.mod implicitely with one of the latest Go version and bumping a tons of others dependencies. It's a big mess.

Would it be possible for the Go constraint to install the Go binaries to stick to the newest minor version instead of major maybe?

I'm not entirely sure of why I observe this behavior because I was able to update only the dependencies manually with go get github.com/dependency@version_in_the_pr without all the rest being touched (while using Go 1.21.1). So I don't get why renovate can't do that as well 🤔

@rarkins
Copy link
Collaborator Author

rarkins commented Nov 9, 2023

Create a reproduction to demonstrate/prove what you're claiming. Go is updating the go.mod directive, not Renovate.

@mtardy
Copy link
Contributor

mtardy commented Dec 5, 2023

Create a reproduction to demonstrate/prove what you're claiming. Go is updating the go.mod directive, not Renovate.

Interestingly it was caused by #26057, because we have a replace directive in some go.mod that was using a local module in a submodule.

@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 37.378.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
manager:gomod Go Modules priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:in-progress Someone is working on implementation type:feature Feature (new functionality)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants