feat(RFE-9399): Add DR CLI lifecycle commands, backup verification, and Agent namespace auto-detection#8685
Open
dpateriya wants to merge 1 commit into
Open
Conversation
…nd Agent namespace auto-detection Add get/destroy lifecycle commands, backup integrity verification, and Agent platform namespace auto-detection to the HyperShift DR CLI. Get commands (hypershift get oadp-backups/restores/schedules): - List Velero resources with table/json/yaml output - Filter by --hc-name and --hc-namespace - Sort by creation timestamp (newest first) Destroy commands (hypershift destroy oadp-backup/restore/schedule): - Delete specified Velero resource by name - Validates existence before deletion Backup verification (hypershift verify oadp-backup): - Checks phase, expiration, items count, BSL availability - Also available as --verify flag on oadp-restore to block restore creation when backup integrity checks fail Agent namespace auto-detection: - Reads spec.platform.agent.agentNamespace from HostedCluster - Automatically includes it in backup/restore/schedule namespaces - Deduplicates if already specified via --include-additional-namespaces Signed-off-by: Divyam pateriya <dpateriy@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Contributor
📝 WalkthroughWalkthroughThis PR adds new 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dpateriya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
area/cli
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8685 +/- ##
==========================================
- Coverage 41.43% 41.33% -0.10%
==========================================
Files 756 761 +5
Lines 93647 94124 +477
==========================================
+ Hits 38802 38910 +108
- Misses 52124 52483 +359
- Partials 2721 2731 +10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
cmd/oadp/verify_backup.go (1)
106-106: 💤 Low valueUnused logger parameter.
The
logr.Loggerparameter is unused (named_). Consider removing it or using it for internal diagnostic logging within the verification checks.♻️ Optional: Remove unused parameter
-func VerifyBackup(ctx context.Context, c client.Client, backupName, oadpNamespace string, _ logr.Logger) ([]VerifyResult, error) { +func VerifyBackup(ctx context.Context, c client.Client, backupName, oadpNamespace string) ([]VerifyResult, error) {Then update the callers in verify_backup.go line 81 and restore.go line 205 to remove the logger argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/oadp/verify_backup.go` at line 106, The VerifyBackup function currently declares an unused logr.Logger parameter (named `_`). Fix this by either removing the logger parameter from VerifyBackup's signature and deleting the logger argument at all call sites that pass it, or by renaming `_` to `logger` and using it for diagnostic logs inside VerifyBackup (e.g., logging start/step/results); ensure you update every invocation of VerifyBackup to match the new signature so callers no longer pass an unused logger or instead pass and the function uses it.cmd/oadp/lifecycle_test.go (4)
141-141: 💤 Low valueRemove or implement verification for
expectLogMessagefield.The
expectLogMessagefield is declared and set in test cases but never used in the test logic. Either remove it or implement log output verification if you intend to test logging behavior.From
common.go:283(context snippet 2),autoIncludeAgentNamespacelogs:"Auto-detected agent namespace, including in operation". If log verification is desired, consider using a test logger sink to capture and verify log output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/oadp/lifecycle_test.go` at line 141, The test struct field expectLogMessage is declared and unused; either remove it from the test cases in lifecycle_test.go or implement log verification: when testing behavior that triggers autoIncludeAgentNamespace (which logs "Auto-detected agent namespace, including in operation"), wire a test logger sink or capture output in the test, run the code path that calls autoIncludeAgentNamespace, and assert the captured logs contain the expected message; update or remove references to expectLogMessage accordingly so the field is not left unused.
283-286: ⚡ Quick winVerify that the backup was actually deleted.
The test only checks that
runDestroyreturns no error but doesn't verify the side effect—that the backup was actually removed from the fake client. Add a verification step:✅ Suggested verification
err := opts.runDestroy(ctx, "Backup") if err != nil { t.Fatalf("expected no error, got: %v", err) } + + // Verify the backup was actually deleted + deletedBackup := &unstructured.Unstructured{} + deletedBackup.SetAPIVersion("velero.io/v1") + deletedBackup.SetKind("Backup") + key := client.ObjectKey{Name: "to-delete", Namespace: "openshift-adp"} + err = fakeClient.Get(ctx, key, deletedBackup) + if err == nil { + t.Error("expected backup to be deleted, but it still exists") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/oadp/lifecycle_test.go` around lines 283 - 286, After calling opts.runDestroy(ctx, "Backup"), add an assertion that the fake backup client no longer contains the Backup resource: query the same fake client used in the test (e.g., the test's fake backup client referenced by opts or the test setup) with a Get/List for the "Backup" name/label and fail the test if the resource is still present (expect NotFound or zero results); this ensures runDestroy not only returns nil but actually removed the backup.
196-199: 💤 Low valueConsider order-independent slice comparison.
The current index-based comparison (lines 196-199) assumes both slices have the same order. While this works for the current implementation, a set-based comparison would be more robust if the append order ever changes:
♻️ Optional: More robust comparison
- for i, ns := range tt.expectedNS { - if i >= len(opts.IncludeNamespaces) || opts.IncludeNamespaces[i] != ns { - t.Errorf("expected namespace[%d]=%q, got %v", i, ns, opts.IncludeNamespaces) - } - } + expectedSet := make(map[string]bool) + for _, ns := range tt.expectedNS { + expectedSet[ns] = true + } + for _, ns := range opts.IncludeNamespaces { + if !expectedSet[ns] { + t.Errorf("unexpected namespace %q in result", ns) + } + delete(expectedSet, ns) + } + for ns := range expectedSet { + t.Errorf("expected namespace %q not found in result", ns) + }Or with gomega (per earlier suggestion):
g.Expect(opts.IncludeNamespaces).To(ConsistOf(tt.expectedNS)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/oadp/lifecycle_test.go` around lines 196 - 199, The test currently compares opts.IncludeNamespaces to tt.expectedNS with an index-based loop which assumes identical order; change this to an order-independent comparison by first verifying lengths match and then either (a) sort both slices and compare equality, or (b) build a set/map from one slice and assert every element of the other is present (and no extras). Locate the check using opts.IncludeNamespaces and tt.expectedNS in the test and replace the index loop with one of these set-based or sorted-comparison approaches (alternatively, if using Gomega, replace the assertion with g.Expect(opts.IncludeNamespaces).To(ConsistOf(tt.expectedNS))).
25-48: ⚡ Quick winAdd test coverage for BackupStorageLocation checks.
The test case at Line 36 sets
spec.storageLocation: "default", but noBackupStorageLocationobject is added to the fake client. According toverify_backup.go:159-175(context snippet 1),VerifyBackupattempts to fetch and validate the BSL's phase. Without a BSL object in the fake client, this check isn't exercised.Add a test case that includes a BSL object to verify the
storage_locationcheck works correctly:🧪 Suggested test case with BSL
{ name: "When backup references available BSL it should pass storage location check", backup: map[string]interface{}{ "apiVersion": "velero.io/v1", "kind": "Backup", "metadata": map[string]interface{}{ "name": "test-backup-bsl", "namespace": "openshift-adp", "creationTimestamp": "2026-06-01T10:00:00Z", }, "spec": map[string]interface{}{ "storageLocation": "default", }, "status": map[string]interface{}{ "phase": "Completed", "progress": map[string]interface{}{ "itemsBackedUp": int64(10), }, }, }, bsl: &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "velero.io/v1", "kind": "BackupStorageLocation", "metadata": map[string]interface{}{ "name": "default", "namespace": "openshift-adp", }, "status": map[string]interface{}{ "phase": "Available", }, }, }, expectError: false, },Update the test loop to add the BSL to the fake client when
tt.bsl != nil.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/oadp/lifecycle_test.go` around lines 25 - 48, The test currently sets spec.storageLocation but never adds a BackupStorageLocation object so VerifyBackup's storage_location check isn't exercised; add a new subtest (e.g., name "When backup references available BSL it should pass storage location check") that includes tt.bsl as an *unstructured.Unstructured BackupStorageLocation with metadata.name "default", metadata.namespace "openshift-adp" and status.phase "Available", and update the test setup loop to inject tt.bsl into the fake client when tt.bsl != nil so VerifyBackup (the function performing the BSL fetch/validation) exercises the BSL phase check for storageLocation "default".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/oadp/lifecycle_test.go`:
- Around line 18-308: Tests use raw testing assertions
(t.Errorf/t.Fatalf/t.Error/t.Fatal); update all five tests (TestVerifyBackup,
TestAutoIncludeAgentNamespace, TestGetOutputTable, TestDestroyOptions,
TestDestroyOptionsNotFound) to use Gomega matchers. For each test create g :=
NewWithT(t) and replace error checks with g.Expect(err).To(HaveOccurred()) or
g.Expect(err).NotTo(HaveOccurred()), replace equality/length/contain checks with
g.Expect(...).To(Equal(...)), g.Expect(...).To(HaveLen(...)),
g.Expect(stringOutput).To(ContainSubstring("...")), and assert boolean
conditions with g.Expect(...).To(BeTrue()/BeFalse()); also replace manual
map/slice existence checks in TestVerifyBackup/TestAutoIncludeAgentNamespace
with appropriate Expect assertions. Add the gomega import (NewWithT, matchers)
and remove t.* assertion calls.
In `@cmd/oadp/restore.go`:
- Around line 202-222: Validate and handle the case where verification was
requested but no explicit backup name was provided: inside the RunE entry point,
add a check for o.VerifyBackup combined with a schedule-based restore (i.e.,
when o.BackupName is empty or a boolean/field representing --from-schedule is
set) and either return a user-facing error (making --verify and --from-schedule
mutually exclusive) or emit a clear warning before proceeding; update the logic
that currently checks o.VerifyBackup && o.BackupName != "" in the restore flow
(the block using VerifyBackup(ctx, o.Client, o.BackupName, o.OADPNamespace,
o.Log)) so it cannot be silently skipped when the user requested verification
(reference o.VerifyBackup, o.BackupName, RunE and the VerifyBackup call).
---
Nitpick comments:
In `@cmd/oadp/lifecycle_test.go`:
- Line 141: The test struct field expectLogMessage is declared and unused;
either remove it from the test cases in lifecycle_test.go or implement log
verification: when testing behavior that triggers autoIncludeAgentNamespace
(which logs "Auto-detected agent namespace, including in operation"), wire a
test logger sink or capture output in the test, run the code path that calls
autoIncludeAgentNamespace, and assert the captured logs contain the expected
message; update or remove references to expectLogMessage accordingly so the
field is not left unused.
- Around line 283-286: After calling opts.runDestroy(ctx, "Backup"), add an
assertion that the fake backup client no longer contains the Backup resource:
query the same fake client used in the test (e.g., the test's fake backup client
referenced by opts or the test setup) with a Get/List for the "Backup"
name/label and fail the test if the resource is still present (expect NotFound
or zero results); this ensures runDestroy not only returns nil but actually
removed the backup.
- Around line 196-199: The test currently compares opts.IncludeNamespaces to
tt.expectedNS with an index-based loop which assumes identical order; change
this to an order-independent comparison by first verifying lengths match and
then either (a) sort both slices and compare equality, or (b) build a set/map
from one slice and assert every element of the other is present (and no extras).
Locate the check using opts.IncludeNamespaces and tt.expectedNS in the test and
replace the index loop with one of these set-based or sorted-comparison
approaches (alternatively, if using Gomega, replace the assertion with
g.Expect(opts.IncludeNamespaces).To(ConsistOf(tt.expectedNS))).
- Around line 25-48: The test currently sets spec.storageLocation but never adds
a BackupStorageLocation object so VerifyBackup's storage_location check isn't
exercised; add a new subtest (e.g., name "When backup references available BSL
it should pass storage location check") that includes tt.bsl as an
*unstructured.Unstructured BackupStorageLocation with metadata.name "default",
metadata.namespace "openshift-adp" and status.phase "Available", and update the
test setup loop to inject tt.bsl into the fake client when tt.bsl != nil so
VerifyBackup (the function performing the BSL fetch/validation) exercises the
BSL phase check for storageLocation "default".
In `@cmd/oadp/verify_backup.go`:
- Line 106: The VerifyBackup function currently declares an unused logr.Logger
parameter (named `_`). Fix this by either removing the logger parameter from
VerifyBackup's signature and deleting the logger argument at all call sites that
pass it, or by renaming `_` to `logger` and using it for diagnostic logs inside
VerifyBackup (e.g., logging start/step/results); ensure you update every
invocation of VerifyBackup to match the new signature so callers no longer pass
an unused logger or instead pass and the function uses it.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9e2360ff-1aa5-4fc8-81f3-6d9704d38cec
📒 Files selected for processing (14)
cmd/destroy/destroy.gocmd/get/get.gocmd/oadp/backup.gocmd/oadp/common.gocmd/oadp/destroy.gocmd/oadp/get.gocmd/oadp/lifecycle_test.gocmd/oadp/restore.gocmd/oadp/schedule.gocmd/oadp/types.gocmd/oadp/verify_backup.gocmd/verify/verify.gomain.gosupport/oadp/validate.go
Comment on lines
+18
to
+308
| func TestVerifyBackup(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| backup map[string]interface{} | ||
| expectError bool | ||
| failChecks []string | ||
| }{ | ||
| { | ||
| name: "When backup is completed with valid items it should pass all checks", | ||
| backup: map[string]interface{}{ | ||
| "apiVersion": "velero.io/v1", | ||
| "kind": "Backup", | ||
| "metadata": map[string]interface{}{ | ||
| "name": "test-backup", | ||
| "namespace": "openshift-adp", | ||
| "creationTimestamp": "2026-06-01T10:00:00Z", | ||
| }, | ||
| "spec": map[string]interface{}{ | ||
| "storageLocation": "default", | ||
| }, | ||
| "status": map[string]interface{}{ | ||
| "phase": "Completed", | ||
| "expiration": time.Now().Add(24 * time.Hour).Format(time.RFC3339), | ||
| "progress": map[string]interface{}{ | ||
| "itemsBackedUp": int64(142), | ||
| "totalItems": int64(142), | ||
| }, | ||
| }, | ||
| }, | ||
| expectError: false, | ||
| }, | ||
| { | ||
| name: "When backup has failed phase it should fail the phase check", | ||
| backup: map[string]interface{}{ | ||
| "apiVersion": "velero.io/v1", | ||
| "kind": "Backup", | ||
| "metadata": map[string]interface{}{ | ||
| "name": "failed-backup", | ||
| "namespace": "openshift-adp", | ||
| "creationTimestamp": "2026-06-01T10:00:00Z", | ||
| }, | ||
| "spec": map[string]interface{}{}, | ||
| "status": map[string]interface{}{ | ||
| "phase": "Failed", | ||
| "progress": map[string]interface{}{ | ||
| "itemsBackedUp": int64(0), | ||
| "totalItems": int64(100), | ||
| }, | ||
| }, | ||
| }, | ||
| expectError: false, | ||
| failChecks: []string{"phase", "items"}, | ||
| }, | ||
| { | ||
| name: "When backup has expired it should fail the expiration check", | ||
| backup: map[string]interface{}{ | ||
| "apiVersion": "velero.io/v1", | ||
| "kind": "Backup", | ||
| "metadata": map[string]interface{}{ | ||
| "name": "expired-backup", | ||
| "namespace": "openshift-adp", | ||
| "creationTimestamp": "2026-06-01T10:00:00Z", | ||
| }, | ||
| "spec": map[string]interface{}{}, | ||
| "status": map[string]interface{}{ | ||
| "phase": "Completed", | ||
| "expiration": time.Now().Add(-2 * time.Hour).Format(time.RFC3339), | ||
| "progress": map[string]interface{}{ | ||
| "itemsBackedUp": int64(142), | ||
| "totalItems": int64(142), | ||
| }, | ||
| }, | ||
| }, | ||
| expectError: false, | ||
| failChecks: []string{"expiration"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| ctx := context.Background() | ||
| scheme := runtime.NewScheme() | ||
|
|
||
| backupObj := &unstructured.Unstructured{Object: tt.backup} | ||
| fakeClient := fake.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| WithObjects(backupObj). | ||
| Build() | ||
|
|
||
| results, err := VerifyBackup(ctx, fakeClient, backupObj.GetName(), "openshift-adp", log.Log) | ||
| if tt.expectError { | ||
| if err == nil { | ||
| t.Errorf("expected error but got none") | ||
| } | ||
| return | ||
| } | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| failedChecks := make(map[string]bool) | ||
| for _, r := range results { | ||
| if !r.Passed { | ||
| failedChecks[r.Check] = true | ||
| } | ||
| } | ||
|
|
||
| for _, expected := range tt.failChecks { | ||
| if !failedChecks[expected] { | ||
| t.Errorf("expected check '%s' to fail, but it passed", expected) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestAutoIncludeAgentNamespace(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| platform string | ||
| agentNamespace string | ||
| existingNS []string | ||
| expectedNS []string | ||
| expectLogMessage bool | ||
| }{ | ||
| { | ||
| name: "When platform is AGENT with namespace it should auto-include", | ||
| platform: "AGENT", | ||
| agentNamespace: "my-agent-ns", | ||
| existingNS: nil, | ||
| expectedNS: []string{"my-agent-ns"}, | ||
| expectLogMessage: true, | ||
| }, | ||
| { | ||
| name: "When platform is AGENT and namespace already included it should not duplicate", | ||
| platform: "AGENT", | ||
| agentNamespace: "my-agent-ns", | ||
| existingNS: []string{"my-agent-ns"}, | ||
| expectedNS: []string{"my-agent-ns"}, | ||
| expectLogMessage: false, | ||
| }, | ||
| { | ||
| name: "When platform is AWS it should not include agent namespace", | ||
| platform: "AWS", | ||
| agentNamespace: "", | ||
| existingNS: nil, | ||
| expectedNS: nil, | ||
| expectLogMessage: false, | ||
| }, | ||
| { | ||
| name: "When platform is AGENT but namespace is empty it should skip", | ||
| platform: "AGENT", | ||
| agentNamespace: "", | ||
| existingNS: nil, | ||
| expectedNS: nil, | ||
| expectLogMessage: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| opts := &CreateOptions{ | ||
| Log: log.Log, | ||
| IncludeNamespaces: tt.existingNS, | ||
| } | ||
| info := &supportoadp.PlatformInfo{ | ||
| Type: tt.platform, | ||
| AgentNamespace: tt.agentNamespace, | ||
| } | ||
|
|
||
| autoIncludeAgentNamespace(opts, info) | ||
|
|
||
| if len(tt.expectedNS) == 0 && len(opts.IncludeNamespaces) == 0 { | ||
| return | ||
| } | ||
| if len(opts.IncludeNamespaces) != len(tt.expectedNS) { | ||
| t.Errorf("expected namespaces %v, got %v", tt.expectedNS, opts.IncludeNamespaces) | ||
| } | ||
| for i, ns := range tt.expectedNS { | ||
| if i >= len(opts.IncludeNamespaces) || opts.IncludeNamespaces[i] != ns { | ||
| t.Errorf("expected namespace[%d]=%q, got %v", i, ns, opts.IncludeNamespaces) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGetOutputTable(t *testing.T) { | ||
| items := []unstructured.Unstructured{ | ||
| { | ||
| Object: map[string]interface{}{ | ||
| "apiVersion": "velero.io/v1", | ||
| "kind": "Backup", | ||
| "metadata": map[string]interface{}{ | ||
| "name": "test-backup-1", | ||
| "namespace": "openshift-adp", | ||
| "creationTimestamp": time.Now().Add(-2 * time.Hour).Format(time.RFC3339), | ||
| }, | ||
| "status": map[string]interface{}{ | ||
| "phase": "Completed", | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Object: map[string]interface{}{ | ||
| "apiVersion": "velero.io/v1", | ||
| "kind": "Backup", | ||
| "metadata": map[string]interface{}{ | ||
| "name": "test-backup-2", | ||
| "namespace": "openshift-adp", | ||
| "creationTimestamp": time.Now().Add(-48 * time.Hour).Format(time.RFC3339), | ||
| }, | ||
| "status": map[string]interface{}{ | ||
| "phase": "Failed", | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| var buf bytes.Buffer | ||
| err := outputTable(&buf, items, "Backup") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| output := buf.String() | ||
| if !bytes.Contains([]byte(output), []byte("NAME")) { | ||
| t.Error("table output should contain NAME header") | ||
| } | ||
| if !bytes.Contains([]byte(output), []byte("test-backup-1")) { | ||
| t.Error("table output should contain test-backup-1") | ||
| } | ||
| if !bytes.Contains([]byte(output), []byte("Completed")) { | ||
| t.Error("table output should contain Completed status") | ||
| } | ||
| } | ||
|
|
||
| func TestDestroyOptions(t *testing.T) { | ||
| ctx := context.Background() | ||
| scheme := runtime.NewScheme() | ||
|
|
||
| backup := &unstructured.Unstructured{ | ||
| Object: map[string]interface{}{ | ||
| "apiVersion": "velero.io/v1", | ||
| "kind": "Backup", | ||
| "metadata": map[string]interface{}{ | ||
| "name": "to-delete", | ||
| "namespace": "openshift-adp", | ||
| }, | ||
| "spec": map[string]interface{}{}, | ||
| }, | ||
| } | ||
|
|
||
| fakeClient := fake.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| WithObjects(backup). | ||
| Build() | ||
|
|
||
| opts := &DestroyOptions{ | ||
| Name: "to-delete", | ||
| OADPNamespace: "openshift-adp", | ||
| Log: log.Log, | ||
| Client: fakeClient, | ||
| } | ||
|
|
||
| err := opts.runDestroy(ctx, "Backup") | ||
| if err != nil { | ||
| t.Fatalf("expected no error, got: %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestDestroyOptionsNotFound(t *testing.T) { | ||
| ctx := context.Background() | ||
| scheme := runtime.NewScheme() | ||
|
|
||
| fakeClient := fake.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| Build() | ||
|
|
||
| opts := &DestroyOptions{ | ||
| Name: "nonexistent", | ||
| OADPNamespace: "openshift-adp", | ||
| Log: log.Log, | ||
| Client: fakeClient, | ||
| } | ||
|
|
||
| err := opts.runDestroy(ctx, "Backup") | ||
| if err == nil { | ||
| t.Fatal("expected error for nonexistent backup, got none") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use gomega for test assertions instead of standard testing conditionals.
This file uses standard testing package assertions (t.Errorf, t.Fatalf, t.Error, t.Fatal) throughout all five test functions. As per coding guidelines, prefer gomega for unit test assertions to get better failure messages and more expressive matchers.
Example conversion for TestVerifyBackup:
+import (
+ . "github.com/onsi/gomega"
+)
+
func TestVerifyBackup(t *testing.T) {
+ g := NewWithT(t)
tests := []struct {
...
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
...
results, err := VerifyBackup(ctx, fakeClient, backupObj.GetName(), "openshift-adp", log.Log)
if tt.expectError {
- if err == nil {
- t.Errorf("expected error but got none")
- }
+ g.Expect(err).To(HaveOccurred(), "expected error but got none")
return
}
- if err != nil {
- t.Fatalf("unexpected error: %v", err)
- }
+ g.Expect(err).NotTo(HaveOccurred())
failedChecks := make(map[string]bool)
for _, r := range results {
if !r.Passed {
failedChecks[r.Check] = true
}
}
for _, expected := range tt.failChecks {
- if !failedChecks[expected] {
- t.Errorf("expected check '%s' to fail, but it passed", expected)
- }
+ g.Expect(failedChecks).To(HaveKey(expected), "expected check '%s' to fail", expected)
}
})
}
}Apply similar conversions to the other test functions.
As per coding guidelines: "Prefer gomega for unit test assertions" (applies to **/*_test.go).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestVerifyBackup(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| backup map[string]interface{} | |
| expectError bool | |
| failChecks []string | |
| }{ | |
| { | |
| name: "When backup is completed with valid items it should pass all checks", | |
| backup: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "test-backup", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": "2026-06-01T10:00:00Z", | |
| }, | |
| "spec": map[string]interface{}{ | |
| "storageLocation": "default", | |
| }, | |
| "status": map[string]interface{}{ | |
| "phase": "Completed", | |
| "expiration": time.Now().Add(24 * time.Hour).Format(time.RFC3339), | |
| "progress": map[string]interface{}{ | |
| "itemsBackedUp": int64(142), | |
| "totalItems": int64(142), | |
| }, | |
| }, | |
| }, | |
| expectError: false, | |
| }, | |
| { | |
| name: "When backup has failed phase it should fail the phase check", | |
| backup: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "failed-backup", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": "2026-06-01T10:00:00Z", | |
| }, | |
| "spec": map[string]interface{}{}, | |
| "status": map[string]interface{}{ | |
| "phase": "Failed", | |
| "progress": map[string]interface{}{ | |
| "itemsBackedUp": int64(0), | |
| "totalItems": int64(100), | |
| }, | |
| }, | |
| }, | |
| expectError: false, | |
| failChecks: []string{"phase", "items"}, | |
| }, | |
| { | |
| name: "When backup has expired it should fail the expiration check", | |
| backup: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "expired-backup", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": "2026-06-01T10:00:00Z", | |
| }, | |
| "spec": map[string]interface{}{}, | |
| "status": map[string]interface{}{ | |
| "phase": "Completed", | |
| "expiration": time.Now().Add(-2 * time.Hour).Format(time.RFC3339), | |
| "progress": map[string]interface{}{ | |
| "itemsBackedUp": int64(142), | |
| "totalItems": int64(142), | |
| }, | |
| }, | |
| }, | |
| expectError: false, | |
| failChecks: []string{"expiration"}, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| ctx := context.Background() | |
| scheme := runtime.NewScheme() | |
| backupObj := &unstructured.Unstructured{Object: tt.backup} | |
| fakeClient := fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| WithObjects(backupObj). | |
| Build() | |
| results, err := VerifyBackup(ctx, fakeClient, backupObj.GetName(), "openshift-adp", log.Log) | |
| if tt.expectError { | |
| if err == nil { | |
| t.Errorf("expected error but got none") | |
| } | |
| return | |
| } | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| failedChecks := make(map[string]bool) | |
| for _, r := range results { | |
| if !r.Passed { | |
| failedChecks[r.Check] = true | |
| } | |
| } | |
| for _, expected := range tt.failChecks { | |
| if !failedChecks[expected] { | |
| t.Errorf("expected check '%s' to fail, but it passed", expected) | |
| } | |
| } | |
| }) | |
| } | |
| } | |
| func TestAutoIncludeAgentNamespace(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| platform string | |
| agentNamespace string | |
| existingNS []string | |
| expectedNS []string | |
| expectLogMessage bool | |
| }{ | |
| { | |
| name: "When platform is AGENT with namespace it should auto-include", | |
| platform: "AGENT", | |
| agentNamespace: "my-agent-ns", | |
| existingNS: nil, | |
| expectedNS: []string{"my-agent-ns"}, | |
| expectLogMessage: true, | |
| }, | |
| { | |
| name: "When platform is AGENT and namespace already included it should not duplicate", | |
| platform: "AGENT", | |
| agentNamespace: "my-agent-ns", | |
| existingNS: []string{"my-agent-ns"}, | |
| expectedNS: []string{"my-agent-ns"}, | |
| expectLogMessage: false, | |
| }, | |
| { | |
| name: "When platform is AWS it should not include agent namespace", | |
| platform: "AWS", | |
| agentNamespace: "", | |
| existingNS: nil, | |
| expectedNS: nil, | |
| expectLogMessage: false, | |
| }, | |
| { | |
| name: "When platform is AGENT but namespace is empty it should skip", | |
| platform: "AGENT", | |
| agentNamespace: "", | |
| existingNS: nil, | |
| expectedNS: nil, | |
| expectLogMessage: false, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| opts := &CreateOptions{ | |
| Log: log.Log, | |
| IncludeNamespaces: tt.existingNS, | |
| } | |
| info := &supportoadp.PlatformInfo{ | |
| Type: tt.platform, | |
| AgentNamespace: tt.agentNamespace, | |
| } | |
| autoIncludeAgentNamespace(opts, info) | |
| if len(tt.expectedNS) == 0 && len(opts.IncludeNamespaces) == 0 { | |
| return | |
| } | |
| if len(opts.IncludeNamespaces) != len(tt.expectedNS) { | |
| t.Errorf("expected namespaces %v, got %v", tt.expectedNS, opts.IncludeNamespaces) | |
| } | |
| for i, ns := range tt.expectedNS { | |
| if i >= len(opts.IncludeNamespaces) || opts.IncludeNamespaces[i] != ns { | |
| t.Errorf("expected namespace[%d]=%q, got %v", i, ns, opts.IncludeNamespaces) | |
| } | |
| } | |
| }) | |
| } | |
| } | |
| func TestGetOutputTable(t *testing.T) { | |
| items := []unstructured.Unstructured{ | |
| { | |
| Object: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "test-backup-1", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": time.Now().Add(-2 * time.Hour).Format(time.RFC3339), | |
| }, | |
| "status": map[string]interface{}{ | |
| "phase": "Completed", | |
| }, | |
| }, | |
| }, | |
| { | |
| Object: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "test-backup-2", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": time.Now().Add(-48 * time.Hour).Format(time.RFC3339), | |
| }, | |
| "status": map[string]interface{}{ | |
| "phase": "Failed", | |
| }, | |
| }, | |
| }, | |
| } | |
| var buf bytes.Buffer | |
| err := outputTable(&buf, items, "Backup") | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| output := buf.String() | |
| if !bytes.Contains([]byte(output), []byte("NAME")) { | |
| t.Error("table output should contain NAME header") | |
| } | |
| if !bytes.Contains([]byte(output), []byte("test-backup-1")) { | |
| t.Error("table output should contain test-backup-1") | |
| } | |
| if !bytes.Contains([]byte(output), []byte("Completed")) { | |
| t.Error("table output should contain Completed status") | |
| } | |
| } | |
| func TestDestroyOptions(t *testing.T) { | |
| ctx := context.Background() | |
| scheme := runtime.NewScheme() | |
| backup := &unstructured.Unstructured{ | |
| Object: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "to-delete", | |
| "namespace": "openshift-adp", | |
| }, | |
| "spec": map[string]interface{}{}, | |
| }, | |
| } | |
| fakeClient := fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| WithObjects(backup). | |
| Build() | |
| opts := &DestroyOptions{ | |
| Name: "to-delete", | |
| OADPNamespace: "openshift-adp", | |
| Log: log.Log, | |
| Client: fakeClient, | |
| } | |
| err := opts.runDestroy(ctx, "Backup") | |
| if err != nil { | |
| t.Fatalf("expected no error, got: %v", err) | |
| } | |
| } | |
| func TestDestroyOptionsNotFound(t *testing.T) { | |
| ctx := context.Background() | |
| scheme := runtime.NewScheme() | |
| fakeClient := fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| Build() | |
| opts := &DestroyOptions{ | |
| Name: "nonexistent", | |
| OADPNamespace: "openshift-adp", | |
| Log: log.Log, | |
| Client: fakeClient, | |
| } | |
| err := opts.runDestroy(ctx, "Backup") | |
| if err == nil { | |
| t.Fatal("expected error for nonexistent backup, got none") | |
| } | |
| } | |
| import ( | |
| . "github.com/onsi/gomega" | |
| ) | |
| func TestVerifyBackup(t *testing.T) { | |
| g := NewWithT(t) | |
| tests := []struct { | |
| name string | |
| backup map[string]interface{} | |
| expectError bool | |
| failChecks []string | |
| }{ | |
| { | |
| name: "When backup is completed with valid items it should pass all checks", | |
| backup: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "test-backup", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": "2026-06-01T10:00:00Z", | |
| }, | |
| "spec": map[string]interface{}{ | |
| "storageLocation": "default", | |
| }, | |
| "status": map[string]interface{}{ | |
| "phase": "Completed", | |
| "expiration": time.Now().Add(24 * time.Hour).Format(time.RFC3339), | |
| "progress": map[string]interface{}{ | |
| "itemsBackedUp": int64(142), | |
| "totalItems": int64(142), | |
| }, | |
| }, | |
| }, | |
| expectError: false, | |
| }, | |
| { | |
| name: "When backup has failed phase it should fail the phase check", | |
| backup: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "failed-backup", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": "2026-06-01T10:00:00Z", | |
| }, | |
| "spec": map[string]interface{}{}, | |
| "status": map[string]interface{}{ | |
| "phase": "Failed", | |
| "progress": map[string]interface{}{ | |
| "itemsBackedUp": int64(0), | |
| "totalItems": int64(100), | |
| }, | |
| }, | |
| }, | |
| expectError: false, | |
| failChecks: []string{"phase", "items"}, | |
| }, | |
| { | |
| name: "When backup has expired it should fail the expiration check", | |
| backup: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "expired-backup", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": "2026-06-01T10:00:00Z", | |
| }, | |
| "spec": map[string]interface{}{}, | |
| "status": map[string]interface{}{ | |
| "phase": "Completed", | |
| "expiration": time.Now().Add(-2 * time.Hour).Format(time.RFC3339), | |
| "progress": map[string]interface{}{ | |
| "itemsBackedUp": int64(142), | |
| "totalItems": int64(142), | |
| }, | |
| }, | |
| }, | |
| expectError: false, | |
| failChecks: []string{"expiration"}, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| ctx := context.Background() | |
| scheme := runtime.NewScheme() | |
| backupObj := &unstructured.Unstructured{Object: tt.backup} | |
| fakeClient := fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| WithObjects(backupObj). | |
| Build() | |
| results, err := VerifyBackup(ctx, fakeClient, backupObj.GetName(), "openshift-adp", log.Log) | |
| g.Expect(err).To(HaveOccurred(), "expected error but got none") | |
| if tt.expectError { | |
| return | |
| } | |
| g.Expect(err).NotTo(HaveOccurred()) | |
| failedChecks := make(map[string]bool) | |
| for _, r := range results { | |
| if !r.Passed { | |
| failedChecks[r.Check] = true | |
| } | |
| } | |
| for _, expected := range tt.failChecks { | |
| g.Expect(failedChecks).To(HaveKey(expected), "expected check '%s' to fail", expected) | |
| } | |
| }) | |
| } | |
| } | |
| func TestAutoIncludeAgentNamespace(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| platform string | |
| agentNamespace string | |
| existingNS []string | |
| expectedNS []string | |
| expectLogMessage bool | |
| }{ | |
| { | |
| name: "When platform is AGENT with namespace it should auto-include", | |
| platform: "AGENT", | |
| agentNamespace: "my-agent-ns", | |
| existingNS: nil, | |
| expectedNS: []string{"my-agent-ns"}, | |
| expectLogMessage: true, | |
| }, | |
| { | |
| name: "When platform is AGENT and namespace already included it should not duplicate", | |
| platform: "AGENT", | |
| agentNamespace: "my-agent-ns", | |
| existingNS: []string{"my-agent-ns"}, | |
| expectedNS: []string{"my-agent-ns"}, | |
| expectLogMessage: false, | |
| }, | |
| { | |
| name: "When platform is AWS it should not include agent namespace", | |
| platform: "AWS", | |
| agentNamespace: "", | |
| existingNS: nil, | |
| expectedNS: nil, | |
| expectLogMessage: false, | |
| }, | |
| { | |
| name: "When platform is AGENT but namespace is empty it should skip", | |
| platform: "AGENT", | |
| agentNamespace: "", | |
| existingNS: nil, | |
| expectedNS: nil, | |
| expectLogMessage: false, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| opts := &CreateOptions{ | |
| Log: log.Log, | |
| IncludeNamespaces: tt.existingNS, | |
| } | |
| info := &supportoadp.PlatformInfo{ | |
| Type: tt.platform, | |
| AgentNamespace: tt.agentNamespace, | |
| } | |
| autoIncludeAgentNamespace(opts, info) | |
| if len(tt.expectedNS) == 0 && len(opts.IncludeNamespaces) == 0 { | |
| return | |
| } | |
| if len(opts.IncludeNamespaces) != len(tt.expectedNS) { | |
| t.Errorf("expected namespaces %v, got %v", tt.expectedNS, opts.IncludeNamespaces) | |
| } | |
| for i, ns := range tt.expectedNS { | |
| if i >= len(opts.IncludeNamespaces) || opts.IncludeNamespaces[i] != ns { | |
| t.Errorf("expected namespace[%d]=%q, got %v", i, ns, opts.IncludeNamespaces) | |
| } | |
| } | |
| }) | |
| } | |
| } | |
| func TestGetOutputTable(t *testing.T) { | |
| items := []unstructured.Unstructured{ | |
| { | |
| Object: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "test-backup-1", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": time.Now().Add(-2 * time.Hour).Format(time.RFC3339), | |
| }, | |
| "status": map[string]interface{}{ | |
| "phase": "Completed", | |
| }, | |
| }, | |
| }, | |
| { | |
| Object: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "test-backup-2", | |
| "namespace": "openshift-adp", | |
| "creationTimestamp": time.Now().Add(-48 * time.Hour).Format(time.RFC3339), | |
| }, | |
| "status": map[string]interface{}{ | |
| "phase": "Failed", | |
| }, | |
| }, | |
| }, | |
| } | |
| var buf bytes.Buffer | |
| err := outputTable(&buf, items, "Backup") | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| output := buf.String() | |
| if !bytes.Contains([]byte(output), []byte("NAME")) { | |
| t.Error("table output should contain NAME header") | |
| } | |
| if !bytes.Contains([]byte(output), []byte("test-backup-1")) { | |
| t.Error("table output should contain test-backup-1") | |
| } | |
| if !bytes.Contains([]byte(output), []byte("Completed")) { | |
| t.Error("table output should contain Completed status") | |
| } | |
| } | |
| func TestDestroyOptions(t *testing.T) { | |
| ctx := context.Background() | |
| scheme := runtime.NewScheme() | |
| backup := &unstructured.Unstructured{ | |
| Object: map[string]interface{}{ | |
| "apiVersion": "velero.io/v1", | |
| "kind": "Backup", | |
| "metadata": map[string]interface{}{ | |
| "name": "to-delete", | |
| "namespace": "openshift-adp", | |
| }, | |
| "spec": map[string]interface{}{}, | |
| }, | |
| } | |
| fakeClient := fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| WithObjects(backup). | |
| Build() | |
| opts := &DestroyOptions{ | |
| Name: "to-delete", | |
| OADPNamespace: "openshift-adp", | |
| Log: log.Log, | |
| Client: fakeClient, | |
| } | |
| err := opts.runDestroy(ctx, "Backup") | |
| if err != nil { | |
| t.Fatalf("expected no error, got: %v", err) | |
| } | |
| } | |
| func TestDestroyOptionsNotFound(t *testing.T) { | |
| ctx := context.Background() | |
| scheme := runtime.NewScheme() | |
| fakeClient := fake.NewClientBuilder(). | |
| WithScheme(scheme). | |
| Build() | |
| opts := &DestroyOptions{ | |
| Name: "nonexistent", | |
| OADPNamespace: "openshift-adp", | |
| Log: log.Log, | |
| Client: fakeClient, | |
| } | |
| err := opts.runDestroy(ctx, "Backup") | |
| if err == nil { | |
| t.Fatal("expected error for nonexistent backup, got none") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/oadp/lifecycle_test.go` around lines 18 - 308, Tests use raw testing
assertions (t.Errorf/t.Fatalf/t.Error/t.Fatal); update all five tests
(TestVerifyBackup, TestAutoIncludeAgentNamespace, TestGetOutputTable,
TestDestroyOptions, TestDestroyOptionsNotFound) to use Gomega matchers. For each
test create g := NewWithT(t) and replace error checks with
g.Expect(err).To(HaveOccurred()) or g.Expect(err).NotTo(HaveOccurred()), replace
equality/length/contain checks with g.Expect(...).To(Equal(...)),
g.Expect(...).To(HaveLen(...)),
g.Expect(stringOutput).To(ContainSubstring("...")), and assert boolean
conditions with g.Expect(...).To(BeTrue()/BeFalse()); also replace manual
map/slice existence checks in TestVerifyBackup/TestAutoIncludeAgentNamespace
with appropriate Expect assertions. Add the gomega import (NewWithT, matchers)
and remove t.* assertion calls.
Comment on lines
+202
to
+222
| // Step 3.5: Run backup integrity verification if --verify is set | ||
| if o.VerifyBackup && o.BackupName != "" && o.Client != nil { | ||
| o.Log.Info("Running backup integrity verification...") | ||
| results, verifyErr := VerifyBackup(ctx, o.Client, o.BackupName, o.OADPNamespace, o.Log) | ||
| if verifyErr != nil { | ||
| return fmt.Errorf("backup verification failed: %w", verifyErr) | ||
| } | ||
| allPassed := true | ||
| for _, r := range results { | ||
| if r.Passed { | ||
| o.Log.Info("Backup verification", "check", r.Check, "result", "pass", "detail", r.Detail) | ||
| } else { | ||
| o.Log.Info("Backup verification", "check", r.Check, "result", "FAIL", "detail", r.Detail) | ||
| allPassed = false | ||
| } | ||
| } | ||
| if !allPassed { | ||
| return fmt.Errorf("backup verification failed for '%s' — restore aborted", o.BackupName) | ||
| } | ||
| o.Log.Info("Backup verification passed, proceeding with restore") | ||
| } |
Contributor
There was a problem hiding this comment.
Verification silently skipped with --from-schedule.
When a user specifies --verify --from-schedule schedule-name, the verification step is silently skipped because BackupName is empty (line 203 condition). The user explicitly requested verification but receives no indication that it didn't run. This could create a false sense of security.
Recommend either:
- Add flag validation to make
--verifyand--from-schedulemutually exclusive, or - Log a warning when
--verifyis set but skipped due to schedule-based restore
Option 1: Mutual exclusion validation
Add validation in the RunE function after line 70:
if err := opts.validateEtcdSnapshotFlags(changedFlags); err != nil {
return err
}
+ if opts.VerifyBackup && opts.ScheduleName != "" {
+ return fmt.Errorf("--verify cannot be used with --from-schedule (schedule-based restores don't reference a specific backup name)")
+ }
return opts.RunRestore(cmd.Context())Option 2: Warning when verification is skipped
Add a warning log after line 195:
}
+ // Warn if verification was requested but will be skipped
+ if o.VerifyBackup && o.ScheduleName != "" {
+ o.Log.Info("Warning: --verify flag has no effect with --from-schedule (schedule-based restores select the latest backup at runtime)")
+ }
+
// Step 3: Generate restore object🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/oadp/restore.go` around lines 202 - 222, Validate and handle the case
where verification was requested but no explicit backup name was provided:
inside the RunE entry point, add a check for o.VerifyBackup combined with a
schedule-based restore (i.e., when o.BackupName is empty or a boolean/field
representing --from-schedule is set) and either return a user-facing error
(making --verify and --from-schedule mutually exclusive) or emit a clear warning
before proceeding; update the logic that currently checks o.VerifyBackup &&
o.BackupName != "" in the restore flow (the block using VerifyBackup(ctx,
o.Client, o.BackupName, o.OADPNamespace, o.Log)) so it cannot be silently
skipped when the user requested verification (reference o.VerifyBackup,
o.BackupName, RunE and the VerifyBackup call).
Contributor
|
@dpateriya: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Now I have all the details from all four failing checks. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll four failures stem from newly added OADP (OpenShift API for Data Protection) CLI code in PR #8685. The lint job catches two issues: Root CauseFour distinct issues, all in newly added code:
Recommendations
Evidence
|
Open
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements RFE-9399: adds lifecycle management commands, backup integrity verification, and Agent namespace auto-detection to the HCP DR CLI.
New Commands
hypershift get oadp-backupshypershift get oadp-restoreshypershift get oadp-scheduleshypershift destroy oadp-backup --name <name>hypershift destroy oadp-restore --name <name>hypershift destroy oadp-schedule --name <name>hypershift verify oadp-backup --name <name>New Flag
hypershift create oadp-restore --verify— runs backup integrity verification before creating the restore; blocks on failureAgent Namespace Auto-Detection
spec.platform.agent.agentNamespacefrom the HostedCluster--include-additional-namespacesVerification Checks (
verify oadp-backup)Completedstatus.expiration)AvailableTest Plan
VerifyBackup(pass, failed phase, expired)autoIncludeAgentNamespace(AGENT with ns, AGENT without ns, AWS, dedup)go vetpasses on all modified packagesgo test ./cmd/oadp/... ./support/oadp/...)Made with Cursor
Summary by CodeRabbit
Release Notes
hypershift getcommand to list OADP backups, restores, and schedules with filtering by hosted cluster and multiple output formats (table, JSON, YAML)hypershift destroycommand to delete OADP resourceshypershift verifycommand and--verifyflag to validate backup integrity before restore operations