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

feat(self-hosted): convert experimental env vars to config options #29154

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

Conversation

RahulGautamSingh
Copy link
Collaborator

@RahulGautamSingh RahulGautamSingh commented May 19, 2024

Changes

  • Converted the following experimental env vars to self-hosted config options
Env var Config option
RENOVATE_X_DELETE_CONFIG_FILE deleteConfigFile
RENOVATE_X_EAGER_GLOBAL_EXTENDS eagerGlobalExtends
RENOVATE_X_IGNORE_NODE_WARN ignoreNodeWarn
RENOVATE_X_S3_ENDPOINT s3Endpoint
RENOVATE_X_S3_PATH_STYLE s3PathStyle

There are some boolean types here. They have not been converted to experimentalFlags because GlobalConfig hasn't been initialized yet, when they are used.

Context

Last of: #27879 (comment)

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:

  • 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 (locally)

@RahulGautamSingh RahulGautamSingh requested review from HonkingGoose, rarkins and viceice and removed request for HonkingGoose and rarkins May 19, 2024 10:40
docs/usage/self-hosted-configuration.md Outdated Show resolved Hide resolved
docs/usage/self-hosted-configuration.md Outdated Show resolved Hide resolved
docs/usage/self-hosted-configuration.md Outdated Show resolved Hide resolved
docs/usage/self-hosted-configuration.md Outdated Show resolved Hide resolved
docs/usage/self-hosted-configuration.md Outdated Show resolved Hide resolved
lib/config/options/index.ts Outdated Show resolved Hide resolved
lib/config/options/index.ts Outdated Show resolved Hide resolved
lib/config/options/index.ts Outdated Show resolved Hide resolved
lib/config/options/index.ts Outdated Show resolved Hide resolved
lib/config/options/index.ts Outdated Show resolved Hide resolved
@@ -79,21 +79,22 @@ export async function getConfig(env: NodeJS.ProcessEnv): Promise<AllConfig> {
logger.debug('No config file found on disk - skipping');
}

await deleteNonDefaultConfig(env); // Try deletion only if RENOVATE_CONFIG_FILE is specified
await deleteNonDefaultConfig(env, !!config.deleteConfigFile); // Try deletion only if RENOVATE_CONFIG_FILE is specified
Copy link
Member

Choose a reason for hiding this comment

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

this is a problem. We can't set RENOVATE_DELETE_CONFIG_FILE from env, because we don't pass it to the file getConfig. so the config value isn't yet available and causes a breaking change.
also cli flags are not yet available, so wer probably need to move the delete code to a later stage.

@rarkins WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nabeelsaabna can we move this to a later stage and still achieve the goal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In that case, we could move it to the config/parse/index file, after we have parsed the env vars and cli flags and combined the config.

@RahulGautamSingh
Copy link
Collaborator Author

Verified each migration locally, by either logging the values of each option at runtime or confirming that they are working as expected
check 1: default values passed to functions ☑️
check 2: custom values (from cli, env, file), overwrites default values ☑️
check 3: old env vars parsed and then overwrite the default values ☑️

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.

None yet

4 participants