feat/OAK-12253 : add a mechanism to override FT defaults via system property

[reschke]

No description provided.

[rishabhdaim]

Findings outside this diff:

AGENTS.md (~Feature Toggles): the new -Doak-feature.<toggleName>=true JVM property is not mentioned. Root AGENTS.md still describes whiteboard-only enablement. Worth a bullet noting that the initial state can be set at Feature.newFeature time via -Doak-feature.<toggleName>=true.

[reschke]

AGENTS.md (~Feature Toggles): the new -Doak-feature.=true JVM property is not mentioned. Root AGENTS.md still describes whiteboard-only enablement. Worth a bullet noting that the initial state can be set at Feature.newFeature time via -Doak-feature.=true.

I have a problem with that. We should not duplicate informations. This needs to go into oak-doc somewhere (as everything else we have to say about feature toggles).

[github-actions]

Commit-Check ❌

Commit rejected by Commit-Check.                                  
                                                                  
  (c).-.(c)    (c).-.(c)    (c).-.(c)    (c).-.(c)    (c).-.(c)  
   / ._. \      / ._. \      / ._. \      / ._. \      / ._. \   
 __\( C )/__  __\( H )/__  __\( E )/__  __\( C )/__  __\( K )/__ 
(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)
   || E ||      || R ||      || R ||      || O ||      || R ||   
 _.' '-' '._  _.' '-' '._  _.' '-' '._  _.' '-' '._  _.' '-' '._ 
(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)
 `-´     `-´  `-´     `-´  `-´     `-´  `-´     `-´  `-´     `-´ 
                                                                  
Commit rejected.                                                  
                                                                  
Type message check failed ==> OAK-12253: add a mechanism to override FT defaults via system property 
The commit message should follow Conventional Commits. See https://www.conventionalcommits.org
Suggest: Use <type>(<scope>): <description>, where <type> is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci 

--- Commit 2/7:
Type message check failed ==> OAK-12253: add a mechanism to override FT defaults via system property - temporary workaround for Sonar complaint) 
The commit message should follow Conventional Commits. See https://www.conventionalcommits.org
Suggest: Use <type>(<scope>): <description>, where <type> is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci 

Type subject_max_length check failed ==> OAK-12253: add a mechanism to override FT defaults via system property - temporary workaround for Sonar complaint) 
Subject must be at most 80 characters
Suggest: Keep the subject concise (<= configured max) 

--- Commit 3/7:
Type message check failed ==> Revert "OAK-12253: add a mechanism to override FT defaults via system property - temporary workaround for Sonar complaint)"

This reverts commit 98b38ac11765e7f6c0ea722152bd3a0ba2960a97. 
The commit message should follow Conventional Commits. See https://www.conventionalcommits.org
Suggest: Use <type>(<scope>): <description>, where <type> is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci 

Type subject_max_length check failed ==> Revert "OAK-12253: add a mechanism to override FT defaults via system property - temporary workaround for Sonar complaint)" 
Subject must be at most 80 characters
Suggest: Keep the subject concise (<= configured max) 

--- Commit 4/7:
Type message check failed ==> OAK-12253: add a mechanism to override FT defaults via system property - avoid unneeded call that could cause IAE 
The commit message should follow Conventional Commits. See https://www.conventionalcommits.org
Suggest: Use <type>(<scope>): <description>, where <type> is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci 

Type subject_max_length check failed ==> OAK-12253: add a mechanism to override FT defaults via system property - avoid unneeded call that could cause IAE 
Subject must be at most 80 characters
Suggest: Keep the subject concise (<= configured max) 

--- Commit 5/7:
Type message check failed ==> OAK-12253: add a mechanism to override FT defaults via system property 
The commit message should follow Conventional Commits. See https://www.conventionalcommits.org
Suggest: Use <type>(<scope>): <description>, where <type> is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci 

--- Commit 6/7:
Type message check failed ==> OAK-12253: add a mechanism to override FT defaults via system property 
The commit message should follow Conventional Commits. See https://www.conventionalcommits.org
Suggest: Use <type>(<scope>): <description>, where <type> is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci 

--- Commit 7/7:
Type message check failed ==> OAK-12253: add a mechanism to override FT defaults via system property - make it opt in 
The commit message should follow Conventional Commits. See https://www.conventionalcommits.org
Suggest: Use <type>(<scope>): <description>, where <type> is one of: feat, fix, docs, style, refactor, test, chore, perf, build, ci 

Type subject_max_length check failed ==> OAK-12253: add a mechanism to override FT defaults via system property - make it opt in 
Subject must be at most 80 characters
Suggest: Keep the subject concise (<= configured max) 
Commit rejected by Commit-Check.                                  
                                                                  
  (c).-.(c)    (c).-.(c)    (c).-.(c)    (c).-.(c)    (c).-.(c)  
   / ._. \      / ._. \      / ._. \      / ._. \      / ._. \   
 __\( C )/__  __\( H )/__  __\( E )/__  __\( C )/__  __\( K )/__ 
(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)
   || E ||      || R ||      || R ||      || O ||      || R ||   
 _.' '-' '._  _.' '-' '._  _.' '-' '._  _.' '-' '._  _.' '-' '._ 
(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)
 `-´     `-´  `-´     `-´  `-´     `-´  `-´     `-´  `-´     `-´ 
                                                                  
Commit rejected.                                                  
                                                                  
Type branch check failed ==> OAK-12253 
The branch should follow Conventional Branch. See https://conventional-branch.github.io/
Suggest: Use <type>/<description> with allowed types or add branch name to allow_branch_names in config, or use ignore_authors in config branch section to bypass

[rishabhdaim]

AGENTS.md (~Feature Toggles): the new -Doak-feature.=true JVM property is not mentioned. Root AGENTS.md still describes whiteboard-only enablement. Worth a bullet noting that the initial state can be set at Feature.newFeature time via -Doak-feature.=true.

I have a problem with that. We should not duplicate information. This needs to go into oak-doc somewhere (as everything else we have to say about feature toggles).

Then, this would defeat the very purpose of adding the AGENTS.md file. AI wouldn't be able to read this information from oak-doc

[jsedding]

Makes sense.

[mreutegg]

I'm not too happy with this change. One of the goals of the toggle SPI is to provide an abstraction for feature toggles. The SPI is currently independent of any source where the toggle information comes from. Controlling the toggle through system properties is just one option. I'm also a bit concerned there is no way to opt out. This change introduces hard-coded link between system properties and feature toggles. I would prefer an implementation that is opt in if a user wants to control feature toggles through system properties and then maintain that implementation in a differnt package / bundle.

[reschke]

That's why I waited for your feedback :-)

Hmmm. The initial idea was to make this consistent.

Would it be ok to let the code creating the feature to opt-in using a parameter in newFeature? That way, existing code would remain unaffected. OTOH, letting the toggle to be defaulted from a system property would be made consistent in naming and behavior.

EDIT: hopefully fixed re FeatureToggle vs Feature.

[mreutegg]

I should have been more precise. When I wrote "user", I meant someone deploying an application that contains Oak and not Oak code using the toggle SPI.

To me code in Oak using the toggle SPI should not have to know how a Feature is controlled. This is a deployment choice. Some may hook it up to LaunchDarkly or something similar. This requires additional code that interacts with FeatureToggle.

[reschke]

ok, what this intended to help with is this use case:
new feature toggle added, quickly run tests (locally) without any container

I also have seen cases where in the code, a sysprop is used to initialize the feature toggle. (would need to cehck whether this was a singular case). If that make sense, it would be good to make that pattern consistent.

[reschke]

Then, this would defeat the very purpose of adding the AGENTS.md file. AI wouldn't be able to read this information from oak-doc

Well, then our current approach is in total violation of the DRY rule. We can't maintain both. Either let the documentation link to the AGENTS.md file (hoping it is accurate), or find a way to refresh the file when the documentation changes.

[reschke]

@mreutegg - something like this...?

    private static Feature internalNewFeatureWithSystemPropertyDefault(String name, Whiteboard whiteboard, boolean withSysPropDefault) {
        // by default the initial value is false, but it can be overridden by a system property
        AtomicBoolean value = withSysPropDefault ? new AtomicBoolean(
                SystemPropertySupplier.create("oak-feature." + name, false).
                        loggingTo(LOG).get()) : new AtomicBoolean();
        FeatureToggle adapter = new FeatureToggle(name, value);
        return new Feature(value, whiteboard.register(
                FeatureToggle.class, adapter, Collections.emptyMap()));
    }

    /**
     * Creates a new {@link Feature} with the given name and registers the
     * corresponding {@link FeatureToggle} on the {@link Whiteboard}.
     * Client code must call {@link Feature#close()} when the toggle is
     * not used anymore.
     *
     * @param name the name of the feature toggle.
     * @param whiteboard the whiteboard where to register the feature toggle.
     * @return the feature toggle.
     */
    public static Feature newFeature(String name, Whiteboard whiteboard) {
        return internalNewFeatureWithSystemPropertyDefault(name, whiteboard, false);
    }

    /**
     * Same as {@linkplain #newFeature(String, Whiteboard)}m but with the initial state provided
     * by a system property, named based on the feature's name.
     */
    public static Feature newFeatureWithSystemPropertyDefault(String name, Whiteboard whiteboard) {
        return internalNewFeatureWithSystemPropertyDefault(name, whiteboard, true);
    }

[rishabhdaim]

Optional (for local consideration only — not pushing code): defaultOverriddenAsTrue / defaultOverriddenAsFalse already cover explicit boolean overrides. For other oak-feature.* values, SystemPropertySupplier uses Boolean.valueOf(String) semantics: only "true" (case-insensitive) enables the toggle; strings like "yes", "garbage", "1", etc. are treated as false, not as a parse error with log-and-fallback. If you want to document that contract in tests, something like:

    /**
     * SystemPropertySupplier uses {@link Boolean#valueOf(String)} for boolean props: only
     * {@code "true"} (case-insensitive) enables the toggle; values like {@code "yes"} or
     * {@code "garbage"} are treated as {@code false}, not as a parse error.
     */
    @Test
    public void defaultOverriddenWithNonTrueValueStaysDisabled() {
        String sysPropName = "oak-feature.my.toggle";
        for (String value : new String[] {"garbage", "yes", "1", "on", ""}) {
            System.setProperty(sysPropName, value);
            try (Feature feature = newFeatureWithSystemPropertyDefault("my.toggle", whiteboard)) {
                assertFalse("property value '" + value + "' must not enable the toggle", feature.isEnabled());
            } finally {
                System.clearProperty(sysPropName);
            }
        }
    }

[rishabhdaim]

Findings outside the diff (impact review follow-up)

Documentation (newFeatureWithSystemPropertyDefault) — The PR adds an opt-in path for JVM overrides, but user-facing docs still do not describe newFeatureWithSystemPropertyDefault or the oak-feature.<toggleName> naming. Thread consensus here is to put that in oak-doc (feature toggles section) rather than duplicating prose in AGENTS.md; a short link from agent guidance is enough if we want discoverability.

Commit-Check / branch policy — CI still rejects this PR: branch name OAK-12253 does not match the expected issue/OAK-12253 pattern, and several commits use non–Conventional Commits subjects (some over the 80-character limit). Squash/reword to conventional messages and rename the branch before merge.

[rishabhdaim]

Suggested change

	* in which case The property name is derived from the toggle name. This helps to quickly verify
	* in which case the property name is derived from the toggle name. This helps to quickly verify

[sonarqubecloud]

Quality Gate failed

Failed conditions
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE