[platform] Add alphabetical sorting to registry resource lists #1764
Conversation
Add sorting helper package with generic functions for sorting resources by name (cluster-scoped) or namespace/name (namespace-scoped). Apply sorting to: - TenantNamespace (by name) - TenantSecret (refactored to use helper) - TenantModule (new) - Application (new) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Remove unused helmReleaseGVR variables - Fix non-constant format strings in klog.Errorf calls - Simplify embedded field selectors (ListMeta, ObjectMeta, Generic) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRemoved hard-coded GVRs, standardized ResourceVersion/UID sourcing from Application/Tenant objects, introduced reusable generic sorting helpers, applied consistent alphabetical sorting across several list endpoints, adjusted table ResourceVersion assignments, tightened some logging/error handling, and added unit tests for sorting and list ordering. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)pkg/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (3)
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 |
Summary of ChangesHello @lexfrei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience and consistency across the platform by ensuring that various registry resource lists are always presented in a predictable, alphabetically sorted order. It achieves this by introducing a new, generic sorting utility package and integrating it into the TenantNamespace, TenantSecret, TenantModule, and Application list operations. Additionally, the PR improves code quality by resolving several identified linter errors, making the codebase cleaner and more maintainable. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable abstraction for sorting registry resources, which greatly improves code reuse and maintainability. The introduction of the pkg/registry/sorting package with generic helpers is a solid approach. The accompanying tests for the new sorting logic are also well-written and appreciated. Additionally, fixing pre-existing linter errors improves the overall code quality. My review includes a couple of suggestions to make the new sorting functions more efficient and idiomatic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/registry/core/tenantmodule/rest.go (1)
589-605: Minor inconsistency in ResourceVersion access.For
TenantModuleList, direct field access is used (obj.ResourceVersion), while forTenantModule, the getter method is used (obj.GetResourceVersion()). Both work correctly due to embeddedObjectMeta, but for consistency, consider using the same access pattern.🔎 Suggested consistency fix
case *corev1alpha1.TenantModuleList: table = r.buildTableFromTenantModules(obj.Items) - table.ResourceVersion = obj.ResourceVersion + table.ResourceVersion = obj.GetResourceVersion() case *corev1alpha1.TenantModule: table = r.buildTableFromTenantModule(*obj) table.ResourceVersion = obj.GetResourceVersion()pkg/registry/apps/application/rest.go (1)
951-967: Minor inconsistency in ResourceVersion access.Similar to the TenantModule case,
ApplicationListuses direct field access (obj.ResourceVersion) while singleApplicationuses the getter (obj.GetResourceVersion()). Both work correctly, but consistency would improve readability.🔎 Suggested consistency fix
case *appsv1alpha1.ApplicationList: table = r.buildTableFromApplications(obj.Items) - table.ResourceVersion = obj.ResourceVersion + table.ResourceVersion = obj.GetResourceVersion() case *appsv1alpha1.Application: table = r.buildTableFromApplication(*obj) table.ResourceVersion = obj.GetResourceVersion()pkg/registry/sorting/sort.go (2)
29-38: Consider usingcmp.Comparefor cleaner comparison logic.Go 1.21 introduced
cmp.Comparewhich directly returns -1, 0, or 1 for ordered types, simplifying the comparison logic.🔎 Proposed refactor
import ( + "cmp" "slices" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" )func ByName[T any, PT interface { *T NameGetter }](items []T) { slices.SortFunc(items, func(a, b T) int { pa, pb := PT(&a), PT(&b) - switch { - case pa.GetName() < pb.GetName(): - return -1 - case pa.GetName() > pb.GetName(): - return 1 - } - return 0 + return cmp.Compare(pa.GetName(), pb.GetName()) }) }
47-58: Same simplification applies here withcmp.Compare.The comparison logic can be simplified using
cmp.Comparefor consistency with the suggested refactor forByName.🔎 Proposed refactor
func ByNamespacedName[T any, PT interface { *T NamespaceGetter }](items []T) { slices.SortFunc(items, func(a, b T) int { pa, pb := PT(&a), PT(&b) aKey := pa.GetNamespace() + "/" + pa.GetName() bKey := pb.GetNamespace() + "/" + pb.GetName() - switch { - case aKey < bKey: - return -1 - case aKey > bKey: - return 1 - } - return 0 + return cmp.Compare(aKey, bKey) }) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pkg/registry/apps/application/rest.gopkg/registry/apps/application/rest_defaulting.gopkg/registry/apps/application/rest_sorting_test.gopkg/registry/core/tenantmodule/rest.gopkg/registry/core/tenantmodule/rest_test.gopkg/registry/core/tenantnamespace/rest.gopkg/registry/core/tenantnamespace/rest_test.gopkg/registry/core/tenantsecret/rest.gopkg/registry/sorting/sort.gopkg/registry/sorting/sort_test.go
🧰 Additional context used
📓 Path-based instructions (2)
pkg/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory
Files:
pkg/registry/core/tenantmodule/rest.gopkg/registry/sorting/sort_test.gopkg/registry/apps/application/rest_sorting_test.gopkg/registry/core/tenantnamespace/rest.gopkg/registry/sorting/sort.gopkg/registry/core/tenantmodule/rest_test.gopkg/registry/core/tenantnamespace/rest_test.gopkg/registry/core/tenantsecret/rest.gopkg/registry/apps/application/rest_defaulting.gopkg/registry/apps/application/rest.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do NOT manually edit vendor/ directory or auto-generated files (zz_generated.*.go)
Files:
pkg/registry/core/tenantmodule/rest.gopkg/registry/sorting/sort_test.gopkg/registry/apps/application/rest_sorting_test.gopkg/registry/core/tenantnamespace/rest.gopkg/registry/sorting/sort.gopkg/registry/core/tenantmodule/rest_test.gopkg/registry/core/tenantnamespace/rest_test.gopkg/registry/core/tenantsecret/rest.gopkg/registry/apps/application/rest_defaulting.gopkg/registry/apps/application/rest.go
🧠 Learnings (1)
📚 Learning: 2025-12-25T09:45:26.524Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory
Applied to files:
pkg/registry/core/tenantmodule/rest.gopkg/registry/core/tenantsecret/rest.go
🧬 Code graph analysis (5)
pkg/registry/core/tenantmodule/rest.go (1)
pkg/registry/sorting/sort.go (1)
ByNamespacedName(43-59)
pkg/registry/apps/application/rest_sorting_test.go (1)
pkg/registry/sorting/sort.go (1)
ByNamespacedName(43-59)
pkg/registry/core/tenantnamespace/rest.go (1)
pkg/registry/sorting/sort.go (1)
ByName(25-39)
pkg/registry/core/tenantmodule/rest_test.go (1)
pkg/registry/sorting/sort.go (1)
ByNamespacedName(43-59)
pkg/registry/apps/application/rest.go (2)
pkg/registry/sorting/sort.go (1)
ByNamespacedName(43-59)pkg/apis/apps/v1alpha1/types.go (1)
Application(66-74)
🔇 Additional comments (21)
pkg/registry/core/tenantnamespace/rest_test.go (1)
12-39: LGTM! Well-structured test for alphabetical sorting.The test correctly validates that
makeListsortsTenantNamespaceitems alphabetically by name. The test setup with out-of-order input and explicit expected order verification is appropriate.pkg/registry/core/tenantsecret/rest.go (3)
30-31: LGTM! Import for sorting utility.The import of the sorting package aligns with the PR's goal of consolidating sorting logic.
278-282: LGTM! Sorting applied correctly after list construction.The sorting is appropriately applied after all items are collected, ensuring deterministic ordering by namespace/name.
469-481: LGTM! ResourceVersion assignment simplified.The change from
tbl.ListMeta.ResourceVersiontotbl.ResourceVersionis correct. Themetav1.Tablestruct embedsListMeta, soResourceVersionis directly accessible and this is the idiomatic way to set it.pkg/registry/apps/application/rest_sorting_test.go (1)
14-35: LGTM! Comprehensive test for namespace/name sorting.The test correctly validates the
ByNamespacedNamesorting behavior forApplicationitems, covering the multi-key sorting scenario (namespace first, then name within namespace).pkg/registry/sorting/sort_test.go (2)
11-17: LGTM! Test types properly satisfy sorting interfaces.The test types embed
metav1.ObjectMetawhich providesGetName()andGetNamespace()methods, correctly satisfying theNameGetterandNamespaceGetterinterfaces required by the sorting functions.
19-60: LGTM! Comprehensive unit tests for sorting utilities.Both
TestByNameandTestByNamespacedNameprovide good coverage with clear input/expected-output patterns. The tests validate the core sorting behavior that the rest of the PR depends on.pkg/registry/core/tenantmodule/rest_test.go (1)
14-35: LGTM! Consistent test pattern for TenantModule sorting.The test follows the established pattern for validating
ByNamespacedNamesorting, ensuring consistency across resource types.pkg/registry/core/tenantmodule/rest.go (3)
43-45: LGTM! Import for sorting utility.The import is correctly placed and aligns with the PR's consolidation of sorting logic.
283-288: LGTM! Sorting applied after list construction.The sorting is correctly applied after all items are collected and before returning the list, ensuring deterministic ordering.
509-517: LGTM! Simplified error logging.The change from
klog.Errorf("...: %v", err)toklog.Error(err)is appropriate since the error message is already self-contained and logging the error directly provides the necessary context.pkg/registry/core/tenantnamespace/rest.go (3)
29-30: LGTM! Import for sorting utility.The import is correctly placed for the sorting package.
205-217: LGTM! ResourceVersion assignment simplified.The direct
tbl.ResourceVersionassignment is correct and more idiomatic than going throughListMeta.
258-261: LGTM! Correct use of ByName for cluster-scoped resource.Since
TenantNamespaceis cluster-scoped (perNamespaceScoped() bool { return false }on line 75), usingByNamerather thanByNamespacedNameis the correct choice.pkg/registry/apps/application/rest.go (5)
45-52: LGTM! Import for sorting utility.The import is correctly placed alongside other internal package imports.
389-394: LGTM! Sorting applied after list construction.The sorting is correctly applied after all items are collected, ensuring deterministic alphabetical ordering by namespace/name for API responses.
439-443: LGTM! Simplified error handling.The consolidated error logging and return statement is cleaner and avoids redundant message variable construction.
751-759: LGTM! Simplified error logging.Using
klog.Error(err)directly is appropriate when the error message is self-explanatory.
906-913: LGTM! Direct field access for ResourceVersion and UID.The change from
app.ObjectMeta.ResourceVersiontoapp.ResourceVersion(and similarly for UID) is functionally equivalent due to Go's embedded struct promotion and is the more idiomatic access pattern.pkg/registry/sorting/sort.go (1)
61-74: LGTM!The
ObjectMetaWrappercorrectly implementsNamespaceGetterby wrappingmetav1.ObjectMeta. The methods are simple delegations to the embedded struct's fields.pkg/registry/apps/application/rest_defaulting.go (1)
75-75: No issues found. The field access changes (froms.Generic.Typetos.Typeand froms.Generic.Default.Objecttos.Default.Object) are compatible with k8s.io/apiextensions-apiserver v0.34.1 and have been successfully applied. The commit message confirms this was an intentional refactoring to simplify embedded field selectors, and the test suite validates that the defaulting logic works correctly.
- Simplify ByName with strings.Compare - Avoid string allocation in ByNamespacedName by comparing fields separately Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/registry/sorting/sort.go (1)
51-64: Consider removing redundant method definitions.
metav1.ObjectMetaalready implementsGetName()andGetNamespace()methods (via themetav1.Objectinterface). SinceObjectMetais embedded, these methods are automatically promoted toObjectMetaWrapper, making the explicit definitions on lines 57-64 unnecessary.🔎 Proposed simplification
// ObjectMetaWrapper wraps metav1.ObjectMeta to implement NamespaceGetter. type ObjectMetaWrapper struct { metav1.ObjectMeta } - -// GetName returns the name. -func (o *ObjectMetaWrapper) GetName() string { - return o.Name -} - -// GetNamespace returns the namespace. -func (o *ObjectMetaWrapper) GetNamespace() string { - return o.Namespace -}The embedded
ObjectMetaalready provides these methods, so the wrapper will satisfyNamespaceGetterwithout explicit definitions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/registry/sorting/sort.go
🧰 Additional context used
📓 Path-based instructions (2)
pkg/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory
Files:
pkg/registry/sorting/sort.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do NOT manually edit vendor/ directory or auto-generated files (zz_generated.*.go)
Files:
pkg/registry/sorting/sort.go
🔇 Additional comments (3)
pkg/registry/sorting/sort.go (3)
13-22: Clean interface design with proper composition.The interface hierarchy is well-designed:
NamespaceGetterembeddingNameGetterallows theByNamespacedNamefunction to work with any type that provides both methods while keeping the interfaces minimal.
26-34: LGTM!The generic constraint and use of
strings.Compareis idiomatic. Past review feedback has been addressed.
38-49: LGTM!The implementation efficiently compares namespace first with early return, avoiding string concatenation allocations. Past review feedback has been addressed.
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.39
git worktree add -d .worktree/backport-1764-to-release-0.39 origin/release-0.39
cd .worktree/backport-1764-to-release-0.39
git switch --create backport-1764-to-release-0.39
git cherry-pick -x 0b16f83eae90b09667db2d9b5621d72609558aec 19e06f7beb59df87ac6e2b8281ef3cef6d072b2d e9cad4507be9e1118dc94b7ff493e036817c10d3 da56c88aa76064ea29177c4bf44db3f1df27e9e4 |
## What this PR does Adds deterministic alphabetical sorting to all registry resource lists: - TenantNamespace (sorted by name) - TenantSecret (sorted by namespace/name) - TenantModule (sorted by namespace/name) - Application (sorted by namespace/name) Introduces a new `pkg/registry/sorting` package with generic helper functions to avoid code duplication. Also fixes pre-existing linter errors: - Unused `helmReleaseGVR` variables - Non-constant format strings in `klog.Errorf` calls - Redundant embedded field selectors ### Release note ```release-note [platform] Registry resource lists are now returned in alphabetical order ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a reusable alphabetical sorting utility and applied consistent sorting across list endpoints. * **Bug Fixes** * Ensured ResourceVersion/UID are sourced correctly for accurate list/table responses. * Simplified and improved error logging and table ResourceVersion handling. * Made schema defaulting behavior more consistent. * **Tests** * Added unit tests validating alphabetical sorting for multiple resource types. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## What this PR does Adds deterministic alphabetical sorting to all registry resource lists: - TenantNamespace (sorted by name) - TenantSecret (sorted by namespace/name) - TenantModule (sorted by namespace/name) - Application (sorted by namespace/name) Introduces a new `pkg/registry/sorting` package with generic helper functions to avoid code duplication. Also fixes pre-existing linter errors: - Unused `helmReleaseGVR` variables - Non-constant format strings in `klog.Errorf` calls - Redundant embedded field selectors ### Release note ```release-note [platform] Registry resource lists are now returned in alphabetical order ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a reusable alphabetical sorting utility and applied consistent sorting across list endpoints. * **Bug Fixes** * Ensured ResourceVersion/UID are sourced correctly for accurate list/table responses. * Simplified and improved error logging and table ResourceVersion handling. * Made schema defaulting behavior more consistent. * **Tests** * Added unit tests validating alphabetical sorting for multiple resource types. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## What this PR does Adds deterministic alphabetical sorting to all registry resource lists: - TenantNamespace (sorted by name) - TenantSecret (sorted by namespace/name) - TenantModule (sorted by namespace/name) - Application (sorted by namespace/name) Introduces a new `pkg/registry/sorting` package with generic helper functions to avoid code duplication. Also fixes pre-existing linter errors: - Unused `helmReleaseGVR` variables - Non-constant format strings in `klog.Errorf` calls - Redundant embedded field selectors ### Release note ```release-note [platform] Registry resource lists are now returned in alphabetical order ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a reusable alphabetical sorting utility and applied consistent sorting across list endpoints. * **Bug Fixes** * Ensured ResourceVersion/UID are sourced correctly for accurate list/table responses. * Simplified and improved error logging and table ResourceVersion handling. * Made schema defaulting behavior more consistent. * **Tests** * Added unit tests validating alphabetical sorting for multiple resource types. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Adds deterministic alphabetical sorting to all registry resource lists:
Introduces a new
pkg/registry/sortingpackage with generic helper functionsto avoid code duplication.
Also fixes pre-existing linter errors:
helmReleaseGVRvariablesklog.ErrorfcallsRelease note
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.