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

Convert experimental env into globalOnly experimentalFlags #27879

Open
rarkins opened this issue Mar 12, 2024 · 12 comments · May be fixed by #28187
Open

Convert experimental env into globalOnly experimentalFlags #27879

rarkins opened this issue Mar 12, 2024 · 12 comments · May be fixed by #28187
Assignees
Labels
breaking Breaking change, requires major version bump core:config Related to config capabilities and presets priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code

Comments

@rarkins
Copy link
Collaborator

rarkins commented Mar 12, 2024

Describe the proposed change(s).

Remove all RENOVATE_X_ logic in code, convert instead to a new admin option experimentalFlags. Convert docs accordingly (all should be documented and it enforced with type checking).

@rarkins rarkins added priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code breaking Breaking change, requires major version bump core:config Related to config capabilities and presets labels Mar 12, 2024
@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 18, 2024

There are some variables in this list which don't have the prefix RENOVATE_X, should they be included in this refactor too?
eg., https://docs.renovatebot.com/self-hosted-experimental/#otel_exporter_otlp_endpoint

@RahulGautamSingh
Copy link
Collaborator

Do you mean we store all these variables and values in experimentalFlags object?
ie.,
old

# .env
RENOVATE_X_HARD_EXIT=false

new

// config.js
"exprimentalFlags": {
  "HARD_EXIT": false
}

@rarkins
Copy link
Collaborator Author

rarkins commented Mar 18, 2024

I was going to keep them as an array. If they need a value then it could be like abc=10. One like this could be turned around such as "softExit" or "exitCodeZero"

@RahulGautamSingh
Copy link
Collaborator

There are some options such as RENOVATE_X_IGNORE_RE2 which are used before GlobalConfig is initialized, what should be do about them?

@rarkins
Copy link
Collaborator Author

rarkins commented Mar 19, 2024

How many of those are there? I guess we need to keep them

@RahulGautamSingh
Copy link
Collaborator

Two with the prefix RENOVATE_X:

RENOVATE_X_HARD_EXIT

if (process.env.RENOVATE_X_HARD_EXIT) {

RENOVATE_X_IGNORE_RE2

if (process.env.RENOVATE_X_IGNORE_RE2) {

@rarkins
Copy link
Collaborator Author

rarkins commented Mar 21, 2024

Ok keep them as is for now

@RahulGautamSingh RahulGautamSingh linked a pull request Mar 30, 2024 that will close this issue
6 tasks
@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 30, 2024

I think we are clear on the requirements. Here's the implementation plan I came up with:

First make necessary changes in initial PR (ideally also convert 2,3 env vars to flags as examples). Then in many follow up PRs convert all the env vars to flags.

For this it would be best to create a new branch from the main branch, where all the initial and follow up PRs could be merged, before finally merging that one into main again. I am not sure about this btw, since these are experimental features doing all in one PR seems fine to me as well. But it should be easy in a multiple PRs.

Changes to be included in the initial PR:

  • add experimentalFlags config option
  • logic for validating experimentalFlags and type-based validation of each flag within it
  • add description of experimentalFlags in the docs

I've put together this PR as a starting point. I've tried to implement the necessary logic to handle the exeperimental flags. I have also converted 2 env vars to flags in this PR as examples.

More details in the PR description.

@rarkins
Copy link
Collaborator Author

rarkins commented Apr 10, 2024

@viceice @secustor what if we drop the "experimental" naming and instead use "globalFlags" or "adminFlags"? We can always flag individual flags as experimental if we want to consider removing them.

Part of using flags here was to reduce the volume of unique config options when many of them are simply boolean and need little more than a one-sentence description.

@secustor
Copy link
Collaborator

IMO it make sense to have experimental in the naming.
We communicate that these features are unstable and subject to change without looking at the docs.
Further that extra scrutiny is expected as they are not part of the normal configuration.

If that is not the intend we can migrate all flags to config options with the experimental setting. 🤷

@rarkins
Copy link
Collaborator Author

rarkins commented Apr 10, 2024

ok

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented May 17, 2024

Fresh list of the the experimental vars which remain to be converted, with some related infromation.

This list doesn't include the env vars which have been converted in #28187, #28397 and #28880

Env Var Conversion Possible Remarks Possible Config Option Name
OTEL_EXPORTER_OTLP_ENDPOINT Used before global config class is initialized
RENOVATE_CACHE_NPM_MINUTES Option removed in v38 cacheNpmMinutes
RENOVATE_X_DELETE_CONFIG_FILE deleteConfigFile
RENOVATE_X_DOCKER_MAX_PAGES dockerMaxPages
RENOVATE_X_EAGER_GLOBAL_EXTENDS eagerGlobalExtends
RENOVATE_X_GITLAB_AUTO_MERGEABLE_CHECK_ATTEMPS gitlabAutoMergeableCheckAttempts
RENOVATE_X_GITLAB_BRANCH_STATUS_DELAY gitlabBranchStatusDelay
RENOVATE_X_GITLAB_MERGE_REQUEST_DELAY gitlabMergeRequestDelay
RENOVATE_X_HARD_EXIT For this to work, the return type of instrument will have to be changed which is not desirable I think hardExit
RENOVATE_X_IGNORE_NODE_WARN ignoreNodeWarn
RENOVATE_X_IGNORE_RE2 Regex is used at one too many places to be sure, if it will be called before global config is initialized
RENOVATE_X_PLATFORM_VERSION platformVersion
RENOVATE_X_S3_ENDPOINT s3Endpoint
RENOVATE_X_S3_PATH_STYLE s3PathStyle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change, requires major version bump core:config Related to config capabilities and presets priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants