Keycloak/PR #4

Feature flag: rolling-updates

keycloak/keycloak#36882
Golden recall: 100% Extra findings: 4

Golden Comments (1/1 found)

#CommentSeverityStatusFriendlyReviewer Detail
#1Incorrect method call for exit codes. picocli.exit() calls System.exit() directly, which is problematic in test mode HIGH✓ Found FR found the bug and dug deeper: picocli.exit() does NOT call System.exit in test mode, so cmd.execute() returns 0 instead of 4 — the test passes when it should fail. Upgraded from Medium to HIGH.

Supplementary Findings (6 findings)

FindingFileSeverityLegitimate?
BREAKING CHANGE: RECREATE_UPGRADE_EXIT_CODE changed from 4 to 3 — any code checking literal 4 will misinterpret itCompatibilityResult.java:32HIGH✓ Real undocumented breaking change
Feature-spec setup in UpgradeTest placed after early return for null strategy — rolling-update flag not activated for that testUpgradeTest.java:104HIGH✓ Critical test bug
Boilerplate feature-gating duplicated in both subcommands — should be centralized in AbstractUpdatesCommandAbstractUpdatesCommand.java:43MEDIUM✓ Unnecessary duplication
Inconsistent exit behavior: UpdateCompatibilityCheck calls picocli.exit(), UpdateCompatibilityMetadata does notUpdateCompatibilityMetadata.java:54MEDIUM✓ Both should follow same pattern
printFeatureDisabled() hardcodes "rolling-updates" and is package-private — should be parameterizable and protectedAbstractUpdatesCommand.java:83LOW✓ Lack of flexibility
testImageChange parameterized but always calls eventuallyRecreateUpgradeStatus regardless of parameterUpgradeTest.java:66LOW✓ Unused parameter

Honest Analysis

FriendlyReviewer detected the single golden bug and actually analyzed it more deeply: the golden described picocli.exit() as calling System.exit(), but FriendlyReviewer correctly noted that in test mode the call is a no-op, so cmd.execute() returns 0 instead of 4 — the test passes for the wrong reason.

The 6 supplementary findings include two real HIGH-severity issues: the breaking change from exit code 4 to 3 (any consumer checking the literal value 4 will break), and the operator test bug where the feature flag setup is placed after an early return, making the test meaningless.

The finding about inconsistent exit behavior between the two subcommands is a legitimate design concern — one calls picocli.exit(), the other does not, and there is no justification for the difference.

FriendlyReviewer's full review

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

friendly-reviewerbotcommented now

Hi there 👋

🌥️ Tech
3 high, 2 medium, 2 low

The merge request introduces a ROLLING_UPDATES preview feature flag and associated CLI commands (UpdateCompatibilityCheck, UpdateCompatibilityMetadata) with an AbstractUpdatesCommand base class, plus an operator integration test (UpgradeTest). While the core feature addition is sound, several issues require attention:

  • Breaking change: CompatibilityResult.RECREATE_UPGRADE_EXIT_CODE changed from 4 to 3; any consumer checking the literal value 4 will misinterpret it.
  • Test mode exit code masking: The feature-disabled path in both CLI commands uses picocli.exit() which is a no-op in test mode, causing tests to incorrectly pass with exit code 0 instead of 4.
  • Duplicated feature-gating boilerplate: Both subcommands repeat identical logic that should be centralized in AbstractUpdatesCommand.
  • Hardcoded feature name: printFeatureDisabled() hardcodes "rolling-updates" and is package-private, hindering reuse.
  • Inconsistent exit behavior: UpdateCompatibilityCheck explicitly calls picocli.exit() on success, while UpdateCompatibilityMetadata does not.
  • Critical operator test bug: UpgradeTest sets FeatureSpec after an early return for null strategy, so the rolling-update feature is not enabled for that test case, causing expected failures.
  • Minor redundancy: testImageChange parameterization is unused since it always calls eventuallyRecreateUpgradeStatus.

The Profile.java change adding the ROLLING_UPDATES enum constant is correct and follows conventions.