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

fix(manager/gomod): perfer to use go version defined as toolchain to update artifacts #34564

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

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Mar 1, 2025

Changes

prefer use version from toolchain directive instead of go diective in go.mod, this will avoid change current toolchain directive.

Context

As explained in #34563, when running go get ./..., go will update toolchain directive in go.mod, which is a depType=toolchain change instead of expected depType=require change.

And there are 2 ways to avoid change of toolchain directive.

I don't think old version of go support go get ./... toolchain@${current toolchain} (just like we still didn't remove -d flag here), so it would be more backward compatible to just use version of current toolchain to avoid this.

If go.mod doesn't have such directive, we still use version frmo go directive.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@trim21 trim21 changed the title fix(manager/gomod): perfer use go toolchain version fix(manager/gomod): perfer use go toolchain version to update artifacts Mar 1, 2025
@trim21 trim21 changed the title fix(manager/gomod): perfer use go toolchain version to update artifacts fix(manager/gomod): perfer to use go version defined as toolchain version to update artifacts Mar 1, 2025
@trim21 trim21 changed the title fix(manager/gomod): perfer to use go version defined as toolchain version to update artifacts fix(manager/gomod): perfer to use go version defined as toolchain to update artifacts Mar 1, 2025
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to consider the edge case where the toolchain version is being upgraded in the same PR

@trim21
Copy link
Contributor Author

trim21 commented Mar 1, 2025

So I think in that case we do not parse GoConstraints but use updated toolchain version from updatedDeps?

@rarkins
Copy link
Collaborator

rarkins commented Mar 1, 2025

Yes first check would be if Go is being updated in updatedDeps. There could also be some weird things like go mod directive updated but go toolchsin not.

@trim21
Copy link
Contributor Author

trim21 commented Mar 1, 2025

Is it possible to update go.mod first then get constraints? this should avoid this problem naturally

@rarkins
Copy link
Collaborator

rarkins commented Mar 1, 2025

By default renovate doesn't bump the go.mod directive. This was the recommendation/request of the golang team. So 99% of the time it's probably only the tool chain getting bumped, or none. I agree with you that we shouldn't "accidentally" bump the tool chain by running a latest go

@viceice
Copy link
Member

viceice commented Mar 1, 2025

I think we simply need to use new file content to extract this information instead of reading old file from disk here.

config.constraints?.go ?? (await getGoConstraints(goModFileName));

@trim21
Copy link
Contributor Author

trim21 commented Mar 1, 2025

I think we simply need to use new file content to extract this information instead of reading old file from disk here.

config.constraints?.go ?? (await getGoConstraints(goModFileName));

Yes, this is what I suggest

@trim21
Copy link
Contributor Author

trim21 commented Mar 1, 2025

By default renovate doesn't bump the go.mod directive. This was the recommendation/request of the golang team. So 99% of the time it's probably only the tool chain getting bumped, or none. I agree with you that we shouldn't "accidentally" bump the tool chain by running a latest go

I don't mean go directive, I mean we update go.mod first then parse toolchain directive from go.mod ( like this PR already did).

In that case if we need to update toolchain directive, we get updated toolchain directive.

I don't know if it's possible.

@trim21
Copy link
Contributor Author

trim21 commented Mar 1, 2025

it should be something like this?

  const goConstraints =
    config.constraints?.go ?? getGoConstraints(massagedGoMod);

@rarkins
Copy link
Collaborator

rarkins commented Mar 1, 2025

Yes. Any updates to go.mod would have been written out already

@trim21 trim21 requested a review from rarkins March 1, 2025 12:12
@trim21 trim21 requested a review from viceice March 1, 2025 20:35
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this definitely work if we are upgrading the tool chain version?

@trim21
Copy link
Contributor Author

trim21 commented Mar 5, 2025

Does this definitely work if we are upgrading the tool chain version?

I don't know, that's what I'm asking since #34564 (comment)

If you also don't know it for sure, I'd suggest we go back again using getGoConstraints(massagedGoMod) , which definitely works.

@casaqori
Copy link

casaqori commented Mar 5, 2025

Interested in this feature.
Would that change then allow toolchain pinning?
For motivation see reply on #34563

@trim21
Copy link
Contributor Author

trim21 commented Mar 5, 2025

Interested in this feature. Would that change then allow toolchain pinning? For motivation see reply on #34563

In my understanding after this change only updateType=toolchain will change toolchain directive, package only change will not change it anymore

@trim21
Copy link
Contributor Author

trim21 commented Mar 5, 2025

There is acually a corner case, when a go.mod has go major.minor.path without toolchain directive for example go 1.23.5, renovate still pick latest go version and add a toolchain directive. but go itself will pick 1.23.5, in my understanding.

@trim21
Copy link
Contributor Author

trim21 commented Mar 5, 2025

After this PR, based on go.mod novatebot will behave like this:

For go.mod:

go 1.22
toolchain go1.23.5

renovate now pick go1.23.5, and go won't change toolchain directive.

For go.mod:

go 1.23.5

renovate now pick go1.23.5, and go won't add toolchain directive.

This PR currently fix these 2 cases.


And a remaining case is this:

For go.mod, without toolchain directive

go 1.23

renovate now pick latest version of go 1 (go1.24.1 at the time of writing), and go will add toolchain directive toolchain go1.24.1

And to make go to not add toolchain directive, we will need to run go get with go get ./... toolchain@none, which maybe a breaking change for some users.

@trim21 trim21 requested a review from rarkins March 5, 2025 16:42
@casaqori
Copy link

casaqori commented Mar 6, 2025

I see, seemingly there is a typo in your observations. Lets clarify:

For go.mod:

go 1.23.5

renovate now pick go1.22.5 go1.23.5, and go won't add toolchain directive.

That will satisfy my ask to pin the toolchain at a specific version. In this case its implicitly specified by the go directive.

@trim21 I had observed that if renovate makes upgrades because of known CVEs in dependencies, it would ignore the go directive at all, and even upgrade. e.g. go1.22.0 to go1.22.13 and toolchain to go1.24.1 with this config:

  "vulnerabilityAlerts": {
    "enabled": true
  },

Might be unrelated to this PR, just giving a heads-up.

@trim21
Copy link
Contributor Author

trim21 commented Mar 6, 2025

I see, seemingly there is a typo in your observations. Lets clarify:

For go.mod:

go 1.23.5

renovate now pick go1.22.5 go1.23.5, and go won't add toolchain directive.

That will satisfy my ask to pin the toolchain at a specific version. In this case its implicitly specified by the go directive.

thanks, it's indeed a typo

@trim21
Copy link
Contributor Author

trim21 commented Mar 6, 2025

I see, seemingly there is a typo in your observations. Lets clarify:

For go.mod:

go 1.23.5

renovate now pick go1.22.5 go1.23.5, and go won't add toolchain directive.

That will satisfy my ask to pin the toolchain at a specific version. In this case its implicitly specified by the go directive.

@trim21 I had observed that if renovate makes upgrades because of known CVEs in dependencies, it would ignore the go directive at all, and even upgrade. e.g. go1.22.0 to go1.22.13 and toolchain to go1.24.1 with this config:

  "vulnerabilityAlerts": {
    "enabled": true
  },

Might be unrelated to this PR, just giving a heads-up.

I don't know how go pick toolchain in this case...

@rarkins rarkins requested a review from viceice March 9, 2025 08:58
Comment on lines +2137 to +2143
datasource.getPkgReleases.mockResolvedValueOnce({
releases: [
{ version: '1.17.0' },
{ version: '1.23.6' },
{ version: '1.24.0' },
],
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drop it and add a should not be called expect after update artifacts

Suggested change
datasource.getPkgReleases.mockResolvedValueOnce({
releases: [
{ version: '1.17.0' },
{ version: '1.23.6' },
{ version: '1.24.0' },
],
});

Comment on lines +2182 to +2189
datasource.getPkgReleases.mockResolvedValueOnce({
releases: [
{ version: '1.17.0' },
{ version: '1.23.5' },
{ version: '1.23.6' },
{ version: '1.24.0' },
],
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

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.

4 participants