refactor: extract and update now decoupled #5835
Conversation
|
This pull request introduces 1 alert when merging 13c3369 into f25ae78 - view on LGTM.com new alerts:
|
Part of osp-cfc-platform/backlog#
@viceice fixed. |
|
Yeah, still trying to understand how to move out getChangelogJSON out of update. |
|
This pull request introduces 1 alert when merging fa19cc7 into fd329d1 - view on LGTM.com new alerts:
|
Done. |
| branches = branches.concat(baseBranchRes.branches); | ||
| branchList = branchList.concat(baseBranchRes.branchList); | ||
| } | ||
| return { res, branches, branchList }; | ||
| return { branches, branchList }; |
There was a problem hiding this comment.
This seems to be skipping packageFiles as expected by the calling function: const { branches, branchList, packageFiles } = await processRepo(config);
There was a problem hiding this comment.
packageFiles variable was ignored before my changes in this place, so that is why I made it like that
Do we need it?
There was a problem hiding this comment.
It's referenced in const additionalFiles = await getAdditionalFiles(config, packageFiles);
There was a problem hiding this comment.
yeah, but exactly in this place packageFiles was ignored.
Lets look in the master branch:
- https://github.com/renovatebot/renovate/blob/master/lib/workers/repository/process/index.ts#L59
as we can see packageFiles is part of baseBranchRes
and on next string packageFiles just ignored.
and further: https://github.com/renovatebot/renovate/blob/master/lib/workers/repository/process/index.ts#L64
only res, branches and branchList are returned.
That is why packageFiles is optional parameter in ExtractResult (and was optional before me).
What I'm trying to protect is that this PR is just refactoring, without changing overall logic behind it.
And additional changes going to be features or bug fixes and not related to this PR itself.
|
Hi @YuraBeznos, I attempted to fix the conflict but I don't have push permissions to your repo. Could you merge from master, resolve the conflict, then push to github? |
| @@ -9,16 +9,17 @@ import { PackageFile } from '../../../manager/common'; | |||
| import { RenovateConfig } from '../../../config'; | |||
| import { BranchConfig } from '../../common'; | |||
|
|
|||
| export type ExtractAndUpdateResult = { | |||
| res: WriteUpdateResult | undefined; | |||
| export type ExtractResult = { | |||
| branches: BranchConfig[]; | |||
| branchList: string[]; | |||
| packageFiles?: Record<string, PackageFile[]>; | |||
There was a problem hiding this comment.
| packageFiles?: Record<string, PackageFile[]>; | |
| packageFiles: Record<string, PackageFile[]>; |
There was a problem hiding this comment.
Sorry, I'm against it. #5835 (comment)
There was a problem hiding this comment.
I don't agree, but if they're ignored them remove them completely?
There was a problem hiding this comment.
This is ignored in one place and in use in another. And that how it works in master right now, without that PR.
If this is a bug, lets fix it in different PR.
| config: RenovateConfig, | ||
| branches: BranchConfig[], | ||
| branchList: string[], | ||
| packageFiles?: Record<string, PackageFile[]> |
There was a problem hiding this comment.
| packageFiles?: Record<string, PackageFile[]> | |
| packageFiles: Record<string, PackageFile[]> |
…post-lookup-pre-commit
Done. |
…post-lookup-pre-commit
|
🎉 This PR is included in version 19.208.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Refactor: move changelog retrieval to post-lookup / pre-commit
Closes #5774