feat: update kubeconfig generate command (#1158)

* feat: update kubeconfig generate command

* improve messages and docs

cli/cmds/kubeconfig.go

@@ -1,6 +1,8 @@
 package cmds
 
 import (
+	"errors"
+	"fmt"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -49,12 +51,15 @@ func NewKubeconfigGenerateCmd(appCtx *AppContext) *cobra.Command {
 	cfg := &GenerateKubeconfigConfig{}
 
 	cmd := &cobra.Command{
-		Use:   "generate",
-		Short: "Generate kubeconfig for clusters.",
-		RunE:  generate(appCtx, cfg),
-		Args:  cobra.NoArgs,
+		Use:     "generate",
+		Short:   "Generate kubeconfig for clusters.",
+		Example: "k3kcli kubeconfig generate [command options] NAME",
+		RunE:    generate(appCtx, cfg),
+		Args:    cobra.MaximumNArgs(1),
 	}
 
+	cmd.ValidArgsFunction = completeClusterNames
+
 	CobraFlagNamespace(appCtx, cmd, completeClusterNamespaces)
 
 	generateKubeconfigFlags(cmd, cfg)
@@ -64,6 +69,11 @@ func NewKubeconfigGenerateCmd(appCtx *AppContext) *cobra.Command {
 
 func generateKubeconfigFlags(cmd *cobra.Command, cfg *GenerateKubeconfigConfig) {
 	cmd.Flags().StringVar(&cfg.name, "name", "", "cluster name")
+
+	if err := cmd.Flags().MarkDeprecated("name", "it will be removed in a future release, use the NAME argument instead"); err != nil {
+		logrus.Fatal(err)
+	}
+
 	cmd.Flags().StringVar(&cfg.configName, "config-name", "", "the name of the generated kubeconfig file")
 	cmd.Flags().StringVar(&cfg.cn, "cn", controller.AdminCommonName, "Common name (CN) of the generated certificates for the kubeconfig")
 	cmd.Flags().StringSliceVar(&cfg.org, "org", nil, "Organization name (ORG) of the generated certificates for the kubeconfig")
@@ -77,14 +87,38 @@ func generate(appCtx *AppContext, cfg *GenerateKubeconfigConfig) func(cmd *cobra
 		ctx := cmd.Context()
 		client := appCtx.Client
 
+		name, namespace := cfg.name, appCtx.Namespace(cfg.name)
+
+		switch {
+		case len(args) == 1:
+			if cfg.name != "" {
+				logrus.Warnf("the --name flag is deprecated and will be removed in a future release, ignoring it in favor of the '%s' argument", args[0])
+			}
+
+			var err error
+			if namespace, name, err = resolveClusterArg(appCtx, args[0]); err != nil {
+				return err
+			}
+
+		case name != "":
+			logrus.Warn("the --name flag is deprecated and will be removed in a future release, use the NAME argument instead")
+
+		default:
+			return errors.New("expected exactly one cluster name")
+		}
+
 		clusterKey := types.NamespacedName{
-			Name:      cfg.name,
-			Namespace: appCtx.Namespace(cfg.name),
+			Name:      name,
+			Namespace: namespace,
 		}
 
 		var cluster v1beta1.Cluster
 
 		if err := client.Get(ctx, clusterKey, &cluster); err != nil {
+			if apierrors.IsNotFound(err) {
+				return fmt.Errorf("cluster %q not found in namespace %q", name, namespace)
+			}
+
 			return err
 		}
 

cli/cmds/kubeconfig_test.go

@@ -0,0 +1,111 @@
+package cmds
+
+import (
+	"testing"
+
+	"github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus/hooks/test"
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	"github.com/rancher/k3k/pkg/apis/k3k.io/v1beta1"
+)
+
+// Test_generate_clusterKey checks how the cluster name and namespace are resolved from the
+// NAME argument and the deprecated --name flag. The cluster is always missing, so the
+// "not found" error reports the resolved key without going through the kubeconfig generation.
+func Test_generate_clusterKey(t *testing.T) {
+	tests := []struct {
+		name          string
+		flagNamespace string
+		flagName      string
+		args          []string
+		wantErr       string
+		wantWarn      string
+	}{
+		{
+			name:    "bare name argument defaults to k3k-<name>",
+			args:    []string{"mycluster"},
+			wantErr: `cluster "mycluster" not found in namespace "k3k-mycluster"`,
+		},
+		{
+			name:    "namespace/name argument is split",
+			args:    []string{"myns/mycluster"},
+			wantErr: `cluster "mycluster" not found in namespace "myns"`,
+		},
+		{
+			name:          "bare name argument respects the namespace flag",
+			flagNamespace: "myns",
+			args:          []string{"mycluster"},
+			wantErr:       `cluster "mycluster" not found in namespace "myns"`,
+		},
+		{
+			name:          "namespace/name argument conflicting with the namespace flag errors",
+			flagNamespace: "otherns",
+			args:          []string{"myns/mycluster"},
+			wantErr:       `namespace mismatch: flag --namespace "otherns" conflicts with argument namespace "myns"`,
+		},
+		{
+			name:     "deprecated name flag is still supported",
+			flagName: "mycluster",
+			wantErr:  `cluster "mycluster" not found in namespace "k3k-mycluster"`,
+			wantWarn: "the --name flag is deprecated and will be removed in a future release, use the NAME argument instead",
+		},
+		{
+			name:          "deprecated name flag respects the namespace flag",
+			flagNamespace: "myns",
+			flagName:      "mycluster",
+			wantErr:       `cluster "mycluster" not found in namespace "myns"`,
+			wantWarn:      "the --name flag is deprecated and will be removed in a future release, use the NAME argument instead",
+		},
+		{
+			name:     "the argument wins over the deprecated name flag",
+			flagName: "ignored",
+			args:     []string{"myns/mycluster"},
+			wantErr:  `cluster "mycluster" not found in namespace "myns"`,
+			wantWarn: "the --name flag is deprecated and will be removed in a future release, ignoring it in favor of the 'myns/mycluster' argument",
+		},
+		{
+			name:    "no cluster name at all errors",
+			wantErr: "expected exactly one cluster name",
+		},
+	}
+
+	scheme := runtime.NewScheme()
+	require.NoError(t, v1beta1.AddToScheme(scheme))
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			logs := test.NewGlobal()
+			defer logs.Reset()
+
+			appCtx := &AppContext{
+				Client:    fake.NewClientBuilder().WithScheme(scheme).Build(),
+				namespace: tt.flagNamespace,
+			}
+			cfg := &GenerateKubeconfigConfig{name: tt.flagName}
+
+			err := generate(appCtx, cfg)(&cobra.Command{}, tt.args)
+
+			assert.EqualError(t, err, tt.wantErr)
+
+			var warnings []string
+
+			for _, entry := range logs.AllEntries() {
+				if entry.Level == logrus.WarnLevel {
+					warnings = append(warnings, entry.Message)
+				}
+			}
+
+			if tt.wantWarn == "" {
+				assert.Empty(t, warnings)
+				return
+			}
+
+			assert.Equal(t, []string{tt.wantWarn}, warnings)
+		})
+	}
+}