refactor: call existing helpers instead of re-implementing them - #3544
refactor: call existing helpers instead of re-implementing them#3544csviri wants to merge 1 commit into
Conversation
No behavior change; each site is replaced by a helper that already exists. - PrimaryUpdateAndCacheUtils#compareResourceVersions (and its private validateResourceVersion) duplicated the entire algorithm of ReconcilerUtilsInternal#validateAndCompareResourceVersions: the length-first compare, the empty check, the leading-zero check and the same exception messages. It had no production caller, so the copy could silently drift from the version all production paths use. Delegate instead. - addFinalizerWithSSA builds its bare SSA skeleton with HasMetadata#initNameAndNamespaceFrom, which ResourceOperations already uses for the same purpose and which is Namespaced-aware. - AbstractInformerPool formats the informer identifier with ReconcilerUtilsInternal#getResourceTypeNameWithVersion instead of concatenating the resource name and version by hand. - EventFilterWindow uses ExtendedResourceEvent#getResourceVersion, which had no callers even though it is exactly the expression used here. - LocallyRunOperatorExtension instantiates reconcilers with Utils#instantiate, so it also supports non-public no-arg constructors and reports the failing class instead of wrapping in a bare RuntimeException.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Refactors several call sites in the Java Operator SDK to delegate to existing helper methods instead of duplicating logic, aiming to reduce drift risk and keep behavior consistent across the codebase.
Changes:
- Delegate resourceVersion validation/comparison to
ReconcilerUtilsInternal.validateAndCompareResourceVersions(...)instead of maintaining a duplicate implementation. - Use shared helper methods for common formatting / metadata initialization (
getResourceTypeNameWithVersion,initNameAndNamespaceFrom,ExtendedResourceEvent#getResourceVersion). - Instantiate reconcilers in
LocallyRunOperatorExtensionviaUtils.instantiate(...)to match core instantiation behavior and improve error reporting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| operator-framework-junit/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java | Switch reconciler instantiation to Utils.instantiate for consistent reflective construction and better error reporting. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPool.java | Use ReconcilerUtilsInternal.getResourceTypeNameWithVersion for consistent informer identification formatting. |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/EventFilterWindow.java | Replace inline resourceVersion extraction/parsing with ExtendedResourceEvent#getResourceVersion(). |
| operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java | Replace duplicated resourceVersion comparison logic with delegation, and use initNameAndNamespaceFrom for SSA skeleton metadata. |
No behavior change; each site is replaced by a helper that already exists.
validateResourceVersion) duplicated the entire algorithm of
ReconcilerUtilsInternal#validateAndCompareResourceVersions: the length-first
compare, the empty check, the leading-zero check and the same exception
messages. It had no production caller, so the copy could silently drift from
the version all production paths use. Delegate instead.
HasMetadata#initNameAndNamespaceFrom, which ResourceOperations already uses
for the same purpose and which is Namespaced-aware.
ReconcilerUtilsInternal#getResourceTypeNameWithVersion instead of
concatenating the resource name and version by hand.
no callers even though it is exactly the expression used here.
so it also supports non-public no-arg constructors and reports the failing
class instead of wrapping in a bare RuntimeException.
Quality-only change: no intended behavior difference. Cut from
nextandtouches a disjoint set of files from the sibling cleanup PRs, so it can be merged
independently and in any order.
Verified on this branch alone:
mvn -o -pl operator-framework-core,operator-framework-junit -am test(693 core + 6 junit tests, no failures) and
mvn spotless:check.