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 · 14 comments
Open

Convert experimental env into globalOnly experimentalFlags #27879

rarkins opened this issue Mar 12, 2024 · 14 comments
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
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

Except the following experimental env vars all the others have either been converted to their own config option, or to experimental flags. Some of the vars have also been removed.

Env Var Remarks
OTEL_EXPORTER_OTLP_ENDPOINT Used before global config is initialized
RENOVATE_X_HARD_EXIT For this to work, the return type of instrument will have to be changed which is not desirable I think
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_SUPPRESS_PRE_COMMIT_WARNING Used before global config is initialized
RENOVATE_X_REBASE_PAGINATION_LINKS Used before global config is initialized
RENOVATE_X_PAGINATE_ALL Used before global config is initialized

@rofreytag
Copy link

rofreytag commented Nov 13, 2024

Hey there, related: RENOVATE_X_GITHUB_HOST_RULES was added in #25361 but there is no documentation for it.
Also I would like to opt-in to using it with the hosted-by-mend variant, and I have no idea wether I can set this at all, so please consider this use case as well

@rarkins
Copy link
Collaborator Author

rarkins commented Nov 14, 2024

No. The main reason this feature was made opt-in was because it doesn't work with app tokens

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.

4 participants