Add rolling-updates feature flag and compatibility framework

[ShashankFC]

Test 4nn---nReplicated from ai-code-review-evaluation/keycloak-greptile#4

[GitHoobar]

@claude

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 2m 2s —— View job

---

Code Review Complete ✓

• Gather context and understand repository structure
• Review PR changes against base branch
• Analyze code quality and identify issues
• Provide comprehensive feedback

---

Summary

This PR adds the rolling-updates feature flag to Keycloak's profile system and updates related compatibility commands, tests, and documentation. The implementation is well-structured and follows Keycloak's existing patterns for feature flags.

Positive Observations

1. Consistent Implementation: The feature flag follows the established pattern in Profile.java, properly declared as Type.PREVIEW
2. Comprehensive Testing: Test coverage updated in both UpgradeTest.java and UpdateCommandDistTest.java
3. Documentation: Clear warnings added to documentation about the preview status of the feature
4. Exit Code Management: Good use of semantic exit codes in CompatibilityResult (0=OK, 3=Recreate, 4=Feature Disabled)

Issues and Concerns

1. Inconsistent Trailing Comma (Style)

Location: common/src/main/java/org/keycloak/common/Profile.java:136-137

ROLLING_UPDATES("Rolling Updates", Type.PREVIEW),
;

The trailing comma after ROLLING_UPDATES is inconsistent with other enum entries in the file. While this works in Java, it's stylistically unusual. Most enum declarations either:

• Have no trailing comma on the last entry, OR
• Place the semicolon on the same line

Recommendation: Consider removing the trailing comma for consistency:

ROLLING_UPDATES("Rolling Updates", Type.PREVIEW);

2. Feature Gate Check Duplication

Location:

• quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityCheck.java:46-50
• quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command/UpdateCompatibilityMetadata.java:46-50

Both commands have identical feature gate checking logic:

if (!Profile.isFeatureEnabled(Profile.Feature.ROLLING_UPDATES)) {
    printFeatureDisabled();
    picocli.exit(CompatibilityResult.FEATURE_DISABLED);
    return;
}

Recommendation: This could be abstracted to AbstractUpdatesCommand to reduce duplication:

protected boolean checkRollingUpdatesFeature() {
    if (!Profile.isFeatureEnabled(Profile.Feature.ROLLING_UPDATES)) {
        printFeatureDisabled();
        picocli.exit(CompatibilityResult.FEATURE_DISABLED);
        return false;
    }
    return true;
}

3. Test Method Naming Convention

Location: operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java:48-53

The upgradeStrategy() method returns update strategies but uses "upgrade" terminology, which could be confusing given that the actual enum is called UpdateStrategy.

Recommendation: Consider renaming to updateStrategy() for consistency with the enum name.

4. Magic String in Documentation Template

Location: docs/guides/templates/kc.adoc:53

bin/kc.[sh|bat] update-compatibility ${parameters} --features=rolling-updates

The feature name is hardcoded in the template. If the feature key changes in the future, this would need manual updating.

Minor Issue: While unlikely to change, consider whether this should reference Profile.Feature.ROLLING_UPDATES.getKey() or be documented in a way that makes maintenance easier.

5. Potential User Experience Issue

Location: Exit code handling in commands

The feature returns exit code 4 when disabled, but the error message could be more helpful. Currently:

"Unable to use this command. The preview feature 'rolling-updates' is not enabled."

Recommendation: Consider adding guidance on how to enable the feature:

"Unable to use this command. The preview feature 'rolling-updates' is not enabled. Enable it with --features=rolling-updates"

6. Missing Null Check in Test

Location: operator/src/test/java/org/keycloak/operator/testsuite/integration/UpgradeTest.java:112-114

if (kc.getSpec().getFeatureSpec() == null) {
    kc.getSpec().setFeatureSpec(new FeatureSpec());
}

This defensive check is good, but there's no similar check before accessing other spec fields (e.g., line 104: kc.getSpec().setInstances(3)). Either all spec accesses should be guarded, or the test setup should guarantee non-null specs.

Code Quality Assessment

Category	Rating	Notes
Correctness	✅ Good	Logic appears sound, proper feature flag usage
Testing	✅ Good	Comprehensive test coverage for both success and failure cases
Documentation	✅ Good	Clear warnings about preview status
Style	⚠️ Minor Issues	Trailing comma inconsistency
Maintainability	⚠️ Minor Issues	Some code duplication in feature checks

Recommendations Priority

1. High: None - the code is functionally correct
2. Medium: Refactor duplicate feature gate checks into base class
3. Low: Fix trailing comma style, improve error messages with usage hints

Testing Notes

The test coverage is comprehensive:

• ✅ Feature disabled scenarios (testFeatureNotEnabled)
• ✅ Missing required options (testMissingOptionOnCheck, testMissingOptionOnSave)
• ✅ Compatible version scenarios (testCompatible)
• ✅ Incompatible version scenarios (testWrongVersions)
• ✅ Integration tests for operator upgrade paths

Security Considerations

No security concerns identified. The feature flag properly gates preview functionality and uses appropriate exit codes for scripting.

Conclusion

This is a solid implementation of a feature flag system for rolling updates. The changes are well-tested and properly documented. The identified issues are minor and mostly related to code style and potential future maintainability improvements rather than functional problems.

Overall Assessment: ✅ Approve with minor suggestions

---