Add rolling-updates feature flag and compatibility framework#7
Conversation
Closes #36840 Signed-off-by: Pedro Ruivo <pruivo@redhat.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces a new "ROLLING_UPDATES" preview feature to Keycloak. The feature is defined as a constant in the Feature enum, checked before execution in update compatibility commands, documented in guides, and enabled in test infrastructure. Exit codes are adjusted to accommodate a new FEATURE_DISABLED status. Changes
Sequence DiagramsequenceDiagram
participant User
participant UpdateCommand as UpdateCompatibilityCheck/<br>UpdateCompatibilityMetadata
participant Profile
participant Output
User->>UpdateCommand: Execute command
UpdateCommand->>Profile: Check if ROLLING_UPDATES enabled
alt Feature Disabled
Profile-->>UpdateCommand: Feature not enabled
UpdateCommand->>Output: printFeatureDisabled()
UpdateCommand-->>User: Exit code 4 (FEATURE_DISABLED)
else Feature Enabled
Profile-->>UpdateCommand: Feature enabled
UpdateCommand->>Output: printPreviewWarning()
UpdateCommand->>UpdateCommand: Validate config
UpdateCommand->>UpdateCommand: Compute compatibility
UpdateCommand->>Output: Write results
UpdateCommand-->>User: Exit code 0 (success)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/guides/operator/advanced-configuration.adoc (1)
475-482: Fixupdate.strategyYAML example to be valid YAMLThe example line
strategy: Recreate|<not set> # <2>is not valid YAML and may confuse users copying the snippet. Since the explanation below already documents the default and the example value, consider showing only the concrete value here.
update: - strategy: Recreate|<not set> # <2> + strategy: Recreate # <2>The annotations
<1>and<2>and the table below can continue to describe other acceptable values such as<not set>.operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java (1)
20-20: Avoid clobbering existing enabled features when turning onROLLING_UPDATESThe new block:
if (kc.getSpec().getFeatureSpec() == null) { kc.getSpec().setFeatureSpec(new FeatureSpec()); } kc.getSpec().getFeatureSpec().setEnabledFeatures(List.of(Profile.Feature.ROLLING_UPDATES.getKey()));forces
enabledFeaturesto exactly[rolling-updates]and usesList.of(...)(immutable). This is fine for the current tests, but it:
- Drops any features that
getTestKeycloakDeployment(true)might already have enabled.- Makes later mutations of
enabledFeatures(e.g., adding another preview feature in this or future tests) throw at runtime.A safer pattern is to preserve any existing entries and append
ROLLING_UPDATESonly if it is not already present, using a mutable list. Conceptually:FeatureSpec featureSpec = kc.getSpec().getFeatureSpec(); if (featureSpec == null) { featureSpec = new FeatureSpec(); kc.getSpec().setFeatureSpec(featureSpec); } List<String> enabled = featureSpec.getEnabledFeatures(); if (enabled == null) { enabled = new ArrayList<>(); } else { enabled = new ArrayList<>(enabled); } if (!enabled.contains(Profile.Feature.ROLLING_UPDATES.getKey())) { enabled.add(Profile.Feature.ROLLING_UPDATES.getKey()); } featureSpec.setEnabledFeatures(enabled);(With the necessary
ArrayListimport.)Also, note that
ROLLING_UPDATESis only enabled whenupdateStrategy != null. If the preview contract is that the feature must be enabled for any use of this new update behavior (including the default/null strategy path), you may want to move this feature setup above theif (updateStrategy == null) { return kc; }early return.Also applies to: 31-31, 35-35, 110-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/UpdateCommandDistTest.javais excluded by!**/dist/**
📒 Files selected for processing (10)
common/src/main/java/org/keycloak/common/Profile.java(1 hunks)docs/guides/operator/advanced-configuration.adoc(2 hunks)docs/guides/server/update-compatibility.adoc(2 hunks)docs/guides/templates/kc.adoc(1 hunks)operator/scripts/Dockerfile-custom-image(1 hunks)operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java(3 hunks)quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractUpdatesCommand.java(1 hunks)quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.java(2 hunks)quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityMetadata.java(2 hunks)quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java(1 hunks)
🔇 Additional comments (11)
docs/guides/templates/kc.adoc (1)
50-55: LGTM! Template correctly documents the required feature flag.The addition of
--features=rolling-updatesto the updatecompatibility macro ensures that documentation consistently shows users how to enable the required preview feature. This aligns with the CAUTION message in update-compatibility.adoc and the runtime feature checks.operator/scripts/Dockerfile-custom-image (1)
5-5: LGTM! Correctly enables the preview feature for operator builds.The addition of
--features=rolling-updatesto the build command ensures that the rolling updates feature is enabled in the operator's custom image. This aligns with the test infrastructure updates (e.g., UpgradeTest.java) that configure the feature for operator scenarios.docs/guides/server/update-compatibility.adoc (2)
12-16: LGTM! Clear warning about the preview feature requirement.The CAUTION block properly informs users that the
rolling-updatesfeature must be explicitly enabled during the preview stage. This aligns with the runtime checks that will fail if the feature is not enabled.
131-134: The review comment is based on an incorrect assumption about exit code changes.The verification shows no evidence that exit code 4 was ever used for
RECREATE_UPGRADE_EXIT_CODE. Current and documented assignments are:
- Exit code 3:
RECREATE_UPGRADE_EXIT_CODE(for shutdown-required upgrades)- Exit code 4:
FEATURE_DISABLED(for disabled rolling-updates feature)A search across the codebase for any reference combining "RECREATE" with exit code 4 returned zero results. The exit code definitions in
CompatibilityResult.javaand their usage inUpdateCompatibilityMetadata.java,UpdateCompatibilityCheck.java, andIncompatibleResult.javaare consistent with the documentation.There is no breaking change to track, and existing automation does not need updating.
Likely an incorrect or invalid review comment.
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.java (2)
23-23: LGTM! Necessary imports added for feature gating.The imports for
ProfileandCompatibilityResultsupport the new feature-flag check that gates the update-compatibility functionality.Also applies to: 25-25
46-50: LGTM! Feature check correctly gates command execution.The pre-flight check for the
ROLLING_UPDATESfeature is properly positioned at the start ofrun(), before any validation or processing. The early return pattern prevents execution when the feature is disabled, and the exit code (FEATURE_DISABLED) aligns with the documented behavior.quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractUpdatesCommand.java (1)
83-85: LGTM! Consistent error messaging helper added.The new
printFeatureDisabled()method provides a centralized, user-friendly error message when the rolling-updates feature is not enabled. The message is clear and actionable, and the package-private visibility is appropriate for internal command usage.quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityMetadata.java (2)
24-24: LGTM! Necessary imports added for feature gating.The imports for
ProfileandCompatibilityResultenable the feature-flag check, consistent with the UpdateCompatibilityCheck command.Also applies to: 26-26
46-50: LGTM! Consistent feature gating across both commands.The feature check implementation is identical to UpdateCompatibilityCheck, ensuring that both metadata generation and compatibility checking are consistently gated by the
ROLLING_UPDATESfeature flag. This prevents users from generating metadata without the feature enabled.common/src/main/java/org/keycloak/common/Profile.java (1)
134-136: LGTM! Feature flag properly defined as PREVIEW.The new
ROLLING_UPDATESfeature is correctly defined with:
- Type.PREVIEW: Appropriate for new functionality under development
- Clear label: "Rolling Updates"
- No dependencies: Correctly represents a standalone feature
- Standard version and availability: Uses defaults appropriately
The placement after
IPA_TUURA_FEDERATIONmaintains the enum structure, and the feature will be properly integrated into Keycloak's feature management system.docs/guides/operator/advanced-configuration.adoc (1)
446-451: Preview caution forrolling-updateslooks clearThe CAUTION block clearly calls out the requirement to enable the
rolling-updatespreview feature and the failure mode if it is not enabled. No changes needed from a technical perspective.
| // see picocli.CommandLine.ExitCode | ||
| // 1 -> software error | ||
| // 2 -> usage error | ||
| int RECREATE_UPGRADE_EXIT_CODE = 3; | ||
| int FEATURE_DISABLED = 4; |
There was a problem hiding this comment.
🧩 Analysis chain
Exit code changes introduce breaking change.
The restructuring of exit codes introduces a breaking change:
RECREATE_UPGRADE_EXIT_CODEchanged from 4 → 3- New
FEATURE_DISABLED= 4
While the alignment with picocli conventions (1=software error, 2=usage error) is good, any existing automation or scripts that rely on exit code 4 meaning "recreate upgrade required" will break.
Recommendations:
- Document this breaking change in release notes
- Consider providing a migration guide for automation/scripts
- Verify that internal Keycloak tooling and operator tests have been updated
Run the following script to check for any existing tests or scripts that assert on exit code 4 in the context of recreate upgrades:
🏁 Script executed:
#!/bin/bash
# Search for test assertions or scripts checking exit code 4
rg -n -C3 'exit.*4|exitCode.*4|assertEquals.*4|assertThat.*4' --type java --type sh -g '!CompatibilityResult.java' -g '!update-compatibility.adoc'Length of output: 285197
Breaking change confirmed; document and manage migration impact.
The script search found no existing test assertions on exit code 4 in the codebase, but this does not invalidate the breaking change identified. The code clearly shows the exit code reassignment: RECREATE_UPGRADE_EXIT_CODE changes from 4 to 3, with 4 now assigned to FEATURE_DISABLED. This is a confirmed breaking change.
The absence of existing tests checking exit code 4 suggests the change either lacks test coverage or the relevant tests exist in external repositories (operator, Keycloak tooling, deployment scripts). This strengthens the need for the original review recommendations:
- Document the breaking change in release notes
- Provide a migration guide for users relying on exit code 4 for recreate upgrade logic
- Verify external tooling (Keycloak operator, deployment automation) that may depend on exit code semantics
The code change itself appears intentional and correct. However, without evidence that this breaking change is documented or that downstream dependencies have been updated, this requires management and communication.
🤖 Prompt for AI Agents
In
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java
around lines 31-35, the exit code constants were reordered so
RECREATE_UPGRADE_EXIT_CODE is now 3 and 4 is FEATURE_DISABLED, which is a
confirmed breaking change; update project artifacts to manage this by (1) adding
a clear entry in the release notes documenting the exit code reassignment and
its impact, (2) creating a migration guide that shows mapping from old to new
exit codes and recommended changes for scripts/operators that relied on exit
code 4, (3) auditing and updating downstream tooling (operator, CI/deploy
scripts, automation) that may interpret exit codes, and (4) adding or updating
tests that assert the expected exit codes (including a regression test verifying
the new values) to prevent silent regressions.
2 participants
Test 4
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#4