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: resolve repoConfig.packageRules.extends with repo config #14978

Merged

Conversation

anomiex
Copy link
Contributor

@anomiex anomiex commented Apr 5, 2022

Changes

Redo #14688, working around the undocumented setting thing from #14827.

Context

Closes #14974

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 tick 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

Apparently there are undocumented settings that would fail validation.
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

needs test case(s) to validate that the bug for sourceUrl never happen again.

@anomiex
Copy link
Contributor Author

anomiex commented Apr 6, 2022

It turns out that's easier said than done, because the tests for the merge module unconditionally mock the migrate-validate module that's responsible for actually throwing the error.

I've added a test that checks that the mocked migrateAndValidate function wasn't passed the packageRules. If you know of a better way to test it, feel free to contribute.

@rarkins rarkins marked this pull request as draft April 9, 2022 05:24
@anomiex anomiex marked this pull request as ready for review April 11, 2022 13:33
@anomiex anomiex requested a review from viceice April 11, 2022 13:33
@viceice viceice requested a review from rarkins April 14, 2022 13:11
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

I'm not sure here 🤷‍♂️

@anomiex anomiex requested a review from viceice May 18, 2022 13:31
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

@rarkins I'm still not sure about side effects here.

@rarkins
Copy link
Collaborator

rarkins commented May 19, 2022

Same here. Let's try to step back:

The problem here is that an extends array within packageRules within config.js doesn't get resolved. If it's present within a renovate.json then it does.

This happens because:

  • extends values are "lazily" resolved, meaning not during global init but instead waiting until repo init
  • we are resolving extends within repo config before merging it with global config, meaning they are missed out

We already have a bit of a hack to copy extends from the root of global config into the repo config before resolving it, but extends within packageRules - or anywhere else non-root will be missed out.

So would this change work instead?

diff --git a/lib/workers/repository/init/merge.ts b/lib/workers/repository/init/merge.ts
index 1ead00699..4ad24befa 100644
--- a/lib/workers/repository/init/merge.ts
+++ b/lib/workers/repository/init/merge.ts
@@ -249,6 +249,7 @@ export async function mergeRenovateConfig(
     delete resolvedConfig.hostRules;
   }
   returnConfig = mergeChildConfig(returnConfig, resolvedConfig);
+  returnConfig = await presets.resolveConfigPresets(returnConfig, config);
   returnConfig.renovateJsonPresent = true;
   // istanbul ignore if
   if (returnConfig.ignorePaths?.length) {

@anomiex
Copy link
Contributor Author

anomiex commented May 25, 2022

So would this change work instead?

It does for my use case. Updated the PR to use that.

@rarkins rarkins requested a review from viceice May 28, 2022 06:42
@anomiex anomiex requested a review from rarkins June 6, 2022 13:29
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

🤷‍♂️

@rarkins rarkins enabled auto-merge (squash) June 10, 2022 05:00
@anomiex
Copy link
Contributor Author

anomiex commented Jun 13, 2022

@rarkins The auto-merge isn't happening for some reason?

@viceice
Copy link
Member

viceice commented Jun 13, 2022

@rarkins The auto-merge isn't happening for some reason?

press the button:
image

@rarkins rarkins merged commit ef41262 into renovatebot:main Jun 13, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 32.84.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@anomiex anomiex deleted the fix/config-js-packageRules-extends-again-sigh branch June 13, 2022 16:43
MaronHatoum pushed a commit to MaronHatoum/renovate that referenced this pull request Jun 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

repoConfig.packageRules.extends no longer works
4 participants