From 9ec3dbae9770f75f5402be0a7dafc99056fbe831 Mon Sep 17 00:00:00 2001 From: Joseph Date: Tue, 17 Feb 2026 13:44:56 -0500 Subject: [PATCH 1/2] Simplify NABSL request listing and fix output handling - Remove complex NABSL-to-request UUID mapping in favor of direct request listing - Fix import path to use internal output package instead of Velero's - Skip output wrapper for delete commands to allow real-time display - Improve parent command checking for logs/describe/delete patterns Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Joseph --- cmd/nabsl-request/get.go | 96 ++++++++++--------------------------- cmd/non-admin/bsl/create.go | 2 +- cmd/root.go | 15 +++++- 3 files changed, 38 insertions(+), 75 deletions(-) diff --git a/cmd/nabsl-request/get.go b/cmd/nabsl-request/get.go index 00729edd..8a1f1502 100644 --- a/cmd/nabsl-request/get.go +++ b/cmd/nabsl-request/get.go @@ -105,92 +105,44 @@ func (o *GetOptions) Run(c *cobra.Command, f client.Factory) error { // Get the admin namespace (from client config) where requests are stored adminNS := f.Namespace() - // Get the current namespace to find user's NABSLs - currentNS, err := shared.GetCurrentNamespace() - if err != nil { - return fmt.Errorf("failed to determine current namespace: %w", err) - } - - // First get all NABSLs in user's namespace to find related requests - var nabslList nacv1alpha1.NonAdminBackupStorageLocationList - err = o.client.List(context.Background(), &nabslList, kbclient.InNamespace(currentNS)) - if err != nil { - return fmt.Errorf("failed to list NABSLs: %w", err) - } - - // Collect request UUIDs from NABSL statuses - requestUUIDs := make(map[string]string) // UUID -> NABSL name - for _, nabsl := range nabslList.Items { - if nabsl.Status.VeleroBackupStorageLocation != nil && nabsl.Status.VeleroBackupStorageLocation.NACUUID != "" { - requestUUIDs[nabsl.Status.VeleroBackupStorageLocation.NACUUID] = nabsl.Name - } - } - if o.Name != "" { - // Get specific request by UUID or NABSL name - var targetUUID string - - // Check if o.Name is a UUID or NABSL name - if _, exists := requestUUIDs[o.Name]; exists { - // o.Name is a UUID - targetUUID = o.Name - } else { - // o.Name might be a NABSL name, find its UUID - for uuid, nabslName := range requestUUIDs { - if nabslName == o.Name { - targetUUID = uuid - break - } - } - } - - if targetUUID != "" { - var request nacv1alpha1.NonAdminBackupStorageLocationRequest - err := o.client.Get(context.Background(), kbclient.ObjectKey{ - Name: targetUUID, - Namespace: adminNS, - }, &request) - if err != nil { - return fmt.Errorf("failed to get request for %q: %w", o.Name, err) - } - - if printed, err := output.PrintWithFormat(c, &request); printed || err != nil { - return err - } - - list := &nacv1alpha1.NonAdminBackupStorageLocationRequestList{ - Items: []nacv1alpha1.NonAdminBackupStorageLocationRequest{request}, - } - return printRequestTable(list) - } - - return fmt.Errorf("request %q not found for NABSLs in namespace %s", o.Name, currentNS) - } - - // List all requests related to user's NABSLs - var userRequests []nacv1alpha1.NonAdminBackupStorageLocationRequest - for uuid := range requestUUIDs { + // Get specific request by name (UUID) var request nacv1alpha1.NonAdminBackupStorageLocationRequest err := o.client.Get(context.Background(), kbclient.ObjectKey{ - Name: uuid, + Name: o.Name, Namespace: adminNS, }, &request) if err != nil { - // Request might not exist yet, skip - continue + return fmt.Errorf("failed to get request %q: %w", o.Name, err) + } + + if printed, err := output.PrintWithFormat(c, &request); printed || err != nil { + return err + } + + list := &nacv1alpha1.NonAdminBackupStorageLocationRequestList{ + Items: []nacv1alpha1.NonAdminBackupStorageLocationRequest{request}, } - userRequests = append(userRequests, request) + return printRequestTable(list) } - requestList := &nacv1alpha1.NonAdminBackupStorageLocationRequestList{ - Items: userRequests, + // List all requests in admin namespace + var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList + var err error + if o.AllNamespaces { + err = o.client.List(context.Background(), &requestList) + } else { + err = o.client.List(context.Background(), &requestList, kbclient.InNamespace(adminNS)) + } + if err != nil { + return fmt.Errorf("failed to list requests: %w", err) } - if printed, err := output.PrintWithFormat(c, requestList); printed || err != nil { + if printed, err := output.PrintWithFormat(c, &requestList); printed || err != nil { return err } - return printRequestTable(requestList) + return printRequestTable(&requestList) } func printRequestTable(requestList *nacv1alpha1.NonAdminBackupStorageLocationRequestList) error { diff --git a/cmd/non-admin/bsl/create.go b/cmd/non-admin/bsl/create.go index f216faa8..7ed22e5e 100644 --- a/cmd/non-admin/bsl/create.go +++ b/cmd/non-admin/bsl/create.go @@ -25,6 +25,7 @@ import ( "github.com/spf13/pflag" kbclient "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/migtools/oadp-cli/cmd/non-admin/output" "github.com/migtools/oadp-cli/cmd/shared" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -32,7 +33,6 @@ import ( "github.com/vmware-tanzu/velero/pkg/client" "github.com/vmware-tanzu/velero/pkg/cmd" "github.com/vmware-tanzu/velero/pkg/cmd/util/flag" - "github.com/vmware-tanzu/velero/pkg/cmd/util/output" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) diff --git a/cmd/root.go b/cmd/root.go index 40e82638..9d1f8963 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -210,10 +210,21 @@ func replaceVeleroWithOADP(cmd *cobra.Command) *cobra.Command { cmd.Example = replaceVeleroCommandWithOADP(cmd.Example) // Skip wrapping commands that stream output or make long-running requests - // to allow real-time display without buffering + // to allow real-time display without buffering. Check both the command itself + // and its parent to handle both "backup describe" and "describe backup" patterns. isLogsCommand := cmd.Use == "logs" || strings.HasPrefix(cmd.Use, "logs ") isDescribeCommand := cmd.Use == "describe" || strings.HasPrefix(cmd.Use, "describe ") - shouldSkipWrapper := isLogsCommand || isDescribeCommand + isDeleteCommand := cmd.Use == "delete" || strings.HasPrefix(cmd.Use, "delete ") + + // Also check if parent command is one we should skip + if cmd.Parent() != nil { + parentUse := cmd.Parent().Use + isLogsCommand = isLogsCommand || parentUse == "logs" || strings.HasPrefix(parentUse, "logs ") + isDescribeCommand = isDescribeCommand || parentUse == "describe" || strings.HasPrefix(parentUse, "describe ") + isDeleteCommand = isDeleteCommand || parentUse == "delete" || strings.HasPrefix(parentUse, "delete ") + } + + shouldSkipWrapper := isLogsCommand || isDescribeCommand || isDeleteCommand // Wrap the Run function to replace velero in output if cmd.Run != nil && !shouldSkipWrapper { From 2271062469ff910331850bf96972c697c652c7f3 Mon Sep 17 00:00:00 2001 From: Joseph Date: Tue, 17 Feb 2026 19:38:20 -0500 Subject: [PATCH 2/2] Remove --force flag and make storage location required for backup creation The --force flag allowed users to bypass the storage-location requirement, which could lead to unexpected behavior. This change removes the flag and simplifies the backup creation flow by making --storage-location a required parameter. Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Joseph --- cmd/non-admin/backup/README.md | 10 ----- cmd/non-admin/backup/backup_test.go | 2 - cmd/non-admin/backup/create.go | 69 ++--------------------------- docs/oadp-self-service.md | 6 +-- 4 files changed, 5 insertions(+), 82 deletions(-) diff --git a/cmd/non-admin/backup/README.md b/cmd/non-admin/backup/README.md index 5ba9f9c8..d2efb38b 100644 --- a/cmd/non-admin/backup/README.md +++ b/cmd/non-admin/backup/README.md @@ -45,12 +45,6 @@ The following flags represent the minimal viable product for backup creation: | `--snapshot-move-data` | OptionalBool | - | Move snapshot data | ✅ MVP | | `--default-volumes-to-fs-backup` | OptionalBool | - | Use filesystem backup | ✅ MVP | -### Control Flags - -| Flag | Type | Default | Description | Status | -|------|------|---------|-------------|--------| -| `--force`, `-f` | Boolean | false | Skip storage-location requirement | ✅ MVP | - ## Restricted Flags (Not Available) The following flags are **restricted** for non-admin users per the NAB API restrictions: @@ -110,9 +104,6 @@ oadp nonadmin backup create my-backup \ # Create backup with specific storage location oadp nonadmin backup create my-backup \ --storage-location my-nabsl - -# Force creation with admin defaults (interactive confirmation) -oadp nonadmin backup create my-backup --force ``` ## Architecture Notes @@ -133,7 +124,6 @@ type CreateOptions struct { // NAB-specific fields Name string - Force bool client kbclient.WithWatch currentNamespace string } diff --git a/cmd/non-admin/backup/backup_test.go b/cmd/non-admin/backup/backup_test.go index f1f08866..3c28f50c 100644 --- a/cmd/non-admin/backup/backup_test.go +++ b/cmd/non-admin/backup/backup_test.go @@ -51,7 +51,6 @@ func TestNonAdminBackupCommands(t *testing.T) { "--storage-location", "--include-resources", "--exclude-resources", - "--force", }, }, { @@ -244,7 +243,6 @@ func TestNonAdminBackupCreateFlags(t *testing.T) { "--snapshot-volumes", "--snapshot-move-data", "--default-volumes-to-fs-backup", - "--force", } testutil.TestHelpCommand(t, binaryPath, diff --git a/cmd/non-admin/backup/create.go b/cmd/non-admin/backup/create.go index 32fb3dd5..f0d4d5c3 100644 --- a/cmd/non-admin/backup/create.go +++ b/cmd/non-admin/backup/create.go @@ -17,11 +17,8 @@ limitations under the License. */ import ( - "bufio" "context" "fmt" - "os" - "strings" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -80,7 +77,6 @@ type CreateOptions struct { // NAB-specific fields Name string // The NonAdminBackup resource name (maps to Velero's BackupName) - Force bool // NAB-specific: bypass storage-location requirement client kbclient.WithWatch currentNamespace string } @@ -88,7 +84,6 @@ type CreateOptions struct { func NewCreateOptions() *CreateOptions { return &CreateOptions{ CreateOptions: velerobackup.NewCreateOptions(), - Force: false, } } @@ -120,9 +115,6 @@ func (o *CreateOptions) BindFlags(flags *pflag.FlagSet) { f = flags.VarPF(&o.DefaultVolumesToFsBackup, "default-volumes-to-fs-backup", "", "Use pod volume file system backup by default for volumes.") f.NoOptDefVal = cmd.TRUE - - // NAB-specific control flag - flags.BoolVarP(&o.Force, "force", "f", o.Force, "Force creation without specifying a storage location (uses admin defaults).") } func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Factory) error { @@ -139,8 +131,8 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto } // Storage location validation - if !o.Force && o.StorageLocation == "" { - return fmt.Errorf("a valid NonAdminBackupStorageLocation must be provided via --storage-location, or use --force to create with admin defaults") + if o.StorageLocation == "" { + return fmt.Errorf("--storage-location is required") } return nil @@ -178,18 +170,13 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error { return err } - // Prompt for confirmation if using force without storage location - if err := o.promptForceConfirmation(); err != nil { - return err - } - // Create the backup if err := o.client.Create(context.TODO(), nonAdminBackup, &kbclient.CreateOptions{}); err != nil { return err } - fmt.Println(o.formatSuccessMessage(nonAdminBackup.Name)) - fmt.Print(o.formatCommandSuggestion(nonAdminBackup.Name)) + fmt.Printf("NonAdminBackup request %q submitted successfully.\n", nonAdminBackup.Name) + fmt.Printf("Run `oc oadp nonadmin backup describe %s` or `oc oadp nonadmin backup logs %s` for more details.\n", nonAdminBackup.Name, nonAdminBackup.Name) return nil } @@ -250,51 +237,3 @@ func (o *CreateOptions) createNonAdminBackup(namespace string, backupSpec *veler }). Result() } - -// promptForceConfirmation prompts the user to confirm when using --force without storage location -func (o *CreateOptions) promptForceConfirmation() error { - if !o.Force || o.StorageLocation != "" { - return nil - } - - fmt.Println("\nWARNING: Using --force without specifying a storage location is not ideal.") - fmt.Println("This will use admin defaults and certain features like logs may not work as expected.") - - fmt.Print("Do you want to continue? (y/N): ") - reader := bufio.NewReader(os.Stdin) - response, err := reader.ReadString('\n') - if err != nil { - return fmt.Errorf("failed to read user input: %w", err) - } - - response = strings.TrimSpace(strings.ToLower(response)) - if response != "y" && response != "yes" { - fmt.Println("Operation cancelled.") - return fmt.Errorf("operation cancelled by user") - } - - fmt.Println() - return nil -} - -// usingAdminDefaults returns true if force flag is used without storage location -func (o *CreateOptions) usingAdminDefaults() bool { - return o.Force && o.StorageLocation == "" -} - -// formatSuccessMessage returns the success message with optional admin defaults note -func (o *CreateOptions) formatSuccessMessage(name string) string { - if o.usingAdminDefaults() { - return fmt.Sprintf("NonAdminBackup request %q submitted successfully (using admin defaults).", name) - } - return fmt.Sprintf("NonAdminBackup request %q submitted successfully.", name) -} - -// formatCommandSuggestion returns the command suggestion with optional admin defaults note -func (o *CreateOptions) formatCommandSuggestion(name string) string { - baseMsg := fmt.Sprintf("Run `oc oadp nonadmin backup describe %s` or `oc oadp nonadmin backup logs %s` for more details.", name, name) - if o.usingAdminDefaults() { - return fmt.Sprintf("%s (Created using admin defaults)\n", baseMsg) - } - return baseMsg + "\n" -} diff --git a/docs/oadp-self-service.md b/docs/oadp-self-service.md index 7b5a588e..591c3f91 100644 --- a/docs/oadp-self-service.md +++ b/docs/oadp-self-service.md @@ -520,7 +520,6 @@ type CreateOptions struct { // NAB/NAR-specific fields Name string - Force bool // backup only client kbclient.WithWatch currentNamespace string } @@ -534,7 +533,7 @@ type CreateOptions struct { - ✅ **Easy Enhancement Path** - Add flags incrementally as features mature - ✅ **Type Safety** - Reuses Velero's validated type definitions -## Backup Create: MVP Flags (13 total) +## Backup Create: MVP Flags (12 total) ### Resource Filtering (2) - `--include-resources` (default: `["*"]`) @@ -558,9 +557,6 @@ type CreateOptions struct { - `--snapshot-move-data` - `--default-volumes-to-fs-backup` -### Control Flags (1) -- `--force` (bypass storage-location requirement) - ### Flags Removed from Original Implementation **Restricted (API limitations):**