diff --git a/.gitignore b/.gitignore index befa29e..c45a9fa 100644 --- a/.gitignore +++ b/.gitignore @@ -48,3 +48,5 @@ go.work .Trashes ehthumbs.db Thumbs.db + +vendor/ \ No newline at end of file diff --git a/cmd/non-admin/backup/describe.go b/cmd/non-admin/backup/describe.go index 22214b2..59c92b7 100644 --- a/cmd/non-admin/backup/describe.go +++ b/cmd/non-admin/backup/describe.go @@ -72,7 +72,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command { } // Print in Velero-style format - printNonAdminBackupDetails(cmd, &nab, kbClient, backupName, userNamespace, effectiveTimeout) + printNonAdminBackupDetails(cmd, &nab, kbClient, backupName, userNamespace, effectiveTimeout, details) // Add detailed output if --details flag is set if details { @@ -98,7 +98,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command { } // printNonAdminBackupDetails prints backup details in Velero admin describe format -func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup, kbClient kbclient.Client, backupName string, userNamespace string, timeout time.Duration) { +func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup, kbClient kbclient.Client, backupName string, userNamespace string, timeout time.Duration, showDetails bool) { out := cmd.OutOrStdout() // Get Velero backup reference if available @@ -322,22 +322,24 @@ func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBac fmt.Fprintf(out, "\n") - // Fetch and display Resource List - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() + // Fetch and display Resource List (only if showDetails is true) + if showDetails { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() - resourceList, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ - BackupName: backupName, - DataType: "BackupResourceList", - Namespace: userNamespace, - HTTPTimeout: timeout, - }) + resourceList, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{ + BackupName: backupName, + DataType: "BackupResourceList", + Namespace: userNamespace, + HTTPTimeout: timeout, + }) - if err == nil && resourceList != "" { - if formattedList := formatResourceList(resourceList); formattedList != "" { - fmt.Fprintf(out, "Resource List:\n") - fmt.Fprintf(out, "%s\n", formattedList) - fmt.Fprintf(out, "\n") + if err == nil && resourceList != "" { + if formattedList := formatResourceList(resourceList); formattedList != "" { + fmt.Fprintf(out, "Resource List:\n") + fmt.Fprintf(out, "%s\n", formattedList) + fmt.Fprintf(out, "\n") + } } } diff --git a/cmd/non-admin/backup/get.go b/cmd/non-admin/backup/get.go index a1118b8..9756182 100644 --- a/cmd/non-admin/backup/get.go +++ b/cmd/non-admin/backup/get.go @@ -20,11 +20,11 @@ import ( "fmt" "time" + "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" "github.com/spf13/cobra" "github.com/vmware-tanzu/velero/pkg/client" - "github.com/vmware-tanzu/velero/pkg/cmd/util/output" kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) diff --git a/cmd/non-admin/bsl/bsl.go b/cmd/non-admin/bsl/bsl.go index b8eed27..cd6a215 100644 --- a/cmd/non-admin/bsl/bsl.go +++ b/cmd/non-admin/bsl/bsl.go @@ -30,7 +30,7 @@ func NewBSLCommand(f client.Factory) *cobra.Command { } c.AddCommand( - NewCreateCommand(f), + NewCreateCommand(f, "create"), NewGetCommand(f, "get"), ) diff --git a/cmd/non-admin/bsl/create.go b/cmd/non-admin/bsl/create.go index 0fc0c5a..f216faa 100644 --- a/cmd/non-admin/bsl/create.go +++ b/cmd/non-admin/bsl/create.go @@ -36,11 +36,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func NewCreateCommand(f client.Factory) *cobra.Command { +func NewCreateCommand(f client.Factory, use string) *cobra.Command { o := NewCreateOptions() c := &cobra.Command{ - Use: "create NAME", + Use: use + " NAME", Short: "Create a non-admin backup storage location", Args: cobra.ExactArgs(1), Run: func(c *cobra.Command, args []string) { diff --git a/cmd/non-admin/bsl/get.go b/cmd/non-admin/bsl/get.go index 8db5f29..5e1a503 100644 --- a/cmd/non-admin/bsl/get.go +++ b/cmd/non-admin/bsl/get.go @@ -21,11 +21,11 @@ import ( "fmt" "time" + "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" "github.com/spf13/cobra" "github.com/vmware-tanzu/velero/pkg/client" - "github.com/vmware-tanzu/velero/pkg/cmd/util/output" kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) diff --git a/cmd/non-admin/output/output.go b/cmd/non-admin/output/output.go new file mode 100644 index 0000000..a27eca0 --- /dev/null +++ b/cmd/non-admin/output/output.go @@ -0,0 +1,148 @@ +/* +Copyright 2025 The OADP CLI Contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package output + +import ( + "bytes" + "fmt" + "io" + + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/pkg/errors" + "github.com/spf13/cobra" + "github.com/spf13/pflag" + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + velerooutput "github.com/vmware-tanzu/velero/pkg/cmd/util/output" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" +) + +// NonAdminScheme returns a runtime.Scheme with NonAdmin types registered +func NonAdminScheme() *runtime.Scheme { + scheme := runtime.NewScheme() + + // Add NonAdmin types + if err := nacv1alpha1.AddToScheme(scheme); err != nil { + panic(fmt.Sprintf("failed to add NonAdmin types to scheme: %v", err)) + } + + // Add Velero types for compatibility + if err := velerov1api.AddToScheme(scheme); err != nil { + panic(fmt.Sprintf("failed to add Velero types to scheme: %v", err)) + } + + return scheme +} + +// BindFlags wraps Velero's BindFlags to add output flags +func BindFlags(flags *pflag.FlagSet) { + velerooutput.BindFlags(flags) +} + +// ClearOutputFlagDefault wraps Velero's ClearOutputFlagDefault +func ClearOutputFlagDefault(cmd *cobra.Command) { + velerooutput.ClearOutputFlagDefault(cmd) +} + +// PrintWithFormat prints the provided object in the format specified by +// the command's flags. This is a custom implementation for nonadmin commands +// that supports NonAdmin CRD types (NonAdminBackup, NonAdminRestore, etc.) +func PrintWithFormat(c *cobra.Command, obj runtime.Object) (bool, error) { + format := velerooutput.GetOutputFlagValue(c) + if format == "" { + return false, nil + } + + switch format { + case "json", "yaml": + return printEncoded(obj, format) + case "table": + // Table format is not supported by this function + // The caller should handle table printing + return false, nil + } + + return false, errors.Errorf("unsupported output format %q; valid values are 'table', 'json', and 'yaml'", format) +} + +func printEncoded(obj runtime.Object, format string) (bool, error) { + // assume we're printing obj + toPrint := obj + + if meta.IsListType(obj) { + list, _ := meta.ExtractList(obj) + if len(list) == 1 { + // if obj was a list and there was only 1 item, just print that 1 instead of a list + toPrint = list[0] + } + } + + encoded, err := encode(toPrint, format) + if err != nil { + return false, err + } + + fmt.Println(string(encoded)) + + return true, nil +} + +func encode(obj runtime.Object, format string) ([]byte, error) { + buf := new(bytes.Buffer) + + if err := encodeTo(obj, format, buf); err != nil { + return nil, err + } + return buf.Bytes(), nil +} + +func encodeTo(obj runtime.Object, format string, w io.Writer) error { + encoder, err := encoderFor(format, obj) + if err != nil { + return err + } + + return errors.WithStack(encoder.Encode(obj, w)) +} + +func encoderFor(format string, obj runtime.Object) (runtime.Encoder, error) { + var encoder runtime.Encoder + + // Use NonAdminScheme instead of Velero's scheme + codecFactory := serializer.NewCodecFactory(NonAdminScheme()) + + desiredMediaType := fmt.Sprintf("application/%s", format) + serializerInfo, found := runtime.SerializerInfoForMediaType(codecFactory.SupportedMediaTypes(), desiredMediaType) + if !found { + return nil, errors.Errorf("unable to locate an encoder for %q", desiredMediaType) + } + if serializerInfo.PrettySerializer != nil { + encoder = serializerInfo.PrettySerializer + } else { + encoder = serializerInfo.Serializer + } + + if !obj.GetObjectKind().GroupVersionKind().Empty() { + return encoder, nil + } + + // Use the appropriate GroupVersion for encoding + // For NonAdmin types, use nacv1alpha1.GroupVersion + encoder = codecFactory.EncoderForVersion(encoder, nacv1alpha1.GroupVersion) + return encoder, nil +} diff --git a/cmd/non-admin/output/output_test.go b/cmd/non-admin/output/output_test.go new file mode 100644 index 0000000..4a130fa --- /dev/null +++ b/cmd/non-admin/output/output_test.go @@ -0,0 +1,520 @@ +/* +Copyright 2025 The OADP CLI Contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package output + +import ( + "bytes" + "io" + "os" + "strings" + "testing" + + nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" + "github.com/spf13/cobra" + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func TestNonAdminScheme(t *testing.T) { + scheme := NonAdminScheme() + + tests := []struct { + name string + gvk schema.GroupVersionKind + objType runtime.Object + }{ + { + name: "NonAdminBackup is registered", + gvk: schema.GroupVersionKind{ + Group: "oadp.openshift.io", + Version: "v1alpha1", + Kind: "NonAdminBackup", + }, + objType: &nacv1alpha1.NonAdminBackup{}, + }, + { + name: "NonAdminBackupList is registered", + gvk: schema.GroupVersionKind{ + Group: "oadp.openshift.io", + Version: "v1alpha1", + Kind: "NonAdminBackupList", + }, + objType: &nacv1alpha1.NonAdminBackupList{}, + }, + { + name: "NonAdminRestore is registered", + gvk: schema.GroupVersionKind{ + Group: "oadp.openshift.io", + Version: "v1alpha1", + Kind: "NonAdminRestore", + }, + objType: &nacv1alpha1.NonAdminRestore{}, + }, + { + name: "NonAdminRestoreList is registered", + gvk: schema.GroupVersionKind{ + Group: "oadp.openshift.io", + Version: "v1alpha1", + Kind: "NonAdminRestoreList", + }, + objType: &nacv1alpha1.NonAdminRestoreList{}, + }, + { + name: "NonAdminBackupStorageLocation is registered", + gvk: schema.GroupVersionKind{ + Group: "oadp.openshift.io", + Version: "v1alpha1", + Kind: "NonAdminBackupStorageLocation", + }, + objType: &nacv1alpha1.NonAdminBackupStorageLocation{}, + }, + { + name: "Velero Backup is registered", + gvk: schema.GroupVersionKind{ + Group: "velero.io", + Version: "v1", + Kind: "Backup", + }, + objType: &velerov1api.Backup{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Check if the type is recognized by the scheme + gvks, _, err := scheme.ObjectKinds(tt.objType) + if err != nil { + t.Fatalf("Failed to get ObjectKinds: %v", err) + } + + found := false + for _, gvk := range gvks { + if gvk.Group == tt.gvk.Group && gvk.Version == tt.gvk.Version && gvk.Kind == tt.gvk.Kind { + found = true + break + } + } + + if !found { + t.Errorf("Expected GVK %v to be registered in scheme, but it was not found", tt.gvk) + } + }) + } +} + +func TestBindFlags(t *testing.T) { + cmd := &cobra.Command{} + BindFlags(cmd.Flags()) + + // Check that the output flag is bound + outputFlag := cmd.Flags().Lookup("output") + if outputFlag == nil { + t.Fatal("Expected 'output' flag to be bound, but it was not found") + } + + // Check that the label-columns flag is bound + labelColumnsFlag := cmd.Flags().Lookup("label-columns") + if labelColumnsFlag == nil { + t.Fatal("Expected 'label-columns' flag to be bound, but it was not found") + } + + // Check that the show-labels flag is bound + showLabelsFlag := cmd.Flags().Lookup("show-labels") + if showLabelsFlag == nil { + t.Fatal("Expected 'show-labels' flag to be bound, but it was not found") + } +} + +func TestClearOutputFlagDefault(t *testing.T) { + cmd := &cobra.Command{} + BindFlags(cmd.Flags()) + + // Initially, the default should be "table" + outputFlag := cmd.Flags().Lookup("output") + if outputFlag.DefValue != "table" { + t.Errorf("Expected default value to be 'table', got %q", outputFlag.DefValue) + } + + // Clear the default + ClearOutputFlagDefault(cmd) + + // After clearing, the default should be empty + if outputFlag.DefValue != "" { + t.Errorf("Expected default value to be empty after clearing, got %q", outputFlag.DefValue) + } +} + +func TestPrintWithFormat(t *testing.T) { + tests := []struct { + name string + outputFormat string + obj runtime.Object + expectPrinted bool + expectError bool + validateOutput func(t *testing.T, output string) + }{ + { + name: "empty format returns false", + outputFormat: "", + obj: createTestBackup("test-backup"), + expectPrinted: false, + expectError: false, + }, + { + name: "yaml format", + outputFormat: "yaml", + obj: createTestBackup("test-backup"), + expectPrinted: true, + expectError: false, + validateOutput: func(t *testing.T, output string) { + if !strings.Contains(output, "apiVersion: oadp.openshift.io/v1alpha1") { + t.Error("Expected YAML output to contain apiVersion") + } + if !strings.Contains(output, "kind: NonAdminBackup") { + t.Error("Expected YAML output to contain kind") + } + if !strings.Contains(output, "name: test-backup") { + t.Error("Expected YAML output to contain name") + } + }, + }, + { + name: "json format", + outputFormat: "json", + obj: createTestBackup("test-backup"), + expectPrinted: true, + expectError: false, + validateOutput: func(t *testing.T, output string) { + if !strings.Contains(output, `"apiVersion": "oadp.openshift.io/v1alpha1"`) { + t.Error("Expected JSON output to contain apiVersion") + } + if !strings.Contains(output, `"kind": "NonAdminBackup"`) { + t.Error("Expected JSON output to contain kind") + } + if !strings.Contains(output, `"name": "test-backup"`) { + t.Error("Expected JSON output to contain name") + } + }, + }, + { + name: "table format returns false", + outputFormat: "table", + obj: createTestBackup("test-backup"), + expectPrinted: false, + expectError: false, + }, + { + name: "invalid format returns error", + outputFormat: "invalid", + obj: createTestBackup("test-backup"), + expectPrinted: false, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a command with the output flag + cmd := &cobra.Command{} + BindFlags(cmd.Flags()) + if tt.outputFormat != "" { + if err := cmd.Flags().Set("output", tt.outputFormat); err != nil { + t.Fatalf("Failed to set output flag: %v", err) + } + } + + // Capture stdout + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + printed, err := PrintWithFormat(cmd, tt.obj) + + // Restore stdout and read captured output + w.Close() + os.Stdout = oldStdout + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("Failed to read output: %v", err) + } + output := buf.String() + + // Check error expectation + if tt.expectError && err == nil { + t.Error("Expected error but got none") + } + if !tt.expectError && err != nil { + t.Errorf("Unexpected error: %v", err) + } + + // Check printed expectation + if printed != tt.expectPrinted { + t.Errorf("Expected printed=%v, got %v", tt.expectPrinted, printed) + } + + // Validate output if provided + if tt.validateOutput != nil { + tt.validateOutput(t, output) + } + }) + } +} + +func TestPrintWithFormatList(t *testing.T) { + tests := []struct { + name string + outputFormat string + obj runtime.Object + expectSingle bool // Should single item list be printed as single object? + validateCount func(t *testing.T, output string) + }{ + { + name: "single item list printed as single object in yaml", + outputFormat: "yaml", + obj: &nacv1alpha1.NonAdminBackupList{ + Items: []nacv1alpha1.NonAdminBackup{ + *createTestBackup("backup-1"), + }, + }, + expectSingle: true, + validateCount: func(t *testing.T, output string) { + // Single object should not have "items:" field + if strings.Contains(output, "items:") { + t.Error("Single item from list should not contain 'items:' field") + } + if !strings.Contains(output, "name: backup-1") { + t.Error("Expected output to contain backup name") + } + }, + }, + { + name: "multiple item list printed as list in yaml", + outputFormat: "yaml", + obj: &nacv1alpha1.NonAdminBackupList{ + Items: []nacv1alpha1.NonAdminBackup{ + *createTestBackup("backup-1"), + *createTestBackup("backup-2"), + }, + }, + expectSingle: false, + validateCount: func(t *testing.T, output string) { + // Multiple objects should have "items:" field + if !strings.Contains(output, "items:") { + t.Error("Multiple items should contain 'items:' field") + } + if !strings.Contains(output, "name: backup-1") { + t.Error("Expected output to contain first backup name") + } + if !strings.Contains(output, "name: backup-2") { + t.Error("Expected output to contain second backup name") + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := &cobra.Command{} + BindFlags(cmd.Flags()) + if err := cmd.Flags().Set("output", tt.outputFormat); err != nil { + t.Fatalf("Failed to set output flag: %v", err) + } + + // Capture stdout + oldStdout := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + _, err := PrintWithFormat(cmd, tt.obj) + + // Restore stdout and read captured output + w.Close() + os.Stdout = oldStdout + var buf bytes.Buffer + if _, copyErr := io.Copy(&buf, r); copyErr != nil { + t.Fatalf("Failed to read output: %v", copyErr) + } + output := buf.String() + + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if tt.validateCount != nil { + tt.validateCount(t, output) + } + }) + } +} + +func TestEncode(t *testing.T) { + tests := []struct { + name string + obj runtime.Object + format string + validate func(t *testing.T, data []byte) + }{ + { + name: "encode NonAdminBackup to yaml", + obj: createTestBackup("test-backup"), + format: "yaml", + validate: func(t *testing.T, data []byte) { + output := string(data) + if !strings.Contains(output, "apiVersion: oadp.openshift.io/v1alpha1") { + t.Error("Expected YAML to contain apiVersion") + } + if !strings.Contains(output, "kind: NonAdminBackup") { + t.Error("Expected YAML to contain kind") + } + }, + }, + { + name: "encode NonAdminBackup to json", + obj: createTestBackup("test-backup"), + format: "json", + validate: func(t *testing.T, data []byte) { + output := string(data) + if !strings.Contains(output, `"apiVersion"`) { + t.Error("Expected JSON to contain apiVersion") + } + if !strings.Contains(output, `"kind"`) { + t.Error("Expected JSON to contain kind") + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data, err := encode(tt.obj, tt.format) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if tt.validate != nil { + tt.validate(t, data) + } + }) + } +} + +func TestEncoderFor(t *testing.T) { + tests := []struct { + name string + format string + obj runtime.Object + expectError bool + }{ + { + name: "yaml encoder", + format: "yaml", + obj: createTestBackup("test"), + expectError: false, + }, + { + name: "json encoder", + format: "json", + obj: createTestBackup("test"), + expectError: false, + }, + { + name: "invalid format", + format: "xml", + obj: createTestBackup("test"), + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + encoder, err := encoderFor(tt.format, tt.obj) + + if tt.expectError { + if err == nil { + t.Error("Expected error but got none") + } + } else { + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if encoder == nil { + t.Error("Expected encoder but got nil") + } + } + }) + } +} + +func TestIsListType(t *testing.T) { + tests := []struct { + name string + obj runtime.Object + isList bool + }{ + { + name: "NonAdminBackupList is a list", + obj: &nacv1alpha1.NonAdminBackupList{}, + isList: true, + }, + { + name: "NonAdminBackup is not a list", + obj: &nacv1alpha1.NonAdminBackup{}, + isList: false, + }, + { + name: "NonAdminRestoreList is a list", + obj: &nacv1alpha1.NonAdminRestoreList{}, + isList: true, + }, + { + name: "NonAdminRestore is not a list", + obj: &nacv1alpha1.NonAdminRestore{}, + isList: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + isList := meta.IsListType(tt.obj) + if isList != tt.isList { + t.Errorf("Expected IsListType=%v, got %v", tt.isList, isList) + } + }) + } +} + +// Helper function to create a test NonAdminBackup +func createTestBackup(name string) *nacv1alpha1.NonAdminBackup { + return &nacv1alpha1.NonAdminBackup{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "oadp.openshift.io/v1alpha1", + Kind: "NonAdminBackup", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "test-namespace", + }, + Spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &velerov1api.BackupSpec{ + IncludedNamespaces: []string{"test-namespace"}, + }, + }, + } +} diff --git a/cmd/non-admin/restore/get.go b/cmd/non-admin/restore/get.go index 6b14224..2c9e655 100644 --- a/cmd/non-admin/restore/get.go +++ b/cmd/non-admin/restore/get.go @@ -20,11 +20,11 @@ import ( "fmt" "time" + "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" "github.com/spf13/cobra" "github.com/vmware-tanzu/velero/pkg/client" - "github.com/vmware-tanzu/velero/pkg/cmd/util/output" kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) diff --git a/cmd/non-admin/verbs/builder.go b/cmd/non-admin/verbs/builder.go index e6b9e9f..e432c5b 100644 --- a/cmd/non-admin/verbs/builder.go +++ b/cmd/non-admin/verbs/builder.go @@ -88,12 +88,14 @@ func (vb *NonAdminVerbBuilder) runEFunc(verb string) func(cmd *cobra.Command, ar } // Get the main command for the resource (e.g., "backup" command) + // This creates a FRESH command instance each time, which is important + // to avoid flag state persistence across invocations resourceCmd := handler.GetCommandFunc(vb.factory) if resourceCmd == nil { return fmt.Errorf("%s command not found for resource type %s", verb, resourceType) } - // Get the specific subcommand for the verb (e.g., "backup get" command) + // Get the specific subcommand for the verb (e.g., "backup describe" command) subCmd := handler.GetSubCommandFunc(resourceCmd) if subCmd == nil { return fmt.Errorf("%s %s command not found", resourceType, verb) @@ -102,11 +104,40 @@ func (vb *NonAdminVerbBuilder) runEFunc(verb string) func(cmd *cobra.Command, ar // Add flags to remaining args so they get passed to the delegated command remainingArgs = vb.addFlagsToArgs(cmd, remainingArgs) - // Create a new command instance to avoid argument inheritance - newSubCmd := vb.createCommandInstance(subCmd) - newSubCmd.SetArgs(remainingArgs) + // Use the original subcommand directly (not a copy) and set its args + // This ensures the RunE closure uses the correct flag variables + subCmd.SetArgs(remainingArgs) + + // Parse flags from the args we set + // IMPORTANT: We cannot use subCmd.Execute() here because it causes Cobra to + // re-parse os.Args from the root command, which results in incorrect command + // path parsing (e.g., "unknown command 'na' for 'backup'" error). + // Instead, we manually parse flags and execute the RunE function directly. + if err := subCmd.ParseFlags(remainingArgs); err != nil { + return err + } + + // Validate arguments + if subCmd.Args != nil { + // Get the args after flag parsing + flags := subCmd.Flags() + argsList := flags.Args() + if err := subCmd.Args(subCmd, argsList); err != nil { + return err + } + } + + // Execute the RunE function directly with parsed args + if subCmd.RunE != nil { + flags := subCmd.Flags() + return subCmd.RunE(subCmd, flags.Args()) + } else if subCmd.Run != nil { + flags := subCmd.Flags() + subCmd.Run(subCmd, flags.Args()) + return nil + } - return newSubCmd.Execute() + return fmt.Errorf("command %s does not have Run or RunE", subCmd.Name()) } } @@ -138,28 +169,10 @@ func (vb *NonAdminVerbBuilder) addFlagsToArgs(cmd *cobra.Command, remainingArgs return remainingArgs } -// createCommandInstance creates a new cobra.Command instance from an existing one to avoid argument/flag inheritance issues. -func (vb *NonAdminVerbBuilder) createCommandInstance(originalCmd *cobra.Command) *cobra.Command { - newCmd := &cobra.Command{ - Use: originalCmd.Use, - Short: originalCmd.Short, - Long: originalCmd.Long, - Run: originalCmd.Run, - RunE: originalCmd.RunE, - } - - // Copy flags from the original command - originalCmd.Flags().VisitAll(func(flag *pflag.Flag) { - newCmd.Flags().AddFlag(flag) - }) - // Also copy persistent flags - originalCmd.PersistentFlags().VisitAll(func(flag *pflag.Flag) { - newCmd.PersistentFlags().AddFlag(flag) - }) - return newCmd -} - // addFlagsFromResources adds flags from all registered resources to the verb command +// Note: We add flag references (not copies) here for recognition at the verb level. +// The actual command execution will use fresh command instances created by GetCommandFunc, +// so flag state won't persist across invocations. func (vb *NonAdminVerbBuilder) addFlagsFromResources(verbCmd *cobra.Command, verb string) { addedFlags := make(map[string]bool) diff --git a/cmd/non-admin/verbs/builder_test.go b/cmd/non-admin/verbs/builder_test.go index 2c4a86c..e3604aa 100644 --- a/cmd/non-admin/verbs/builder_test.go +++ b/cmd/non-admin/verbs/builder_test.go @@ -386,59 +386,153 @@ func TestNonAdminVerbBuilder_MultipleResourceTypes(t *testing.T) { } } -// TestNonAdminVerbBuilder_CreateCommandInstance tests command instance creation -func TestNonAdminVerbBuilder_CreateCommandInstance(t *testing.T) { +// TestNonAdminVerbBuilder_NilHandler tests handling of nil handlers +func TestNonAdminVerbBuilder_NilHandler(t *testing.T) { builder := NewNonAdminVerbBuilder(nil) - originalCmd := &cobra.Command{ - Use: "create NAME", - Short: "Create a resource", - Long: "Create a non-admin resource", + // Register a handler that returns nil for GetCommandFunc + builder.RegisterResource("nil-resource", NonAdminResourceHandler{ + GetCommandFunc: func(f client.Factory) *cobra.Command { return nil }, + GetSubCommandFunc: func(cmd *cobra.Command) *cobra.Command { return nil }, + }) + + verbCmd := builder.BuildVerbCommand(NonAdminVerbConfig{Use: "get"}) + verbCmd.SetArgs([]string{"nil-resource", "test-name"}) + + err := verbCmd.Execute() + if err == nil { + t.Error("Expected error when handler returns nil command, got nil") + } + + if !strings.Contains(err.Error(), "command not found") { + t.Logf("Got error (as expected): %v", err) } - originalCmd.Flags().String("test-flag", "default", "test flag") - originalCmd.PersistentFlags().String("persistent-flag", "default", "persistent flag") +} - newCmd := builder.createCommandInstance(originalCmd) +// TestNonAdminVerbBuilder_VerbNounExecution tests that verb-noun command execution works correctly +// This test specifically verifies that the command delegation doesn't cause os.Args re-parsing issues +func TestNonAdminVerbBuilder_VerbNounExecution(t *testing.T) { + // Track that the delegated command was actually executed + executionCalled := false + var receivedArgs []string - // Verify basic fields are copied - if newCmd.Use != originalCmd.Use { - t.Errorf("Expected Use to be %s, got %s", originalCmd.Use, newCmd.Use) + // Create a mock subcommand that tracks execution + mockSubCmd := &cobra.Command{ + Use: "get", + Short: "Get a backup", + RunE: func(cmd *cobra.Command, args []string) error { + executionCalled = true + receivedArgs = args + return nil + }, } - if newCmd.Short != originalCmd.Short { - t.Errorf("Expected Short to be %s, got %s", originalCmd.Short, newCmd.Short) + + mockResourceCmd := &cobra.Command{ + Use: "backup", + Short: "Work with backups", } - if newCmd.Long != originalCmd.Long { - t.Errorf("Expected Long to be %s, got %s", originalCmd.Long, newCmd.Long) + mockResourceCmd.AddCommand(mockSubCmd) + + handler := NonAdminResourceHandler{ + GetCommandFunc: func(f client.Factory) *cobra.Command { + return mockResourceCmd + }, + GetSubCommandFunc: func(cmd *cobra.Command) *cobra.Command { + return mockSubCmd + }, } - // Verify flags are copied - if newCmd.Flags().Lookup("test-flag") == nil { - t.Error("Expected test-flag to be copied to new command") + // Build the verb command + builder := NewNonAdminVerbBuilder(nil) + builder.RegisterResource("backup", handler) + + verbCmd := builder.BuildVerbCommand(NonAdminVerbConfig{ + Use: "get", + Short: "Get resources", + }) + + // Execute with verb-noun pattern: "get backup my-backup" + verbCmd.SetArgs([]string{"backup", "my-backup"}) + + err := verbCmd.Execute() + if err != nil { + // Check that the error is NOT the "unknown command" error that indicates os.Args re-parsing + if strings.Contains(err.Error(), "unknown command") { + t.Fatalf("Got 'unknown command' error, indicating os.Args re-parsing bug: %v", err) + } + // Other errors are okay (e.g., no cluster connection) + t.Logf("Got expected error (no cluster): %v", err) } - if newCmd.PersistentFlags().Lookup("persistent-flag") == nil { - t.Error("Expected persistent-flag to be copied to new command") + + // Verify the delegated command was actually called + if !executionCalled { + t.Error("Expected delegated command to be executed, but it wasn't called") } + + // Verify the arguments were passed correctly + expectedArgs := []string{"my-backup"} + if len(receivedArgs) != len(expectedArgs) { + t.Errorf("Expected %d args, got %d: %v", len(expectedArgs), len(receivedArgs), receivedArgs) + } else if len(receivedArgs) > 0 && receivedArgs[0] != expectedArgs[0] { + t.Errorf("Expected args %v, got %v", expectedArgs, receivedArgs) + } + + t.Log("Verb-noun execution test passed successfully") } -// TestNonAdminVerbBuilder_NilHandler tests handling of nil handlers -func TestNonAdminVerbBuilder_NilHandler(t *testing.T) { +// TestNonAdminVerbBuilder_VerbNounWithFlags tests verb-noun pattern with flags +func TestNonAdminVerbBuilder_VerbNounWithFlags(t *testing.T) { + executionCalled := false + var receivedFlagValue string + + mockSubCmd := &cobra.Command{ + Use: "get", + Short: "Get a backup", + RunE: func(cmd *cobra.Command, args []string) error { + executionCalled = true + flag := cmd.Flags().Lookup("output") + if flag != nil { + receivedFlagValue = flag.Value.String() + } + return nil + }, + } + mockSubCmd.Flags().String("output", "", "output format") + + mockResourceCmd := &cobra.Command{Use: "backup"} + mockResourceCmd.AddCommand(mockSubCmd) + + handler := NonAdminResourceHandler{ + GetCommandFunc: func(f client.Factory) *cobra.Command { return mockResourceCmd }, + GetSubCommandFunc: func(cmd *cobra.Command) *cobra.Command { return mockSubCmd }, + } + builder := NewNonAdminVerbBuilder(nil) + builder.RegisterResource("backup", handler) - // Register a handler that returns nil for GetCommandFunc - builder.RegisterResource("nil-resource", NonAdminResourceHandler{ - GetCommandFunc: func(f client.Factory) *cobra.Command { return nil }, - GetSubCommandFunc: func(cmd *cobra.Command) *cobra.Command { return nil }, + verbCmd := builder.BuildVerbCommand(NonAdminVerbConfig{ + Use: "get", + Short: "Get resources", }) - verbCmd := builder.BuildVerbCommand(NonAdminVerbConfig{Use: "get"}) - verbCmd.SetArgs([]string{"nil-resource", "test-name"}) + // Execute with verb-noun pattern and flags: "get backup my-backup --output=json" + verbCmd.SetArgs([]string{"backup", "my-backup", "--output=json"}) err := verbCmd.Execute() - if err == nil { - t.Error("Expected error when handler returns nil command, got nil") + if err != nil { + if strings.Contains(err.Error(), "unknown command") { + t.Fatalf("Got 'unknown command' error, indicating os.Args re-parsing bug: %v", err) + } + t.Logf("Got expected error (no cluster): %v", err) } - if !strings.Contains(err.Error(), "command not found") { - t.Logf("Got error (as expected): %v", err) + if !executionCalled { + t.Error("Expected delegated command to be executed") } + + if receivedFlagValue != "json" { + t.Errorf("Expected flag value 'json', got '%s'", receivedFlagValue) + } + + t.Log("Verb-noun with flags test passed successfully") }