-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat(gomod): add support for package replacements #29575
base: main
Are you sure you want to change the base?
Conversation
d47e4e9
to
be641b7
Compare
lib/modules/manager/gomod/update.ts
Outdated
@@ -36,11 +35,14 @@ export function updateDependency({ | |||
const lineToChange = lines[upgrade.managerData.lineNumber]; | |||
logger.trace({ upgrade, lineToChange }, 'go.mod current line'); | |||
if ( | |||
!lineToChange.includes(depNameNoVersion) && | |||
!lineToChange.includes(fromPackageNameNoVersion) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also check toPackageNameNoVersion
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends if this code gets executed the "second" time (next run) after a replacement. If so it looks like you need to check both
d52c9ab
to
cb98d8e
Compare
To make it possible for folks using Go modules to be able to take advantage of Renovate's package replacements, we should add support to the `gomod` manager. This adds support for managing the following `updateType`s: - minor/patch - major - digest For `major` and `digest` updates, we need to support the case that we're performing a replacement, so add an additional check for the `updateType=replacement` and whether there's a `newMajor` or `newDigest`. Because we're now managing multiple `depName`s, we should rename this to `fromPackageName` and `toPackageName`, following how we've done this with the `hermit` manager. Additionally, as we're replacing the dependency's name, we want to have a separate regex capture group for `depName`, which we can also use for determining whether there are quotes around the dependency. As part of renovatebot#24883.
cb98d8e
to
bcc7977
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the second half yet
lib/modules/manager/gomod/update.ts
Outdated
@@ -36,11 +35,14 @@ export function updateDependency({ | |||
const lineToChange = lines[upgrade.managerData.lineNumber]; | |||
logger.trace({ upgrade, lineToChange }, 'go.mod current line'); | |||
if ( | |||
!lineToChange.includes(depNameNoVersion) && | |||
!lineToChange.includes(fromPackageNameNoVersion) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends if this code gets executed the "second" time (next run) after a replacement. If so it looks like you need to check both
/^(?<depPart>\s+[^\s]+[\s]+[=][>]+\s+)(?<divider>[^\s]+\s+)[^\s]+/, | ||
/^(?<depPart>\s+[^\s]+[\s]+[=][>]+\s+)(?<depName>[^\s]+)(?<divider>\s+)[^\s]+/, | ||
); | ||
} else { | ||
updateLineExp = regEx( | ||
/^(?<depPart>replace\s+[^\s]+[\s]+[=][>]+\s+)(?<divider>[^\s]+\s+)[^\s]+/, | ||
/^(?<depPart>replace\s+[^\s]+[\s]+[=][>]+\s+)(?<depName>[^\s]+)(?<divider>\s+)[^\s]+/, | ||
); | ||
} | ||
} else if (depType === 'require' || depType === 'indirect') { | ||
if (upgrade.managerData.multiLine) { | ||
updateLineExp = regEx(/^(?<depPart>\s+[^\s]+)(?<divider>\s+)[^\s]+/); | ||
updateLineExp = regEx( | ||
/^(?<depPart>\s+)(?<depName>[^\s]+)(?<divider>\s+)[^\s]+/, | ||
); | ||
} else { | ||
updateLineExp = regEx( | ||
/^(?<depPart>require\s+[^\s]+)(?<divider>\s+)[^\s]+/, | ||
/^(?<depPart>require\s+)(?<depName>[^\s]+)(?<divider>\s+)[^\s]+/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have concerns about these regex changes breaking something. Unfortunately the existing regex isn't documented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex isn't user-facing is it? I believe it's only used in this file, so - from what I can tell - the usages in this file it seems to be OK, any thoughts on how we can work out if there's any impact? What would you be thinking in terms of documentation here?
lib/modules/manager/gomod/update.ts
Outdated
@@ -22,7 +22,7 @@ export function updateDependency({ | |||
// newName will be available for replacement | |||
const toPackageName = newName ?? fromPackageName; | |||
// istanbul ignore if: should never happen | |||
if (!fromPackageName || !toPackageName || !upgrade.managerData) { | |||
if (!fromPackageName || !upgrade.managerData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may need to undo this check to ensure that eslint
knows that we've checked this
As part of a prefactor for renovatebot#29575, we should make this change independently, to make the changes in renovatebot#29575 easier to review. This is required as we will be adding replacements for `gomod` and so we'll have both a `fromPackageName` and a `toPackageName`.
Moved the renaming into #30030 - LMK if there's any more you think we could move out into a separate PR? |
Sorry, re-requested review a bit early - will fix up E: think we need both vars to do find/replace |
While testing this - here - I've noticed two things: Package update tableThe package updates table presents the old package name with a
Should we instead show the new name?
This is likely a wider change, as it probably impacts other cases where we're doing replacements, so may be worth a separate PR.
|
Hmm I need to retry jamietanna/renovate-iss-oapi#2 and check it also works when we update the module import path |
OK, so retrying with an update to my test repo, I see jamietanna/renovate-iss-oapi#5 raised which does not take the |
Adding support for that looks like it could be a little more involved - separate PR? |
(This isn't yet working - I can't seem to trigger the |
Any thoughts on why we may not be able to see |
@@ -31,7 +31,7 @@ import { getExtraDepsNotice } from './artifacts-extra'; | |||
const { major, valid } = semver; | |||
|
|||
function getUpdateImportPathCmds( | |||
updatedDeps: PackageDependency[], | |||
updatedDeps: Upgrade[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't quite correct, and why things aren't working
@@ -31,7 +31,7 @@ import { getExtraDepsNotice } from './artifacts-extra'; | |||
const { major, valid } = semver; | |||
|
|||
function getUpdateImportPathCmds( | |||
updatedDeps: PackageDependency[], | |||
updatedDeps: Upgrade[], | |||
{ constraints }: UpdateArtifactsConfig, | |||
): string[] { | |||
// Check if we fail to parse any major versions and log that they're skipped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, when we're seeing a replacement, we get the wrong data here 🤔
Changes
Modifies the
gomod
manager to add support for managing package replacements, if present.Context
As part of #24883, towards being able to solve #29567, we want to add support for managing package replacement with Renovate.
To make it possible for folks using Go modules to be able to take
advantage of Renovate's package replacements, we should add support to
the
gomod
manager.This adds support for managing the following
updateType
s:For
major
anddigest
updates, we need to support the case that we'reperforming a replacement, so add an additional check for the
updateType=replacement
and whether there's anewMajor
ornewDigest
.Additionally, as we're replacing the dependency's name, we want to have
a separate regex capture group for
depName
, which we can also use fordetermining whether there are quotes around the dependency.
As part of #24883.
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via:
Example PR: jamietanna/renovate-iss-oapi#2
Note: should the
Package
column be updated in the PR description to be the new package?