-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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(lockfilemaintenance): ensure isLockFileMaintenance
on grouping
#33971
base: main
Are you sure you want to change the base?
Conversation
const isLockFileMaintenance = config.updateType === 'lockFileMaintenance'; | ||
const { isLockFileMaintenance } = config; |
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.
When I see old code which references config.updateType
, it implies to me that config
just be an "update" config and not a "branch" config.
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.
no. it's the branch config, so it simply assumes the proper update type was copied to branch config
if (config.upgrades.some((upgrade) => upgrade.updateType === 'major')) { | ||
config.updateType = 'major'; | ||
} | ||
|
||
// explicit set `isLockFileMaintenance` for the branch for groups | ||
if (config.upgrades.some((upgrade) => upgrade.isLockFileMaintenance)) { |
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.
Here, config = branch config. So you're setting branchConfig.isLockFileMaintenance=true if any update in the branch has isLockFileMaintenance set. This seems logically correct. But I think updateArtifacts()
is no called on a full branch, so the refactorings within manager code seem unrelated or mismatched with this change?
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.
No, it's passing the full branch config. patchConfigForArtifactsUpdate
is only patching the lockFiles
property and config
is of BranchConfig
type.
renovate/lib/workers/repository/update/branch/get-updated.ts
Lines 334 to 344 in f182708
const results = await managerUpdateArtifacts(manager, { | |
packageFileName: packageFile.path, | |
updatedDeps, | |
// TODO #22198 | |
newPackageFileContent: packageFile.contents!.toString(), | |
config: patchConfigForArtifactsUpdate( | |
config, | |
manager, | |
packageFile.path, | |
), | |
}); |
renovate/lib/workers/repository/update/branch/get-updated.ts
Lines 375 to 385 in f182708
const results = await managerUpdateArtifacts(manager, { | |
packageFileName: packageFile.path, | |
updatedDeps, | |
// TODO #22198 | |
newPackageFileContent: packageFile.contents!.toString(), | |
config: patchConfigForArtifactsUpdate( | |
config, | |
manager, | |
packageFile.path, | |
), | |
}); |
renovate/lib/workers/repository/update/branch/get-updated.ts
Lines 421 to 429 in f182708
packageFileName: packageFile.path, | |
updatedDeps: [], | |
newPackageFileContent: contents!, | |
config: patchConfigForArtifactsUpdate( | |
config, | |
manager, | |
packageFile.path, | |
), | |
}); |
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 would test this at least once on a real repo.
ok, it only occurs when grouping lock file maintenance with other updates, so will search the discussions for a repro repo |
Changes
ensure
isLockFileMaintenance
istrue
if grouping lockefile maintenance with other updates even it's not really supported.Context
lockFileMaintenance
not working #32384Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via: