Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 24 additions & 72 deletions cmd/nabsl-request/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Comment on lines 108 to 127
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Help/example no longer matches lookup behavior.

Line 108+ now treats NAME as the request object name/UUID in the admin namespace. The Example still says “Get a specific request by NABSL name,” which will now fail if the NABSL name doesn’t equal the request name. Please update the help/example text or reintroduce the NABSL-name lookup.

Suggested Example wording:

# Get a specific request by request name or UUID
kubectl oadp nabsl-request get nacuser01-my-bsl-96dfa8b7-...
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/nabsl-request/get.go` around lines 108 - 127, The help/example no longer
matches the lookup behavior in cmd/nabsl-request/get.go where the code path
using o.Name treats NAME as the request object name/UUID (the block that GETs
nacv1alpha1.NonAdminBackupStorageLocationRequest and calls
output.PrintWithFormat and printRequestTable); update the command Example/help
text to state that NAME must be the request name/UUID (e.g., "Get a specific
request by request name or UUID") and provide the suggested usage example, or
alternatively reintroduce the NABSL-name lookup code path (resolve NABSL name to
the request object name before calling o.client.Get) so the original example
remains valid.


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 {
Expand Down
10 changes: 0 additions & 10 deletions cmd/non-admin/backup/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand All @@ -133,7 +124,6 @@ type CreateOptions struct {

// NAB-specific fields
Name string
Force bool
client kbclient.WithWatch
currentNamespace string
}
Expand Down
2 changes: 0 additions & 2 deletions cmd/non-admin/backup/backup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ func TestNonAdminBackupCommands(t *testing.T) {
"--storage-location",
"--include-resources",
"--exclude-resources",
"--force",
},
},
{
Expand Down Expand Up @@ -244,7 +243,6 @@ func TestNonAdminBackupCreateFlags(t *testing.T) {
"--snapshot-volumes",
"--snapshot-move-data",
"--default-volumes-to-fs-backup",
"--force",
}

testutil.TestHelpCommand(t, binaryPath,
Expand Down
69 changes: 4 additions & 65 deletions cmd/non-admin/backup/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,8 @@ limitations under the License.
*/

import (
"bufio"
"context"
"fmt"
"os"
"strings"

"github.com/spf13/cobra"
"github.com/spf13/pflag"
Expand Down Expand Up @@ -80,15 +77,13 @@ 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
}

func NewCreateOptions() *CreateOptions {
return &CreateOptions{
CreateOptions: velerobackup.NewCreateOptions(),
Force: false,
}
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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"
}
2 changes: 1 addition & 1 deletion cmd/non-admin/bsl/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ 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"
"github.com/vmware-tanzu/velero/pkg/builder"
"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"
)

Expand Down
15 changes: 13 additions & 2 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 1 addition & 5 deletions docs/oadp-self-service.md
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,6 @@ type CreateOptions struct {

// NAB/NAR-specific fields
Name string
Force bool // backup only
client kbclient.WithWatch
currentNamespace string
}
Expand All @@ -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: `["*"]`)
Expand All @@ -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):**
Expand Down
Loading