[dashboard] Cumulative fixes - #1463
Conversation
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
WalkthroughIntroduces a new dashboard flag (module) in the API, CRD schema, and multiple CRD templates. Updates dashboard controller logic to treat module resources differently (breadcrumb, marketplace panel, sidebar, manager, helpers, prefill). Refactors static dashboard resources to kube-prefixed details and tenantnamespace paths, adds Kubernetes-related sidebars/details, and adjusts URI mappings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Reconciler
participant L as CRD Loader
participant M as Manager.buildExpectedResourceSet
participant S as Sidebar Upserter
participant MP as MarketplacePanel
participant BR as Breadcrumb
participant ST as Static Resources
R->>L: List CRDs with dashboard config
L-->>R: CRD set
R->>M: Build expected resources from CRDs
note over M: For each CRD<br/>- Always: CustomColumns/Forms/Prefill<br/>- If module: skip MarketplacePanel<br/>- Create Breadcrumb, Factory, Sidebars
M->>BR: Ensure breadcrumb (module-aware root)
alt CRD.Dashboard.Module == true
M-x MP: Skip/cleanup MarketplacePanel
else
M->>MP: Ensure MarketplacePanel
end
M->>ST: Ensure kube-* details factories/tabs
M->>S: Upsert sidebars (static + dynamic)
note over S: Static sidebars: labels only<br/>Dynamic details: ownerRef only for matching CRD
R-->>R: Cleanup orphaned resources (logs on Breadcrumb/Factory)
sequenceDiagram
autonumber
participant UI as Dashboard UI
participant API as Controller APIs
participant K as Kubernetes APIs
UI->>API: Open resource details (kind/name/ns)
API->>API: Map to kube-* details sidebar/factory
API->>K: Fetch resource (service/secret/ingress) and related
K-->>API: Resource + related objects
API-->>UI: Render tabs (Details/YAML/Rules), columns, links (tenantnamespace-aware)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 Andrei Kvapil (@kvaps), 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 introduces a series of cumulative fixes and enhancements to the dashboard, primarily focusing on how custom resources and built-in Kubernetes resources are presented and managed. Key changes include the introduction of a 'module' parameter for resource definitions, a significant refactoring of sidebar generation to better organize resources, and standardization of API group handling for applications. It also improves form prefilling, adds a dedicated factory for Ingress resources, and refines table displays for tenant namespaces, leading to a more consistent and functional user interface. Highlights
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 (@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
|
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
230e4de to
364cba3
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a collection of fixes and improvements for the dashboard functionality. Key changes include introducing a Module flag to distinguish tenant modules, which simplifies logic across several controllers. The API group for applications is now correctly fixed. Sidebars have been significantly refactored to correctly handle shared resources and introduce support for built-in Kubernetes resources like Ingress. Factories for Ingress resources and formatted tables for tenant namespaces have been added. Overall, the changes improve the dashboard's consistency and functionality. I've identified a potential bug related to resource labeling that could lead to orphaned resources, and a few areas for minor performance improvements and comment clarification.
| } else { | ||
| // This is a different CRD's sidebar, don't modify owner references or labels | ||
| // Just update the spec | ||
| } |
There was a problem hiding this comment.
There is a potential bug here. When a dynamic sidebar for a different CRD (e.g., for CRD 'B' during reconciliation of CRD 'A') is created for the first time, it will be created without any labels because this else block is empty. This will cause the resource to be missed by the cleanup logic, leading to orphaned resources if the corresponding CRD is deleted. All managed resources should have the dashboard.cozystack.io/managed-by and dashboard.cozystack.io/resource-type labels.
To fix this, the upsertMultipleSidebars function should have access to all CRDs. For each dynamic sidebar, it should find the corresponding owner CRD and use it to set the correct labels via m.addDashboardLabels. The owner reference should still only be set if the sidebar's CRD matches the one being reconciled.
| logger := log.FromContext(ctx) | ||
| logger.Info("Deleting orphaned Breadcrumb resource", "name", item.Name) |
There was a problem hiding this comment.
For better performance, the logger should be initialized once before the for loop, rather than on each iteration within the loop. This same improvement can be applied to the FactoryList case below.
| logger := log.FromContext(ctx) | ||
| logger.Info("Deleting orphaned Factory resource", "name", item.Name) |
There was a problem hiding this comment.
For better performance, the logger should be initialized once before the for loop, rather than on each iteration within the loop.
| _, _, kind := pickGVK(crd) | ||
| lowerKind := strings.ToLower(kind) | ||
| expectedID := fmt.Sprintf("stock-project-factory-%s-details", lowerKind) |
There was a problem hiding this comment.
The values kind, lowerKind, and expectedID are constant within the for loop over ids since they only depend on crd. For better performance, they can be calculated once before the loop.
| "pathToNavigate": "/openapi-ui/{clusterName}/~recordValueSecond~/factory/ingress-details/~recordValue~", | ||
| }), | ||
| } | ||
| // links are now handled through CustomFormsPrefills |
There was a problem hiding this comment.
The comment states that links are now handled by CustomFormsPrefills. However, based on other changes in this PR, it appears they are now handled by CustomColumnsOverrides where links are embedded in the column definitions. Please correct the comment for clarity.
| // links are now handled through CustomFormsPrefills | |
| // links are now handled through CustomColumnsOverrides |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/controller/dashboard/static_refactored.go (2)
1109-1116: Customization ID typo prevents Pods table columns from applying."factory-node-details-/v1/pods" should be "factory-node-details-v1.pods" (matches defined override).
- "customizationId": "factory-node-details-/v1/pods", + "customizationId": "factory-node-details-v1.pods",
1053-1063: Add CustomColumnsOverride for factory-kube-service-details-endpointslice
No override exists for this customizationId in CreateAllCustomColumnsOverrides, so the Endpointslice table falls back to defaults. Add:@@ func CreateAllCustomColumnsOverrides() []*dashboardv1alpha1.CustomColumnsOverride { // Factory service details port mapping createCustomColumnsOverride("factory-kube-service-details-port-mapping", []any{ createStringColumn("Name", ".name"), createStringColumn("Port", ".port"), createStringColumn("Protocol", ".protocol"), createStringColumn("Pod port or name", ".targetPort"), }), + // Factory service details EndpointSlice endpoints + createCustomColumnsOverride("factory-kube-service-details-endpointslice", []any{ + createStringColumn("Address", ".addresses[0]"), + createBoolColumn("Ready", ".conditions.ready"), + createStringColumn("Node", ".nodeName"), + createStringColumn("TargetRef", ".targetRef.name"), + }),internal/controller/dashboard/sidebar.go (1)
124-131: Fix sort order: weight should be descending (matches doc comment).Currently smaller weights appear first. The header comment says items should be sorted by Weight (desc), then Label (A→Z).
Apply this diff:
- if categories[cat][i].Weight != categories[cat][j].Weight { - return categories[cat][i].Weight < categories[cat][j].Weight // lower weight first - } + if categories[cat][i].Weight != categories[cat][j].Weight { + return categories[cat][i].Weight > categories[cat][j].Weight // higher weight first + }
🧹 Nitpick comments (2)
internal/controller/dashboard/sidebar.go (2)
220-231: De‑duplicate targetIDs to avoid redundant CreateOrUpdate cycles.The static list includes workloadmonitor-details and the CRD loop may append it again. Duplicates cause needless writes.
Apply this diff:
for i := range all { def := &all[i] if def.Spec.Dashboard == nil { continue } _, _, kind := pickGVK(def) lowerKind := strings.ToLower(kind) detailsID := fmt.Sprintf("stock-project-factory-%s-details", lowerKind) targetIDs = append(targetIDs, detailsID) } + + // De-duplicate IDs to prevent churn + targetIDs = uniqueStrings(targetIDs)Add this helper (outside this hunk):
func uniqueStrings(in []string) []string { set := make(map[string]struct{}, len(in)) out := make([]string, 0, len(in)) for _, s := range in { if _, ok := set[s]; ok { continue } set[s] = struct{}{} out = append(out, s) } return out }
23-31: Docstring drift: update menu rules to match current behavior.Comment still says “Marketplace has two hardcoded entries incl. Tenant Info” and implies weight desc (which the code currently didn’t do until fixed). The actual code hardcodes only Marketplace (Info now under Administration).
Proposed update:
-// - The first section is "Marketplace" with two hardcoded entries: -// - Marketplace (/openapi-ui/{clusterName}/{namespace}/factory/marketplace) -// - Tenant Info (/openapi-ui/{clusterName}/{namespace}/factory/info-details/info) +// - The first section is "Marketplace" with one hardcoded entry: +// - Marketplace (/openapi-ui/{clusterName}/{namespace}/factory/marketplace) +// - "Info" is under the hardcoded "Administration" section: +// (/openapi-ui/{clusterName}/{namespace}/factory/info-details/info) -// - Items within each category: sort by Weight (desc), then Label (A→Z). +// - Items within each category: sort by Weight (desc), then Label (A→Z).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
api/v1alpha1/cozystackresourcedefinitions_types.go(1 hunks)internal/controller/dashboard/breadcrumb.go(1 hunks)internal/controller/dashboard/customformsprefill.go(1 hunks)internal/controller/dashboard/helpers.go(1 hunks)internal/controller/dashboard/manager.go(4 hunks)internal/controller/dashboard/marketplacepanel.go(2 hunks)internal/controller/dashboard/sidebar.go(6 hunks)internal/controller/dashboard/static_helpers.go(2 hunks)internal/controller/dashboard/static_refactored.go(15 hunks)packages/system/cozystack-api/templates/cozystack-resource-definitions/etcd.yaml(1 hunks)packages/system/cozystack-api/templates/cozystack-resource-definitions/info.yaml(1 hunks)packages/system/cozystack-api/templates/cozystack-resource-definitions/ingress.yaml(1 hunks)packages/system/cozystack-api/templates/cozystack-resource-definitions/monitoring.yaml(1 hunks)packages/system/cozystack-api/templates/cozystack-resource-definitions/seaweedfs.yaml(1 hunks)packages/system/cozystack-controller/templates/crds/cozystack.io_cozystackresourcedefinitions.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/controller/dashboard/sidebar.go (1)
internal/controller/dashboard/manager.go (5)
ResourceTypeDynamic(32-32)LabelManagedBy(21-21)ManagedByValue(30-30)LabelResourceType(22-22)ResourceTypeStatic(31-31)
internal/controller/dashboard/static_refactored.go (1)
internal/controller/dashboard/unified_helpers.go (1)
BadgeSizeMedium(95-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (30)
packages/system/cozystack-api/templates/cozystack-resource-definitions/monitoring.yaml (1)
29-29: Module flag aligns with module-aware controllers.Good call surfacing
module: truehere so the new dashboard logic treats monitoring as a tenant module.packages/system/cozystack-api/templates/cozystack-resource-definitions/info.yaml (1)
29-29: Consistent module flag propagation.Marking
infoas a module keeps it in sync with the updated controller expectations.packages/system/cozystack-api/templates/cozystack-resource-definitions/ingress.yaml (1)
29-29: Ingress module flag looks good.This ensures ingress dashboards land under the tenant-module flows introduced elsewhere.
internal/controller/dashboard/breadcrumb.go (1)
37-42: Breadcrumb switch for modules makes sense.Adjusting the root crumb to “Tenant Modules” when the flag is set matches the new navigation model.
internal/controller/dashboard/helpers.go (1)
35-37: Deterministic apps.cozystack.io GVK fixes the API link issue.Hard-coding the application group/version here resolves the previous mismatch the PR description called out.
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
156-158: Module flag addition to the API type is spot on.The new field is documented and optional, matching how the YAML templates consume it.
packages/system/cozystack-controller/templates/crds/cozystack.io_cozystackresourcedefinitions.yaml (1)
87-89: CRD schema update keeps validation in sync.Adding the boolean
moduleproperty (with description) aligns the CRD with the Go type.packages/system/cozystack-api/templates/cozystack-resource-definitions/etcd.yaml (1)
29-29: etcd now participates in the module flow.Setting
module: trueensures etcd shows up under the tenant-module experience along with the other resources.internal/controller/dashboard/static_helpers.go (2)
175-190: LGTM: renamed ingress rules override and undefined-value defaults.Matches the kube-prefixed ingress details. Defaults for Service/Port/Path are sensible.
690-695: Verify need for kube-ingress-details tab mapping
No matches for “ingress-tabs” or “kube-ingress-details” in the codebase; confirm whether any consumer expects this before adding the special-case.internal/controller/dashboard/marketplacepanel.go (2)
41-55: Correctly skipping modules for MarketplacePanel.Deletion-on-module and log message adjustments look good.
71-82: Verify Application CRD group/versioninternal/controller/dashboard/marketplacepanel.go:71-82 – ensure
apiGroup: "apps.cozystack.io"andapiVersion: "v1alpha1"match an existing CustomResourceDefinition for kindApplication. No matching CRD was found in the repo—confirm or correct the group/version.packages/system/cozystack-api/templates/cozystack-resource-definitions/seaweedfs.yaml (1)
29-30: Module flag added — aligns with controller logic.This will ensure MarketplacePanel is skipped and module-aware paths apply.
internal/controller/dashboard/manager.go (3)
15-16: LGTM: add logger import for cleanup logs.
249-306: Expected set logic matches new module semantics.
- Include all dashboard CRDs for most resources; only non-modules for MarketplacePanel — correct.
- Names align with pickGVK/pickPlural usage.
Please confirm ensure* methods produce these exact names to avoid false-positive deletions during CleanupOrphanedResources.
427-441: LGTM: add visibility logs for orphan deletion.Breadcrumb/Factory cleanup now traceable.
internal/controller/dashboard/static_refactored.go (10)
43-58: LGTM: kube-prefixed breadcrumbs for Secret/Service/Ingress.Matches the new factories and routes.
158-164: LGTM: Service port mapping override.Customization ID and columns align with service spec. Works with service details tab.
185-191: LGTM: Ingress rules override.Pairs with kube-ingress details factory and rules table.
318-320: LGTM: tenantnamespace links in Pods view.Uses tenantnamespace factory path; consistent with broader refactor.
339-347: LGTM: kube-secret details and tenantnamespace link.
361-365: LGTM: tenantnamespaces override.Provides cluster-level table for tenantnamespaces.
804-805: LGTM: kube-secret-details factory key.Aligned with getTabsId override and breadcrumb.
1128-1129: LGTM: kube-service-details factory key.Aligned with getTabsId override and breadcrumb.
1130-1274: LGTM: New kube-ingress-details factory.Header/rules/YAML tabs and EnrichedTable customization are coherent.
Optionally ensure a tabs ID mapping exists if any dependent expects "ingress-tabs" (see suggestion in static_helpers.go).
1454-1456: Drop static TableUriMappings; links handled via CustomFormsOverrides
CreateAllTableUriMappings now returns an empty slice. All link mappings are managed by CreateAllCustomFormsOverrides and the dynamic ensureTableUriMapping logic—no callers depend on static TableUriMappings.internal/controller/dashboard/customformsprefill.go (1)
34-45: No duplicate metadata.name risk. buildPrefillValues (helpers.go) only processes spec fields and never emits metadata.name; customformsprefill.go is the sole injector, so the prepend cannot produce duplicates.Likely an incorrect or invalid review comment.
internal/controller/dashboard/sidebar.go (3)
255-282: Dynamic sidebar labeling/ownership for non-matching CRDs — confirm lifecycle implications.When reconciling CRD A, dynamic sidebars for CRD B are created/updated without labels and owner refs. They’ll only get labels/owner set when CRD B reconciles. Ensure no pruning/selection logic relies on labels/ownerRef presence right after creation.
If labels are required immediately (for discovery or GC), consider at least setting LabelManagedBy on all dynamic sidebars regardless of match and deferring only the ownerRef to the matching CRD.
118-122: LGTM: built-in Kubernetes resource sidebars registered.The keys for services, secrets, and ingresses look correct and consistent with plural naming.
113-116: Leave the key as “modules” – the menu’skeyproperty must matchkeysAndTags(both use"modules"), even though the link points to/…/tenantmodules. Changing to"tenantmodules"would break the mapping between tabs and their sidebar tags.
What this PR does
Release note
Summary by CodeRabbit
New Features
Improvements