Change non-boolean enabled configs to mode #12409
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bneradt
force-pushed
the
fix_enable_value_issues
branch
2 times, most recently
from
acd70c4 to
cc8a02d
Compare
maskit
requested changes
Jul 31, 2025
Member
maskit
left a comment
maskit
left a comment
There was a problem hiding this comment.
The description makes sense. Just one possible error in the change.
bneradt
force-pushed
the
fix_enable_value_issues
branch
4 times, most recently
from
e78cef8 to
dad9c30
Compare
Most "enabled" configurations are booleans that toggle on or off a feature. But we have a few that are named "enabled" but actually take an enumeration of values to control behavior. This is confusing terminology. This patch keeps support for the enabled configuration as currently named, adds a new "mode" configuration for these, and deprecates the old "enabled" config in the docs. proxy.config.ssl.session_cache.enabled was somewhat complicated: with the 10.0.0 release, we implemented it as "value" to try to address the issue of this patch just for that parameter, but kept the "enabled" config description in the docs. So the docs were out of sync with the code. This adds support for "enabled" and immediately deprecates that, since users may have tried to use that, silently keeps support for "value" in case that was used, and adds the now desired "mode" configuration. If a user passes both the deprecated "enabled" config as well as the new "mode" config, then they must be the same value, otherwise an Emergency message is logged because it is impossible for ATS to discern which value should be used. Aside from this Emergency log on mismatch just mentioned, this patch should be backwards compatible without introducing any changes in behavior for current configs. Fixes: apache#11901
bneradt
force-pushed
the
fix_enable_value_issues
branch
from
dad9c30 to
5e3b082
Compare
Contributor
|
It seems some of the autest got caught up into the new Linking this as it had some discussion related to this. |
Contributor
|
100% onboard with this change. The output would be: I think would be a good idea, having some mapping for this so if you hit one of the variables in that mapping you could get an error, like: We could even have this inside ATS, but not sure how far we wanna go. |
Contributor
Author
|
After crawling in the weeds for a while, I think this will be better for the user as a simple rename in 11.x, rather than trying to support both in 10.x: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most "enabled" configurations are booleans that toggle on or off a feature. But we have a few that are named "enabled" but actually take an enumeration of values to control behavior. This is confusing terminology. This patch keeps support for the enabled configuration as currently named, adds a new "mode" configuration for these, and deprecates the old "enabled" config in the docs.
proxy.config.ssl.session_cache.enabled was somewhat complicated: with the 10.0.0 release, we implemented it as "value" to try to address the issue of this patch just for that parameter, but kept the "enabled" config description in the docs. So the docs were out of sync with the code. This adds support for "enabled" and immediately deprecates that, since users may have tried to use that, silently keeps support for "value" in case that was used, and adds the now desired "mode" configuration.
If a user passes both the deprecated "enabled" config as well as the new "mode" config, then they must be the same value, otherwise an Emergency message is logged because it is impossible for ATS to discern which value should be used.
Aside from this Emergency log on mismatch just mentioned, this patch should be backwards compatible without introducing any changes in behavior for current configs.
Fixes: #11901