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(config/inherit): resolve presets #31642

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

Conversation

RahulGautamSingh
Copy link
Collaborator

@RahulGautamSingh RahulGautamSingh commented Sep 26, 2024

Changes

Resolve presets present in inherited config

Context

Closes: #31278

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:

@RahulGautamSingh RahulGautamSingh marked this pull request as draft September 26, 2024 16:30
@RahulGautamSingh RahulGautamSingh marked this pull request as ready for review September 27, 2024 19:49
docs/usage/config-overview.md Outdated Show resolved Hide resolved
docs/usage/config-overview.md Outdated Show resolved Hide resolved
lib/workers/repository/init/inherited.spec.ts Show resolved Hide resolved
@HonkingGoose

This comment was marked as resolved.

lib/workers/repository/init/inherited.ts Outdated Show resolved Hide resolved
lib/workers/repository/init/inherited.ts Outdated Show resolved Hide resolved
lib/workers/repository/init/inherited.ts Show resolved Hide resolved
lib/workers/repository/init/inherited.ts Outdated Show resolved Hide resolved
lib/workers/repository/init/inherited.ts Outdated Show resolved Hide resolved
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
docs/usage/config-overview.md Outdated Show resolved Hide resolved
docs/usage/config-overview.md Show resolved Hide resolved
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
rarkins and others added 2 commits October 11, 2024 12:11
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
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.

Drop support for npm presets in inheritedConfig

lib/workers/repository/init/inherited.ts Outdated Show resolved Hide resolved
@@ -159,6 +159,19 @@ Inherited config may use all Repository config settings, and any Global config o

For information on how the Mend Renovate App supports Inherited config, see the dedicated "Mend Renovate App Config" section toward the end of this page.

#### Presets handling

If the inherited config has presets, then Renovate:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If the inherited config has presets, then Renovate:
If the inherited config contains `extends` presets, then Renovate:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If the inherited config has presets, then Renovate:
If the inherited config contains `extends` presets, then Renovate will:

If the inherited config has presets, then Renovate:

1. Resolves the presets
1. Adds the resolved presets to the inherited config
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. Adds the resolved presets to the inherited config
1. Add resolved preset config at the beginning of the inherited config

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. Adds the resolved presets to the inherited config
1. Add the resolved preset config to the beginning of the inherited config

Maybe this is better. If we go with @rarkins's suggestion, we should keep the the word. 😉

Comment on lines +111 to +116
// Decrypt after resolving, in case the preset contains npm authentication
logger.debug('Resolving presets found in inherited config');
const resolvedConfig = await decryptConfig(
await resolveConfigPresets(returnConfig, config, config.ignorePresets),
config.repository,
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

npm presets have been deprecated for a long time, please remove this logic to simplify things

Comment on lines +133 to +138
returnConfig = removeGlobalConfig(resolvedConfig, true);
if (!dequal(resolvedConfig, returnConfig)) {
logger.debug(
{ inheritedConfig: resolvedConfig, filteredConfig: returnConfig },
'Removed global config from inherited config presets.',
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inherited config does allow some fields which were previously considered "global", such as onboarding. Please check that we don't log in this case

Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

Small additions to @rarkins's suggestions.


1. Resolves the presets
1. Adds the resolved presets to the inherited config
1. Merges the presets on top of the global config
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. Merges the presets on top of the global config
1. Merge the presets on top of the global config

@@ -159,6 +159,19 @@ Inherited config may use all Repository config settings, and any Global config o

For information on how the Mend Renovate App supports Inherited config, see the dedicated "Mend Renovate App Config" section toward the end of this page.

#### Presets handling

If the inherited config has presets, then Renovate:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If the inherited config has presets, then Renovate:
If the inherited config contains `extends` presets, then Renovate will:

If the inherited config has presets, then Renovate:

1. Resolves the presets
1. Adds the resolved presets to the inherited config
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. Adds the resolved presets to the inherited config
1. Add the resolved preset config to the beginning of the inherited config

Maybe this is better. If we go with @rarkins's suggestion, we should keep the the word. 😉

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.

Inherited config should be resolved before merging
4 participants