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

Make it hard to accidentally enable an experimental feature flag (backport #11998) (backport #12058) #12059

Merged
merged 5 commits into from
Aug 19, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Aug 19, 2024

Currently it's easy to enable an experimental feature flag like khepri_db by accident. Given the operation is irreversible, this leads to serious consequences when done in a production environment.

This PR implements the following changes:

  • Management UI
    • Split the feature flags page into two tables - first with all but experimental feature flags, the second with only the experimental flags (and a warning)
    • To enable an experimental feature flag, a checkbox needs to be toggled first
  • CLI
    • rabbitmqctl enable_feature_flag khepri_db won't work unless a new flag --experimental is provided
Screenshot 2024-08-14 at 12 19 57
This is an automatic backport of pull request #11998 done by [Mergify](https://mergify.com).
This is an automatic backport of pull request #12058 done by [Mergify](https://mergify.com).

(cherry picked from commit d9819bc)
(cherry picked from commit 39f28b6)
(cherry picked from commit 58e0c16)
(cherry picked from commit 6fae57e)
Plus, a slightly more scary error message

(cherry picked from commit e3302f2)
(cherry picked from commit 8db0414)
Also, remove the `undef` case which was only needed for
RabbitMQ 3.7 and older.

(cherry picked from commit ddb117f)
(cherry picked from commit 5135d85)
(cherry picked from commit 396ad7a)
(cherry picked from commit 0e1816a)
@dmitrynovik
Copy link

Thanks @mkuratczyk
I think the --force flag is a very good idea!

I'm not so convinced about the UI.
Maybe it's good not to list experimental features in UI at all to avoid the temptation?

@michaelklishin
Copy link
Member

@dmitrynovik they are now listed in a separate category and require an explicit confirmation. The idea is certainly not to hide experimental feature flags. We've had a grand total of 1 up to this point, and in 4.0, that feature flag graduates to be a regular one.

@michaelklishin michaelklishin added this to the 3.13.7 milestone Aug 19, 2024
@michaelklishin michaelklishin merged commit 8b13372 into v3.13.x Aug 19, 2024
226 checks passed
@michaelklishin michaelklishin deleted the mergify/bp/v3.13.x/pr-12058 branch August 19, 2024 23:56
@dmitrynovik
Copy link

dmitrynovik commented Aug 19, 2024

Also, semantically --include-experimental would probably be a better name than --force.

P.S. Looking in the changed files, seems like it's --experimental :)

@mkuratczyk
Copy link
Contributor

I forgot to update the PR description but the flag was changed during the PR review process and what we merged, actually uses --experimental rather than --force. For additional safety --experimental is rejected if the feature flag is all - experimental features need to be explicitly specified to be enabled from the CLI

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.

3 participants