Repository navigation
feat(dashboard): add restorejob related views - #2387
Andrey Kolkov (androndo) wants to merge 1 commit into
Conversation
Signed-off-by: Andrey Kolkov <[email protected]>
dosubot
Bot
added
size/L
📝 WalkthroughWalkthroughThis change adds comprehensive dashboard UI support for the RestoreJob resource in the backups.cozystack.io API group, including sidebar navigation, custom columns display, form configuration for resource creation/editing, and a detailed view factory with metadata and status rendering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for RestoreJob resources in the dashboard, including sidebar integration, custom list columns, form overrides for creation, and a detailed resource view. Feedback includes addressing hardcoded driver-specific labels in the form, resolving duplicate component IDs in the details factory, and improving the display of optional target application references to avoid broken string concatenations.
| createFormItem("spec.targetApplicationRef.name", "Target Application Name (optional, only for rename)", "text"), | ||
| // Driver-specific options (key-value editor). Known keys for VMInstance: | ||
| // targetNamespace, failIfTargetExists, keepOriginalPVC, keepOriginalIpAndMac. | ||
| createFormItem("spec.options", "Options (VMInstance keys: targetNamespace, failIfTargetExists, keepOriginalPVC, keepOriginalIpAndMac)", "object"), |
There was a problem hiding this comment.
The label for spec.options is very specific to VMInstance. While this might be the primary use case, RestoreJob is a generic resource. Hardcoding driver-specific keys in the label makes the dashboard less generic. Consider a more neutral label or a way to provide driver-specific labels dynamically.
| antdFlex("time-block", 6, []any{ | ||
| map[string]any{ | ||
| "type": "antdText", | ||
| "data": map[string]any{ | ||
| "id": "time-icon", | ||
| "text": "🌐", | ||
| }, | ||
| }, | ||
| map[string]any{ | ||
| "type": "parsedText", | ||
| "data": map[string]any{ | ||
| "formatter": "timestamp", | ||
| "id": "time-value", | ||
| "text": "{reqsJsonPath[0]['.metadata.creationTimestamp']['-']}", | ||
| }, | ||
| }, | ||
| }), | ||
| }), | ||
| }), | ||
| }), | ||
| antdCol("col-right", 12, []any{ | ||
| antdFlexVertical("col-right-stack", 24, []any{ | ||
| antdFlexVertical("status-phase-block", 4, []any{ | ||
| antdText("phase-label", true, "Phase", nil), | ||
| parsedText("phase-value", "{reqsJsonPath[0]['.status.phase']['-']}", nil), | ||
| }), | ||
| antdFlexVertical("spec-backup-ref-block", 4, []any{ | ||
| antdText("backup-ref-label", true, "Backup Ref", nil), | ||
| parsedText("backup-ref-value", "{reqsJsonPath[0]['.spec.backupRef.name']['-']}", nil), | ||
| }), | ||
| antdFlexVertical("spec-target-application-ref-block", 4, []any{ | ||
| antdText("target-application-ref-label", true, "Target Application", nil), | ||
| parsedText("target-application-ref-value", "{reqsJsonPath[0]['.spec.targetApplicationRef.kind']['-']}.{reqsJsonPath[0]['.spec.targetApplicationRef.apiGroup']['-']}/{reqsJsonPath[0]['.spec.targetApplicationRef.name']['-']}", nil), | ||
| }), | ||
| antdFlexVertical("spec-options-target-namespace-block", 4, []any{ | ||
| antdText("options-target-namespace-label", true, "Target Namespace", nil), | ||
| parsedText("options-target-namespace-value", "{reqsJsonPath[0]['.spec.options.targetNamespace']['-']}", nil), | ||
| }), | ||
| antdFlexVertical("spec-options-fail-if-target-exists-block", 4, []any{ | ||
| antdText("options-fail-if-target-exists-label", true, "Fail If Target Exists", nil), | ||
| parsedText("options-fail-if-target-exists-value", "{reqsJsonPath[0]['.spec.options.failIfTargetExists']['-']}", nil), | ||
| }), | ||
| antdFlexVertical("spec-options-keep-original-pvc-block", 4, []any{ | ||
| antdText("options-keep-original-pvc-label", true, "Keep Original PVC", nil), | ||
| parsedText("options-keep-original-pvc-value", "{reqsJsonPath[0]['.spec.options.keepOriginalPVC']['-']}", nil), | ||
| }), | ||
| antdFlexVertical("spec-options-keep-original-ip-mac-block", 4, []any{ | ||
| antdText("options-keep-original-ip-mac-label", true, "Keep Original IP/MAC", nil), | ||
| parsedText("options-keep-original-ip-mac-value", "{reqsJsonPath[0]['.spec.options.keepOriginalIpAndMac']['-']}", nil), | ||
| }), | ||
| antdFlexVertical("status-started-at-block", 4, []any{ | ||
| antdText("started-at-label", true, "Started At", nil), | ||
| antdFlex("time-block", 6, []any{ | ||
| map[string]any{ | ||
| "type": "antdText", | ||
| "data": map[string]any{ | ||
| "id": "time-icon", | ||
| "text": "🌐", | ||
| }, | ||
| }, | ||
| map[string]any{ | ||
| "type": "parsedText", | ||
| "data": map[string]any{ | ||
| "formatter": "timestamp", | ||
| "id": "time-value", | ||
| "text": "{reqsJsonPath[0]['.status.startedAt']['-']}", | ||
| }, | ||
| }, | ||
| }), | ||
| }), | ||
| antdFlexVertical("status-completed-at-block", 4, []any{ | ||
| antdText("completed-at-label", true, "Completed At", nil), | ||
| antdFlex("time-block", 6, []any{ | ||
| map[string]any{ | ||
| "type": "antdText", | ||
| "data": map[string]any{ | ||
| "id": "time-icon", | ||
| "text": "🌐", | ||
| }, | ||
| }, | ||
| map[string]any{ | ||
| "type": "parsedText", | ||
| "data": map[string]any{ | ||
| "formatter": "timestamp", | ||
| "id": "time-value", | ||
| "text": "{reqsJsonPath[0]['.status.completedAt']['-']}", | ||
| }, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
The IDs time-block, time-icon, and time-value are duplicated within the restorejob-details factory (used for 'Created', 'Started At', and 'Completed At'). Duplicate IDs within the same factory can lead to issues in the dashboard frontend when targeting specific elements. Please use unique IDs for each block, such as created-time-block, started-time-block, and completed-time-block.
| }), | ||
| antdFlexVertical("spec-target-application-ref-block", 4, []any{ | ||
| antdText("target-application-ref-label", true, "Target Application", nil), | ||
| parsedText("target-application-ref-value", "{reqsJsonPath[0]['.spec.targetApplicationRef.kind']['-']}.{reqsJsonPath[0]['.spec.targetApplicationRef.apiGroup']['-']}/{reqsJsonPath[0]['.spec.targetApplicationRef.name']['-']}", nil), |
There was a problem hiding this comment.
The targetApplicationRef field is optional. If it is omitted, the current JSONPath concatenation will render as .-/-. Since this is a common case (restoring to the original application), it would be better to handle this gracefully in the UI, perhaps by showing a descriptive text like 'Original application' when the reference is missing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/dashboard/sidebar.go`:
- Line 269: The sidebar with ID 'stock-project-factory-restorejob-details' is
being created without the labels/owner refs applied by upsertMultipleSidebars(),
so make it follow the static-label branch or explicitly add the same
labels/owner refs used for other 'stock-project-factory-*-details' entries;
update the logic in upsertMultipleSidebars() (or the place where the list of
sidebar IDs is defined in internal/controller/dashboard/sidebar.go) to include
'stock-project-factory-restorejob-details' in the static branch or add the same
label set and owner reference assignment so the RestoreJob details sidebar is
managed consistently.
In `@internal/controller/dashboard/static_refactored.go`:
- Around line 554-556: Update the user-visible labels for the
TargetApplicationRef fields so they don't restrict the feature to renames: in
the createFormItem calls for "spec.targetApplicationRef.apiGroup",
"spec.targetApplicationRef.kind", and "spec.targetApplicationRef.name" replace
the label text that currently reads "Target Application ... (optional, only for
rename)" with a neutral description such as "Target Application API Group
(optional, used to restore into a different application)", "Target Application
Kind (optional, used to restore into a different application)", and "Target
Application Name (optional, used to restore into a different application)"
respectively so the UI accurately conveys that restoring into a different
application (not just rename) is supported.
- Around line 2054-2056: The UI currently renders noisy "-.-/-" when
.spec.targetApplicationRef is missing; update the rendering around
antdFlexVertical("spec-target-application-ref-block", ...) so it either
conditionally hides the whole block when
reqsJsonPath[0]['.spec.targetApplicationRef'] is undefined/null, or replace
parsedText("target-application-ref-value", ...) with logic that detects missing
parts and returns a single fallback string (e.g., "Same as backup" or "Not
set"). Locate the block using antdFlexVertical and parsedText and implement the
conditional check upstream of parsedText or within its value resolution so
absent targetApplicationRef yields the chosen fallback instead of "-.-/-".
🪄 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
Run ID: 58db88a6-c588-454e-815d-b536a877c316
📒 Files selected for processing (2)
internal/controller/dashboard/sidebar.gointernal/controller/dashboard/static_refactored.go
| "stock-project-factory-plan-details", | ||
| "stock-project-factory-backupjob-details", | ||
| "stock-project-factory-backup-details", | ||
| "stock-project-factory-restorejob-details", |
There was a problem hiding this comment.
Mark this sidebar as static, or label it explicitly.
Line 269 adds stock-project-factory-restorejob-details, but upsertMultipleSidebars() only assigns labels/owner refs for stock-project-factory-*-details IDs when they match the current ApplicationDefinition (Lines 321-345). RestoreJob never hits that match, so this Sidebar gets created unmanaged. Please route it through the static-label branch or set the labels explicitly for this backup-specific details sidebar.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/dashboard/sidebar.go` at line 269, The sidebar with ID
'stock-project-factory-restorejob-details' is being created without the
labels/owner refs applied by upsertMultipleSidebars(), so make it follow the
static-label branch or explicitly add the same labels/owner refs used for other
'stock-project-factory-*-details' entries; update the logic in
upsertMultipleSidebars() (or the place where the list of sidebar IDs is defined
in internal/controller/dashboard/sidebar.go) to include
'stock-project-factory-restorejob-details' in the static branch or add the same
label set and owner reference assignment so the RestoreJob details sidebar is
managed consistently.
| createFormItem("spec.targetApplicationRef.apiGroup", "Target Application API Group (optional, only for rename)", "text"), | ||
| createFormItem("spec.targetApplicationRef.kind", "Target Application Kind (optional, only for rename)", "text"), | ||
| createFormItem("spec.targetApplicationRef.name", "Target Application Name (optional, only for rename)", "text"), |
There was a problem hiding this comment.
Don’t describe targetApplicationRef as rename-only.
These labels are user-visible, and they narrow the feature too much. RestoreJobSpec.TargetApplicationRef is the target application in general, so users can also restore into a different application, not just rename. The current wording is likely to hide that case from the UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/dashboard/static_refactored.go` around lines 554 - 556,
Update the user-visible labels for the TargetApplicationRef fields so they don't
restrict the feature to renames: in the createFormItem calls for
"spec.targetApplicationRef.apiGroup", "spec.targetApplicationRef.kind", and
"spec.targetApplicationRef.name" replace the label text that currently reads
"Target Application ... (optional, only for rename)" with a neutral description
such as "Target Application API Group (optional, used to restore into a
different application)", "Target Application Kind (optional, used to restore
into a different application)", and "Target Application Name (optional, used to
restore into a different application)" respectively so the UI accurately conveys
that restoring into a different application (not just rename) is supported.
| antdFlexVertical("spec-target-application-ref-block", 4, []any{ | ||
| antdText("target-application-ref-label", true, "Target Application", nil), | ||
| parsedText("target-application-ref-value", "{reqsJsonPath[0]['.spec.targetApplicationRef.kind']['-']}.{reqsJsonPath[0]['.spec.targetApplicationRef.apiGroup']['-']}/{reqsJsonPath[0]['.spec.targetApplicationRef.name']['-']}", nil), |
There was a problem hiding this comment.
Handle an omitted targetApplicationRef as one fallback.
targetApplicationRef is optional, so the common case here is "not set". With the current composition, Lines 2054-2056 render -.-/-, which is noisy and misleading on the default path. Please either hide this block when absent or show a single fallback like Same as backup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/dashboard/static_refactored.go` around lines 2054 - 2056,
The UI currently renders noisy "-.-/-" when .spec.targetApplicationRef is
missing; update the rendering around
antdFlexVertical("spec-target-application-ref-block", ...) so it either
conditionally hides the whole block when
reqsJsonPath[0]['.spec.targetApplicationRef'] is undefined/null, or replace
parsedText("target-application-ref-value", ...) with logic that detects missing
parts and returns a single fallback string (e.g., "Same as backup" or "Not
set"). Locate the block using antdFlexVertical and parsedText and implement the
conditional check upstream of parsedText or within its value resolution so
absent targetApplicationRef yields the chosen fallback instead of "-.-/-".
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Blocker: please address the open bot review threads before merge — the findings from coderabbit and gemini are substantive, not noise:
spec.targetApplicationReflabels shouldn't describe the field as rename-only (coderabbit, gemini on lines 554-556) — it also covers cross-application restore.- Omitted
spec.targetApplicationRefrenders as-.-/-on the details page (coderabbit, gemini on line 2056) — this is the common default case for same-application restores and needs a clean fallback. spec.optionsform label hardcodes VMInstance keys (gemini on line 559).- Duplicate component IDs (
time-block,time-icon,time-value) inside therestorejob-detailsfactory (gemini on line 2112). stock-project-factory-restorejob-detailssidebar is created unlabelled byupsertMultipleSidebars(coderabbit on sidebar.go:269) — pre-existing pattern for backup/backupjob/plan, but worth resolving consistently here.
One additional observation below.
| }), | ||
| antdFlexVertical("spec-options-keep-original-ip-mac-block", 4, []any{ | ||
| antdText("options-keep-original-ip-mac-label", true, "Keep Original IP/MAC", nil), | ||
| parsedText("options-keep-original-ip-mac-value", "{reqsJsonPath[0]['.spec.options.keepOriginalIpAndMac']['-']}", nil), |
There was a problem hiding this comment.
Observation (non-blocking, related to gemini's comment on line 559): the details view hardcodes four VMInstance-driver-specific fields (targetNamespace, failIfTargetExists, keepOriginalPVC, keepOriginalIpAndMac) at spec.options.*. RestoreJob is a generic resource — for restores created by a non-VMInstance driver all four rows will render "-" unconditionally.
Worth considering rendering spec.options generically (e.g. key-value over the RawExtension payload) once there's a second driver, or gating this block on spec.targetApplicationRef.kind == VMInstance.
## What this PR does - Introduce list, details, create, and sidebar views for `RestoreJob` in the dashboard controller. - Auto-generate stable component IDs for sidebar entries and detail sub-blocks so upserts remain consistent across reconciles. - Render a single "Same as backup" fallback for an omitted `spec.targetApplicationRef` on the details view instead of concatenating three missing-path fallbacks. - Mark non-CRD-backed `stock-project-factory-*-details` sidebars (`kube-*`, `plan`, `backupjob`, `backup`, `restorejob`) as static so they pick up consistent managed-by labels. ### Release note ```release-note [dashboard] Add RestoreJob views (list, details, create, sidebar link) to the dashboard controller. ``` ## Test plan - [x] Deploy controller to dev stand and open the restore pages in the dashboard - [x] Create a RestoreJob through the UI against an existing Backup; reconcile succeeds and the details view renders - [ ] CI green ## Previous PR #2387 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Introduced comprehensive RestoreJobs dashboard interface enabling users to view and manage backup restoration operations. * New RestoreJobs section added to the Backups menu with dedicated details page and navigation. * Dashboard displays restore job status, phase, backup references, target applications, and operation timestamps for easy monitoring. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
merged in #2437 |
Pull request was closed
3 participants
What this PR does
Related #1973
Release note
Summary by CodeRabbit