Keycloak / PR #24

Add rolling-updates feature flag and compatibility framework

keycloak/keycloak#36882
Golden recall: 0% Extra findings: 6

Golden Comments (0/1 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Incorrect method call for exit codes. The picocli.exit() method calls System.exit() directly, which is problematic: MEDIUM ✗ Not found The review treats the exit codes from other angles — the silent renumbering of the RECREATE exit code 4→3 (CompatibilityResult L34) and the CLI-level code leaking into the compatibility contract (L37) — but no candidate mentions the direct picocli.exit()/System.exit() call. See quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.java:48

Supplementary Findings (6 findings)

Finding File Severity Legitimate?
In createInitialDeployment, ROLLING_UPDATES is enabled only inside the RECREATE branch, which is a no-op because AlwaysRecreateUpgradeLogic ignores the feature flag — the rolling-update scenario this MR is meant to gate is never actually tested UpgradeTest.java:115 HIGH ✓ Real and the most valuable finding of the review: the feature's own test never exercises the feature
The feature-gating block (checking Profile.isFeatureEnabled(...), printing the disabled message, exiting with FEATURE_DISABLED) is duplicated verbatim in both UpdateCompatibilityCheck.run() and UpdateCompatibilityMetadata.run() — should be centralized in the shared AbstractUpdatesCommand base UpdateCompatibilityCheck.java:46 MEDIUM ✓ Real duplication with drift risk between the two commands
RECREATE_UPGRADE_EXIT_CODE silently changed from 4 to 3 — a breaking change for any script that branches on the recreate exit code, unnecessary since 4 did not collide with picocli's reserved codes (1 and 2) CompatibilityResult.java:34 MEDIUM ✓ Real undocumented breaking change for external scripts
ROLLING_UPDATES declared as Type.PREVIEW, which auto-enables it when the preview profile is selected, even though the docs and operator test instruct explicit opt-in (--features=rolling-updates) — PREVIEW_DISABLED_BY_DEFAULT would match the intent Profile.java:136 LOW ✓ Borderline: design-consistency question, not a bug
printFeatureDisabled() hardcodes the literal 'rolling-updates' in the message — should be derived from Profile.Feature.ROLLING_UPDATES.getKey() so the message cannot go stale AbstractUpdatesCommand.java:84 LOW ✓ Real but cosmetic; hardcoded feature key
FEATURE_DISABLED is a CLI-level gate exit code but is placed on the CompatibilityResult interface, the contract returned by CompatibilityManager.isCompatible(...) — it can never be produced by the compatibility logic and leaks a CLI concern into the core contract CompatibilityResult.java:37 LOW ✓ Real contract leak, minor

Honest Analysis

FriendlyReviewer found 0 of the 1 golden comment of this PR — recall 0%. The golden is a process-level concern: picocli.exit() calls System.exit() directly, hard-exiting the JVM and bypassing picocli's own exit handling. The review engages with exit codes extensively, but from a different altitude — it treats them as data: the silent renumbering of the RECREATE code 4→3 (L34), the FEATURE_DISABLED constant leaking into the compatibility contract (L37). No candidate inspects the actual call at UpdateCompatibilityCheck.java:48, so the golden stays invisible even though the review's own findings point at the same file.

The miss should not be read as a weak review: its top finding — the operator test enabling ROLLING_UPDATES only in the RECREATE branch where the flag is ignored, so the rolling-update behavior is never tested — is arguably more valuable than the golden itself, and it is stated precisely with the fix location (move the enablement to the default path). The gating-block duplication and the exit-code renumbering are legitimate contract-level findings, and the six supplementary items contain no filler.

What the miss exposes is an altitude gap: the review reasoned about exit-code constants, never about the invocation path. On a PR whose centerpiece is CLI behavior, not reading how the command actually exits left the golden invisible. The same gap appears in the checklist, where the verdict "Partially covered" is honest — it explicitly states the feature's behavior is not verified by the test suite — yet the summary simultaneously asserts the "mechanics (enum entry, gating, exit-code constants) are consistent with existing conventions", a claim the golden directly contradicts at the call site.

The overall result is mixed: a strong set of six extras, all real and mostly at the right altitude (the test gap is a genuine catch), but the review validated the flag mechanics without ever questioning the exit mechanism itself. For a CLI framework PR, that is the one place the dataset says a maintainer should look first.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
1 high, 2 medium, 3 low 1 mitigated

This MR introduces the rolling-updates preview feature flag and wires it into the update/compatibility CLI commands. The flag is correctly declared in the Profile enum and the commands gate on it via Profile.isFeatureEnabled(...), and the basic mechanics (enum entry, gating, exit-code constants) are consistent with existing conventions. However, the review found a number of issues that should be addressed before merge: the feature-gating block is duplicated verbatim in two sibling commands (should be centralized in the shared base class), the disabled-feature message hardcodes the feature key, a CLI-level exit code is misplaced on the compatibility contract, an undocumented breaking change was introduced to the recreate exit code (4 → 3), and — most importantly — the operator upgrade test enables the ROLLING_UPDATES flag only in the RECREATE branch where it has no effect, meaning the rolling-update scenario is never actually tested with the new feature enabled. The PREVIEW vs PREVIEW_DISABLED_BY_DEFAULT type choice for the new flag is also flagged as a design-consistency question. These are not all blocking, but the test gap and the exit-code change are significant and warrant fixes or explicit documentation before merging.

These might need a close look

  • 🔴 operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java (L115)
    In createInitialDeployment, ROLLING_UPDATES is enabled only inside the RECREATE branch, which is a no-op because AlwaysRecreateUpgradeLogic ignores the feature flag. The default (null) strategy — which is the rolling-updates scenario the feature is meant to gate — never enables the feature. As a result, the rolling-update behavior introduced by this MR is never actually tested, and the test would start failing/asserting the wrong behavior once the operator is wired to gate rolling on the feature. Move the feature enablement to the default (rolling) path (e.g. before the early return) and consider adding a case that deliberately exercises the feature.

Worth checking

  • 🟡 quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.java (L46)
    The feature-gating block (checking Profile.isFeatureEnabled(Profile.Feature.ROLLING_UPDATES), printing the disabled message, and exiting with CompatibilityResult.FEATURE_DISABLED) is duplicated verbatim in both UpdateCompatibilityCheck.run() and UpdateCompatibilityMetadata.run(). Since printFeatureDisabled() was just added to the shared AbstractUpdatesCommand base and these are the only two subclasses, this should be centralized in the base class to prevent drift.
  • 🟡 quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java (L34)
    RECREATE_UPGRADE_EXIT_CODE was silently changed from 4 to 3. This is a breaking change for any script that branches on the recreate exit code. Since 4 did not collide with picocli's reserved codes (1 and 2), the renumbering is not strictly necessary. Either keep 4 for recreate and use a different code for the new feature-disabled case, or explicitly document the breaking change and explain both custom codes in the constant comments.
Small things (take or leave)
  • 🔵 common/src/main/java/org/keycloak/common/Profile.java (L136)
    ROLLING_UPDATES is declared as Type.PREVIEW, which auto-enables it when the preview profile is selected, even though the documentation and operator test instruct users to explicitly enable it (--features=rolling-updates). If the intent is explicit opt-in only, consider PREVIEW_DISABLED_BY_DEFAULT. Not a bug, but confirm the intended semantics.
  • 🔵 quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractUpdatesCommand.java (L84)
    printFeatureDisabled() hardcodes the literal 'rolling-updates' in the message. Prefer deriving it from the enum (Profile.Feature.ROLLING_UPDATES.getKey()) so the message cannot go stale. Also consider adding guidance on how to enable the feature, e.g. "Enable it with --features=rolling-updates or the preview profile."
  • 🔵 quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java (L37)
    FEATURE_DISABLED is a CLI-level gate exit code but is placed on the CompatibilityResult interface, which is the contract returned by CompatibilityManager.isCompatible(...). It can never be produced by the compatibility logic and leaks a CLI concern into the core contract. Consider moving it to the command layer alongside printFeatureDisabled().

✅ Feature-level checklist

Partially covered

  • ⚠️ Introduce a feature flag rolling-updates that gates the rolling update behavior as described in the PR.
    The rolling-updates preview feature flag is declared in the Profile enum and the update/compatibility commands do gate on it, so the core requirement is implemented. However, the operator upgrade test never actually enables the flag for the rolling-update scenario (it is only set in the RECREATE branch where it has no effect), meaning the feature's behavior is not verified by the test suite. Additionally, a silent breaking change to an exit code and several code-quality issues should be addressed. The feature is usable, but the implementation is not fully validated and has a few design/consistency caveats.