From 09795245e73c00b782e8c3ac2146c3a8b0765dff Mon Sep 17 00:00:00 2001 From: Adam Zhang Date: Tue, 24 Mar 2026 10:25:01 +0800 Subject: [PATCH] switch the call order of validate/complete switch the call order of validate/complete which accomplish the same effect. Signed-off-by: Adam Zhang --- pkg/cmd/cli/install/install.go | 8 +++--- pkg/cmd/cli/install/install_test.go | 42 ++++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/cli/install/install.go b/pkg/cmd/cli/install/install.go index baaab1024..81e2df126 100644 --- a/pkg/cmd/cli/install/install.go +++ b/pkg/cmd/cli/install/install.go @@ -385,8 +385,8 @@ This is useful as a starting point for more customized installations. # velero install --provider azure --plugins velero/velero-plugin-for-microsoft-azure:v1.0.0 --bucket $BLOB_CONTAINER --secret-file ./credentials-velero --backup-location-config resourceGroup=$AZURE_BACKUP_RESOURCE_GROUP,storageAccount=$AZURE_STORAGE_ACCOUNT_ID[,subscriptionId=$AZURE_BACKUP_SUBSCRIPTION_ID] --snapshot-location-config apiTimeout=[,resourceGroup=$AZURE_BACKUP_RESOURCE_GROUP,subscriptionId=$AZURE_BACKUP_SUBSCRIPTION_ID]`, Run: func(c *cobra.Command, args []string) { - cmd.CheckError(o.Validate(c, args, f)) cmd.CheckError(o.Complete(args, f)) + cmd.CheckError(o.Validate(c, args, f)) cmd.CheckError(o.Run(c, f)) }, } @@ -570,20 +570,20 @@ func (o *Options) Validate(c *cobra.Command, args []string, f client.Factory) er } if len(o.NodeAgentConfigMap) > 0 { - if err := kubeutil.VerifyJSONConfigs(c.Context(), f.Namespace(), crClient, o.NodeAgentConfigMap, &velerotypes.NodeAgentConfigs{}); err != nil { + if err := kubeutil.VerifyJSONConfigs(c.Context(), o.Namespace, crClient, o.NodeAgentConfigMap, &velerotypes.NodeAgentConfigs{}); err != nil { return fmt.Errorf("--node-agent-configmap specified ConfigMap %s is invalid: %w", o.NodeAgentConfigMap, err) } } if len(o.RepoMaintenanceJobConfigMap) > 0 { - if err := kubeutil.VerifyJSONConfigs(c.Context(), f.Namespace(), crClient, o.RepoMaintenanceJobConfigMap, &velerotypes.JobConfigs{}); err != nil { + if err := kubeutil.VerifyJSONConfigs(c.Context(), o.Namespace, crClient, o.RepoMaintenanceJobConfigMap, &velerotypes.JobConfigs{}); err != nil { return fmt.Errorf("--repo-maintenance-job-configmap specified ConfigMap %s is invalid: %w", o.RepoMaintenanceJobConfigMap, err) } } if len(o.BackupRepoConfigMap) > 0 { config := make(map[string]any) - if err := kubeutil.VerifyJSONConfigs(c.Context(), f.Namespace(), crClient, o.BackupRepoConfigMap, &config); err != nil { + if err := kubeutil.VerifyJSONConfigs(c.Context(), o.Namespace, crClient, o.BackupRepoConfigMap, &config); err != nil { return fmt.Errorf("--backup-repository-configmap specified ConfigMap %s is invalid: %w", o.BackupRepoConfigMap, err) } } diff --git a/pkg/cmd/cli/install/install_test.go b/pkg/cmd/cli/install/install_test.go index e46d0e8db..1e0e7a4cf 100644 --- a/pkg/cmd/cli/install/install_test.go +++ b/pkg/cmd/cli/install/install_test.go @@ -121,10 +121,11 @@ func configMapInNamespace(namespace, name, jsonValue string) *corev1api.ConfigMa } // TestValidateConfigMapsUseFactoryNamespace verifies that Validate resolves the target -// namespace via f.Namespace() (not o.Namespace) for all three ConfigMap flags. +// namespace correctly for all three ConfigMap flags. // -// Before the fix, o.Namespace was "" at Validate time (Complete had not run yet), -// so VerifyJSONConfigs queried the "default" namespace instead of the intended one. +// The fix (Option B) calls Complete before Validate in NewCommand so that o.Namespace is +// populated from f.Namespace() before VerifyJSONConfigs runs. Tests mirror that order by +// calling Complete before Validate. func TestValidateConfigMapsUseFactoryNamespace(t *testing.T) { const targetNS = "tenant-b" const defaultNS = "default" @@ -132,7 +133,6 @@ func TestValidateConfigMapsUseFactoryNamespace(t *testing.T) { // Shared options that satisfy every other validation gate: // - NoDefaultBackupLocation=true + UseVolumeSnapshots=false skips provider/bucket/plugins checks // - NoSecret=true satisfies the secret-file check - // - o.Namespace is intentionally left as "" to mirror the pre-Complete state baseOptions := func() *Options { o := NewInstallOptions() o.NoDefaultBackupLocation = true @@ -215,6 +215,9 @@ func TestValidateConfigMapsUseFactoryNamespace(t *testing.T) { o := baseOptions() tc.setupOpts(o, cmName) + // Mirror the NewCommand call order: Complete populates o.Namespace before Validate runs. + require.NoError(t, o.Complete([]string{}, f)) + c := makeValidateCmd() c.SetContext(context.Background()) @@ -229,3 +232,34 @@ func TestValidateConfigMapsUseFactoryNamespace(t *testing.T) { }) } } + +// TestNewCommandRunClosureOrder covers the Run closure in NewCommand (the lines that were +// reordered by the fix: Complete → Validate → Run). +// +// The closure uses CheckError which calls os.Exit on any error, so the only safe path is one +// where all three steps return nil. DryRun=true causes o.Run to return after PrintWithFormat +// (which is a no-op when no --output flag is set) without touching any cluster clients. +func TestNewCommandRunClosureOrder(t *testing.T) { + const targetNS = "tenant-b" + const cmName = "my-config" + + cm := configMapInNamespace(targetNS, cmName, `{}`) + kbClient := velerotest.NewFakeControllerRuntimeClient(t, cm) + + f := &factorymocks.Factory{} + f.On("Namespace").Return(targetNS) + f.On("KubebuilderClient").Return(kbClient, nil) + + c := NewCommand(f) + c.SetArgs([]string{ + "--no-default-backup-location", + "--use-volume-snapshots=false", + "--no-secret", + "--dry-run", + "--node-agent-configmap", cmName, + }) + + // Execute drives the full Run closure: Complete populates o.Namespace, Validate + // looks up the ConfigMap in targetNS (succeeds), Run returns early via DryRun. + require.NoError(t, c.Execute()) +}