Add rolling-updates feature flag and compatibility framework#4
Add rolling-updates feature flag and compatibility framework#4ShashankFC wants to merge 1 commit into
Conversation
Closes #36840 Signed-off-by: Pedro Ruivo <pruivo@redhat.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis change introduces a new preview feature called "rolling-updates" to the Keycloak codebase. It adds a new feature enum constant, updates documentation and build scripts to enable this feature, implements feature flag validation in compatibility commands, adjusts exit codes, and updates test infrastructure to accommodate the feature. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (2)
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.java (1)
46-50: LGTM: Consistent feature flag implementation.The feature flag check mirrors the implementation in
UpdateCompatibilityMetadata, ensuring consistent behavior across both compatibility commands. The fail-fast approach with early return is appropriate for preview features.♻️ Optional refactor: Consider consolidating feature flag check
If additional commands will need this feature flag check, consider moving it to
AbstractUpdatesCommand:+protected boolean isRollingUpdatesEnabled() { + if (!Profile.isFeatureEnabled(Profile.Feature.ROLLING_UPDATES)) { + printFeatureDisabled(); + picocli.exit(CompatibilityResult.FEATURE_DISABLED); + return false; + } + return true; +}Then in command classes:
@Override public void run() { - if (!Profile.isFeatureEnabled(Profile.Feature.ROLLING_UPDATES)) { - printFeatureDisabled(); - picocli.exit(CompatibilityResult.FEATURE_DISABLED); - return; - } + if (!isRollingUpdatesEnabled()) { + return; + } printPreviewWarning(); validateConfig(); ...This would improve maintainability if more commands are added in the future.
docs/guides/operator/advanced-configuration.adoc (1)
475-482: Consider documenting the FEATURE_DISABLED exit code.The documentation clearly shows how to enable the
rolling-updatesfeature and explains the update strategy values. However, for users writing automation scripts around these commands, it would be helpful to document that the compatibility commands exit with code 4 (FEATURE_DISABLED) when the feature is not enabled.📝 Suggested addition to document exit codes
Consider adding an exit code reference table in the compatibility/update sections:
[%autowidth] .Exit Codes |=== |Exit Code |Meaning |0 |Rolling upgrade is compatible |3 |Recreate upgrade required |4 |Feature not enabled (rolling-updates feature must be enabled) |===This would help users building automation scripts to properly handle different scenarios.
📜 Review details
Configuration used: defaults
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.javadocs/guides/operator/advanced-configuration.adocdocs/guides/server/update-compatibility.adocdocs/guides/templates/kc.adocoperator/scripts/Dockerfile-custom-imageoperator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.javaquarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractUpdatesCommand.javaquarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.javaquarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityMetadata.javaquarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java
🧰 Additional context used
🧬 Code graph analysis (2)
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityMetadata.java (1)
common/src/main/java/org/keycloak/common/Profile.java (1)
Profile(45-485)
operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java (1)
common/src/main/java/org/keycloak/common/Profile.java (1)
Profile(45-485)
🔇 Additional comments (12)
common/src/main/java/org/keycloak/common/Profile.java (1)
134-136: LGTM!The new
ROLLING_UPDATESfeature flag is correctly added as a PREVIEW feature. The trailing comma onIPA_TUURA_FEDERATIONproperly accommodates the new enum constant, and the implementation follows established conventions.operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java (2)
20-20: LGTM!The new imports are correctly added and utilized in the test setup to enable the rolling-updates feature flag.
Also applies to: 31-31, 35-35
112-115: Critical bug: Created FeatureSpec is not set on the spec.The code creates a new
FeatureSpecwhen null (line 113) but never sets it back to the spec. This means line 115 will callsetEnabledFeatureson a potentially null object, causing aNullPointerException.🐛 Proposed fix
if (kc.getSpec().getFeatureSpec() == null) { kc.getSpec().setFeatureSpec(new FeatureSpec()); } kc.getSpec().getFeatureSpec().setEnabledFeatures(List.of(Profile.Feature.ROLLING_UPDATES.getKey()));Likely an incorrect or invalid review comment.
operator/scripts/Dockerfile-custom-image (1)
5-5: LGTM!The
--features=rolling-updatesflag is correctly added to the build command, enabling the preview feature for the custom image.docs/guides/templates/kc.adoc (1)
53-53: LGTM!The
--features=rolling-updatesflag is correctly added to theupdatecompatibilitymacro, ensuring the feature is enabled when the command is invoked through the documentation examples.docs/guides/server/update-compatibility.adoc (2)
12-16: LGTM!The CAUTION block clearly communicates the preview requirement for the rolling-updates feature, warning users that commands will fail if the feature is not enabled.
132-134: The documentation at lines 132-134 is correct and consistent with the implementation. Exit code 4 is properly defined asFEATURE_DISABLEDinCompatibilityResult.java, and bothUpdateCompatibilityCheck.javaandUpdateCompatibilityMetadata.javause this constant to exit when therolling-updatesfeature is disabled. The documentation accurately reflects this behavior.quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java (2)
31-35: LGTM: Exit code constants align with picocli conventions.The comments clarify that exit codes 1 and 2 are reserved by picocli for software and usage errors respectively. Using 3 and 4 for domain-specific results (recreate upgrade and feature disabled) follows a logical progression and avoids conflicts with picocli's standard exit codes.
31-35: Exit code values are already documented and have not changed.Exit codes 0, 1, 2, 3, and 4 are all properly documented in
docs/guides/server/update-compatibility.adoc. TheRECREATE_UPGRADE_EXIT_CODE = 3andFEATURE_DISABLED = 4values were already established in the previous commit and are correctly documented as:
- Code 3: "Rolling Upgrade is not possible. The deployment must be shut down before applying the new configuration."
- Code 4: "Rolling Upgrade is not possible. The feature
rolling-updatesis disabled."No breaking change to exit codes has occurred.
Likely an incorrect or invalid review comment.
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityMetadata.java (1)
46-50: LGTM: Feature flag guard implemented correctly.The feature flag check at the start of
run()provides a fail-fast mechanism when therolling-updatesfeature is disabled. The early return prevents unnecessary validation and processing, and the exit code clearly signals the reason for termination.quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/AbstractUpdatesCommand.java (1)
83-85: LGTM: Helper method is clear and well-scoped.The
printFeatureDisabled()method provides a consistent error message when the rolling-updates feature is not enabled. The package-private visibility is appropriate for shared use within the command package, and the message clearly communicates the issue to users.docs/guides/operator/advanced-configuration.adoc (1)
446-450: LGTM: Clear warning about preview feature requirement.The CAUTION block appropriately warns users that the
rolling-updatesfeature must be enabled during the preview stage. This aligns with the runtime checks inUpdateCompatibilityCheckandUpdateCompatibilityMetadatathat exit withFEATURE_DISABLEDwhen the feature is not enabled.
2 participants
Test 4
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#4