Feature flag: rolling-updates - #1
Open
linxia0415 wants to merge 6 commits into
Open
Conversation
Closes #36840 Signed-off-by: Pedro Ruivo <pruivo@redhat.com>
Signed-off-by: Pedro Ruivo <pruivo@redhat.com>
Signed-off-by: Pedro Ruivo <pruivo@redhat.com>
Signed-off-by: Pedro Ruivo <pruivo@redhat.com>
📝 WalkthroughWalkthroughThis PR introduces the ROLLING_UPDATES preview feature with full enforcement coverage. A new enum value is declared in Profile, CLI commands are guarded to reject execution if the feature is disabled (returning exit code 4), documentation requires the feature during the preview phase, and build/test environments enable it automatically. ChangesRolling Updates Feature Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java (1)
105-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnable
rolling-updatesbefore the null-strategy early return.Line 105 returns before Lines 112-115, so the
updateStrategy == nullbranch never enables the preview feature. That leaves the default-path cases intestImageChange()/testCacheMaxCount()running withoutrolling-updates, which undermines the rolling-upgrade path this PR is trying to cover.Suggested fix
kc.getSpec().setInstances(3); + if (kc.getSpec().getFeatureSpec() == null) { + kc.getSpec().setFeatureSpec(new FeatureSpec()); + } + kc.getSpec().getFeatureSpec().setEnabledFeatures(List.of(Profile.Feature.ROLLING_UPDATES.getKey())); + if (updateStrategy == null) { return kc; } var updateSpec = new UpdateSpec(); updateSpec.setStrategy(updateStrategy); kc.getSpec().setUpdateSpec(updateSpec); - - if (kc.getSpec().getFeatureSpec() == null) { - kc.getSpec().setFeatureSpec(new FeatureSpec()); - } - kc.getSpec().getFeatureSpec().setEnabledFeatures(List.of(Profile.Feature.ROLLING_UPDATES.getKey())); return kc;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java` around lines 105 - 116, The early return when updateStrategy == null prevents enabling the preview feature; move the block that ensures kc.getSpec().getFeatureSpec() is non-null and sets kc.getSpec().getFeatureSpec().setEnabledFeatures(List.of(Profile.Feature.ROLLING_UPDATES.getKey())) to occur before the if (updateStrategy == null) return kc; check so rolling-updates is always enabled for tests like testImageChange() and testCacheMaxCount(), while leaving creation of and assignment to UpdateSpec (updateSpec.setStrategy(updateStrategy); kc.getSpec().setUpdateSpec(updateSpec);) only in the non-null branch.
🧹 Nitpick comments (2)
docs/guides/operator/advanced-configuration.adoc (1)
446-450: ⚡ Quick winConsider specifying what "will fail" means.
The CAUTION states the operator "will fail" if the feature is not enabled, but doesn't describe how users will experience this failure (error message, exit code, etc.). Being more specific would help users troubleshoot.
📝 Suggested clarification
[CAUTION] ==== -While on preview stage, the feature `rolling-updates` must be enabled. -Otherwise, the {project_name} Operator will fail. +While on preview stage, the feature `rolling-updates` must be enabled. +Otherwise, update compatibility commands will exit with code 4 and the error message "The preview feature 'rolling-updates' is not enabled." ====🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/guides/operator/advanced-configuration.adoc` around lines 446 - 450, Update the CAUTION block about `rolling-updates` to specify the observable failure mode: state whether the Operator exits, crashes, or logs a specific error; include the exact error message or log line users will see (or the exit code) and the symptoms (e.g., reconciliation loop stops, Pods stuck in CrashLoopBackOff, Operator logs show "feature flag rolling-updates not enabled" or similar). Keep the warning concise and actionable and add a short remediation step referencing enabling the `rolling-updates` feature flag and where to check logs (Operator pod logs) for the named error.quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java (1)
31-35: ⚡ Quick winConsider documenting exit code 0 for completeness.
The comment block documents picocli exit codes 1 and 2, and custom codes 3 and 4, but doesn't mention that exit code 0 (ROLLING_UPGRADE_EXIT_CODE) means success. Adding this would make the exit code contract fully self-documenting.
📝 Proposed addition to exit code comments
// see picocli.CommandLine.ExitCode +// 0 -> success (rolling upgrade possible) // 1 -> software error // 2 -> usage error int RECREATE_UPGRADE_EXIT_CODE = 3;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java` around lines 31 - 35, Add documentation for the success exit code by updating the comment block to include exit code 0 and its meaning (ROLLING_UPGRADE_EXIT_CODE / success). Edit the comment near the existing exit code constants (RECREATE_UPGRADE_EXIT_CODE and FEATURE_DISABLED) to explicitly state that 0 represents success (ROLLING_UPGRADE_EXIT_CODE) so the exit code contract is fully self-documenting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java`:
- Line 34: RECREATE_UPGRADE_EXIT_CODE was changed from 4 to 3 and
IncompatibleResult.exitCode() now returns RECREATE_UPGRADE_EXIT_CODE (so
recreate-upgrade will exit with 3) while 4 is now FEATURE_DISABLED; update any
external automation, CI scripts, integration tests, and documentation (including
release notes and any ops runbooks) that expect recreate-upgrade to return exit
code 4 to use exit code 3 instead, and audit references to
RECREATE_UPGRADE_EXIT_CODE and FEATURE_DISABLED to ensure docs/tests reflect the
new numeric mapping.
---
Outside diff comments:
In
`@operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java`:
- Around line 105-116: The early return when updateStrategy == null prevents
enabling the preview feature; move the block that ensures
kc.getSpec().getFeatureSpec() is non-null and sets
kc.getSpec().getFeatureSpec().setEnabledFeatures(List.of(Profile.Feature.ROLLING_UPDATES.getKey()))
to occur before the if (updateStrategy == null) return kc; check so
rolling-updates is always enabled for tests like testImageChange() and
testCacheMaxCount(), while leaving creation of and assignment to UpdateSpec
(updateSpec.setStrategy(updateStrategy);
kc.getSpec().setUpdateSpec(updateSpec);) only in the non-null branch.
---
Nitpick comments:
In `@docs/guides/operator/advanced-configuration.adoc`:
- Around line 446-450: Update the CAUTION block about `rolling-updates` to
specify the observable failure mode: state whether the Operator exits, crashes,
or logs a specific error; include the exact error message or log line users will
see (or the exit code) and the symptoms (e.g., reconciliation loop stops, Pods
stuck in CrashLoopBackOff, Operator logs show "feature flag rolling-updates not
enabled" or similar). Keep the warning concise and actionable and add a short
remediation step referencing enabling the `rolling-updates` feature flag and
where to check logs (Operator pod logs) for the named error.
In
`@quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java`:
- Around line 31-35: Add documentation for the success exit code by updating the
comment block to include exit code 0 and its meaning (ROLLING_UPGRADE_EXIT_CODE
/ success). Edit the comment near the existing exit code constants
(RECREATE_UPGRADE_EXIT_CODE and FEATURE_DISABLED) to explicitly state that 0
represents success (ROLLING_UPGRADE_EXIT_CODE) so the exit code contract is
fully self-documenting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e894df48-36ef-43d3-b8cf-bc4493b2059d
⛔ 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 (11)
.github/dependabot.ymlcommon/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
💤 Files with no reviewable changes (1)
- .github/dependabot.yml
| // see picocli.CommandLine.ExitCode | ||
| // 1 -> software error | ||
| // 2 -> usage error | ||
| int RECREATE_UPGRADE_EXIT_CODE = 3; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find references to exit code 4 in the codebase to assess breaking change impact
# Search for hardcoded references to exit code 4
rg -n -C3 '\bexit.*4\b|exitCode.*4|exit_code.*4|EXIT.*4' --type=java --type=sh --type=yaml --type=md
# Search for RECREATE_UPGRADE_EXIT_CODE usage
rg -n -C3 'RECREATE_UPGRADE_EXIT_CODE'Repository: test-code-tool/keycloak__keycloak__CodeRabbit__PR36882__20260407
Length of output: 3541
Recreate-upgrade exit code change needs an automation/docs update (4 → 3).
int RECREATE_UPGRADE_EXIT_CODE = 3;
IncompatibleResult.exitCode() now returns RECREATE_UPGRADE_EXIT_CODE (so recreate-upgrade will exit with 3), while 4 is now FEATURE_DISABLED. A codebase-wide search found no in-repo automation/docs/tests hardcoding “exit code 4” for recreate-upgrade, but any external scripts depending on exit code 4 for recreate-upgrade must be updated to 3 (and release notes/docs should be adjusted accordingly).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/compatibility/CompatibilityResult.java`
at line 34, RECREATE_UPGRADE_EXIT_CODE was changed from 4 to 3 and
IncompatibleResult.exitCode() now returns RECREATE_UPGRADE_EXIT_CODE (so
recreate-upgrade will exit with 3) while 4 is now FEATURE_DISABLED; update any
external automation, CI scripts, integration tests, and documentation (including
release notes and any ops runbooks) that expect recreate-upgrade to return exit
code 4 to use exit code 3 instead, and audit references to
RECREATE_UPGRADE_EXIT_CODE and FEATURE_DISABLED to ensure docs/tests reflect the
new numeric mapping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #36840
Summary by CodeRabbit
Release Notes
New Features
Documentation