Skip to content

Commit

Permalink
[refactor][kubectl-plugin] simplify cobra Validate() tests
Browse files Browse the repository at this point in the history 
We were duplicating logic in each command's `Validate()` test.

1. Abstract away K8s context detection logic with a new interface `KubeContexter`.
1. Consolidate tests for this logic in a separate test for `DefaultKubeContexter`.
1. Mock `KubeContexter` in `Validate()` tests with `MockKubeContexter` to
   simplify the `Validate()` tests.

Signed-off-by: David Xia <[email protected]>
  • Loading branch information
@davidxia
davidxia committed Feb 12, 2025
1 parent 39e8028 commit aa85751
Show file tree
Hide file tree
Showing 16 changed files with 217 additions and 443 deletions.
8 changes: 5 additions & 3 deletions kubectl-plugin/pkg/cmd/create/create_cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
type CreateClusterOptions struct {
configFlags *genericclioptions.ConfigFlags
ioStreams *genericclioptions.IOStreams
kubeContexter util.KubeContexter
clusterName string
rayVersion string
image string
Expand Down Expand Up @@ -47,8 +48,9 @@ var (

func NewCreateClusterOptions(streams genericclioptions.IOStreams) *CreateClusterOptions {
return &CreateClusterOptions{
configFlags: genericclioptions.NewConfigFlags(true),
ioStreams: &streams,
configFlags: genericclioptions.NewConfigFlags(true),
ioStreams: &streams,
kubeContexter: &util.DefaultKubeContexter{},
}
}

Expand Down Expand Up @@ -110,7 +112,7 @@ func (options *CreateClusterOptions) Validate() error {
if err != nil {
return fmt.Errorf("Error retrieving raw config: %w", err)
}
if !util.HasKubectlContext(config, options.configFlags) {
if !options.kubeContexter.HasContext(config, options.configFlags) {
return fmt.Errorf("no context is currently set, use %q or %q to select a new one", "--context", "kubectl config use-context <context>")
}

Expand Down
68 changes: 6 additions & 62 deletions kubectl-plugin/pkg/cmd/create/create_cluster_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,80 +23,24 @@ func TestRayCreateClusterComplete(t *testing.T) {
}

func TestRayCreateClusterValidate(t *testing.T) {
testStreams, _, _, _ := genericclioptions.NewTestIOStreams()
testNS, testContext, testBT, testImpersonate := "test-namespace", "test-context", "test-bearer-token", "test-person"

kubeConfigWithCurrentContext, err := util.CreateTempKubeConfigFile(t, testContext)
require.NoError(t, err)

kubeConfigWithoutCurrentContext, err := util.CreateTempKubeConfigFile(t, "")
require.NoError(t, err)

tests := []struct {
name string
opts *CreateClusterOptions
expectError string
}{
{
name: "Test validation when no context is set",
name: "should error when no K8s context is set",
opts: &CreateClusterOptions{
configFlags: &genericclioptions.ConfigFlags{
KubeConfig: &kubeConfigWithoutCurrentContext,
},
ioStreams: &testStreams,
configFlags: genericclioptions.NewConfigFlags(true),
kubeContexter: util.NewMockKubeContexter(false),
},
expectError: "no context is currently set, use \"--context\" or \"kubectl config use-context <context>\" to select a new one",
},
{
name: "no error when kubeconfig has current context and --context switch isn't set",
opts: &CreateClusterOptions{
configFlags: &genericclioptions.ConfigFlags{
KubeConfig: &kubeConfigWithCurrentContext,
},
ioStreams: &testStreams,
},
},
{
name: "no error when kubeconfig has no current context and --context switch is set",
opts: &CreateClusterOptions{
configFlags: &genericclioptions.ConfigFlags{
KubeConfig: &kubeConfigWithoutCurrentContext,
Context: &testContext,
},
ioStreams: &testStreams,
},
},
{
name: "no error when kubeconfig has current context and --context switch is set",
opts: &CreateClusterOptions{
configFlags: &genericclioptions.ConfigFlags{
KubeConfig: &kubeConfigWithCurrentContext,
Context: &testContext,
},
ioStreams: &testStreams,
},
},
{
name: "Successful submit job validation with RayJob",
name: "should not error when K8s context is set",
opts: &CreateClusterOptions{
configFlags: &genericclioptions.ConfigFlags{
Namespace: &testNS,
Context: &testContext,
KubeConfig: &kubeConfigWithCurrentContext,
BearerToken: &testBT,
Impersonate: &testImpersonate,
ImpersonateGroup: &[]string{"fake-group"},
},
ioStreams: &testStreams,
clusterName: "fakeclustername",
rayVersion: "ray-version",
image: "ray-image",
headCPU: "5",
headGPU: "1",
headMemory: "5Gi",
workerReplicas: 3,
workerCPU: "4",
workerMemory: "5Gi",
configFlags: genericclioptions.NewConfigFlags(true),
kubeContexter: util.NewMockKubeContexter(true),
},
},
}
Expand Down
8 changes: 5 additions & 3 deletions kubectl-plugin/pkg/cmd/create/create_workergroup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const (
type CreateWorkerGroupOptions struct {
configFlags *genericclioptions.ConfigFlags
ioStreams *genericclioptions.IOStreams
kubeContexter util.KubeContexter
clusterName string
groupName string
rayVersion string
Expand Down Expand Up @@ -55,8 +56,9 @@ var (

func NewCreateWorkerGroupOptions(streams genericclioptions.IOStreams) *CreateWorkerGroupOptions {
return &CreateWorkerGroupOptions{
configFlags: genericclioptions.NewConfigFlags(true),
ioStreams: &streams,
configFlags: genericclioptions.NewConfigFlags(true),
ioStreams: &streams,
kubeContexter: &util.DefaultKubeContexter{},
}
}

Expand Down Expand Up @@ -121,7 +123,7 @@ func (options *CreateWorkerGroupOptions) Validate() error {
if err != nil {
return fmt.Errorf("Error retrieving raw config: %w", err)
}
if !util.HasKubectlContext(config, options.configFlags) {
if !options.kubeContexter.HasContext(config, options.configFlags) {
return fmt.Errorf("no context is currently set, use %q or %q to select a new one", "--context", "kubectl config use-context <context>")
}

Expand Down
18 changes: 10 additions & 8 deletions kubectl-plugin/pkg/cmd/delete/delete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ import (
)

type DeleteOptions struct {
configFlags *genericclioptions.ConfigFlags
ioStreams *genericiooptions.IOStreams
ResourceType util.ResourceType
ResourceName string
Namespace string
configFlags *genericclioptions.ConfigFlags
ioStreams *genericiooptions.IOStreams
kubeContexter util.KubeContexter
ResourceType util.ResourceType
ResourceName string
Namespace string
}

var deleteExample = templates.Examples(`
Expand All @@ -44,8 +45,9 @@ var deleteExample = templates.Examples(`
func NewDeleteOptions(streams genericiooptions.IOStreams) *DeleteOptions {
configFlags := genericclioptions.NewConfigFlags(true)
return &DeleteOptions{
ioStreams: &streams,
configFlags: configFlags,
ioStreams: &streams,
configFlags: configFlags,
kubeContexter: &util.DefaultKubeContexter{},
}
}

Expand Down Expand Up @@ -118,7 +120,7 @@ func (options *DeleteOptions) Validate() error {
if err != nil {
return fmt.Errorf("Error retrieving raw config: %w", err)
}
if !util.HasKubectlContext(config, options.configFlags) {
if !options.kubeContexter.HasContext(config, options.configFlags) {
return fmt.Errorf("no context is currently set, use %q or %q to select a new one", "--context", "kubectl config use-context <context>")
}
return nil
Expand Down
63 changes: 6 additions & 57 deletions kubectl-plugin/pkg/cmd/delete/delete_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,75 +113,24 @@ func TestComplete(t *testing.T) {
}

func TestRayDeleteValidate(t *testing.T) {
testStreams, _, _, _ := genericclioptions.NewTestIOStreams()

testNS, testContext, testBT, testImpersonate := "test-namespace", "test-context", "test-bearer-token", "test-person"

kubeConfigWithCurrentContext, err := util.CreateTempKubeConfigFile(t, testContext)
require.NoError(t, err)

kubeConfigWithoutCurrentContext, err := util.CreateTempKubeConfigFile(t, "")
require.NoError(t, err)

tests := []struct {
name string
opts *DeleteOptions
expectError string
}{
{
name: "Test validation when no context is set",
name: "should error when no K8s context is set",
opts: &DeleteOptions{
configFlags: &genericclioptions.ConfigFlags{
KubeConfig: &kubeConfigWithoutCurrentContext,
},
ioStreams: &testStreams,
configFlags: genericclioptions.NewConfigFlags(true),
kubeContexter: util.NewMockKubeContexter(false),
},
expectError: "no context is currently set, use \"--context\" or \"kubectl config use-context <context>\" to select a new one",
},
{
name: "no error when kubeconfig has current context and --context switch isn't set",
opts: &DeleteOptions{
configFlags: &genericclioptions.ConfigFlags{
KubeConfig: &kubeConfigWithCurrentContext,
},
ioStreams: &testStreams,
},
},
{
name: "no error when kubeconfig has no current context and --context switch is set",
opts: &DeleteOptions{
configFlags: &genericclioptions.ConfigFlags{
KubeConfig: &kubeConfigWithoutCurrentContext,
Context: &testContext,
},
ioStreams: &testStreams,
},
},
{
name: "no error when kubeconfig has current context and --context switch is set",
opts: &DeleteOptions{
configFlags: &genericclioptions.ConfigFlags{
KubeConfig: &kubeConfigWithCurrentContext,
Context: &testContext,
},
ioStreams: &testStreams,
},
},
{
name: "Successful submit job validation with RayJob",
name: "should not error when K8s context is set",
opts: &DeleteOptions{
configFlags: &genericclioptions.ConfigFlags{
Namespace: &testNS,
Context: &testContext,
KubeConfig: &kubeConfigWithCurrentContext,
BearerToken: &testBT,
Impersonate: &testImpersonate,
ImpersonateGroup: &[]string{"fake-group"},
},
ioStreams: &testStreams,
ResourceType: util.RayJob,
ResourceName: "test-rayjob",
Namespace: testNS,
configFlags: genericclioptions.NewConfigFlags(true),
kubeContexter: util.NewMockKubeContexter(true),
},
},
}
Expand Down
18 changes: 10 additions & 8 deletions kubectl-plugin/pkg/cmd/get/get_cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,16 @@ import (
type GetClusterOptions struct {
configFlags *genericclioptions.ConfigFlags
ioStreams *genericclioptions.IOStreams
kubeContexter util.KubeContexter
args []string
AllNamespaces bool
allNamespaces bool
}

func NewGetClusterOptions(streams genericclioptions.IOStreams) *GetClusterOptions {
return &GetClusterOptions{
configFlags: genericclioptions.NewConfigFlags(true),
ioStreams: &streams,
configFlags: genericclioptions.NewConfigFlags(true),
ioStreams: &streams,
kubeContexter: &util.DefaultKubeContexter{},
}
}

Expand Down Expand Up @@ -59,14 +61,14 @@ func NewGetClusterCommand(streams genericclioptions.IOStreams) *cobra.Command {
return options.Run(cmd.Context(), k8sClient)
},
}
cmd.Flags().BoolVarP(&options.AllNamespaces, "all-namespaces", "A", options.AllNamespaces, "If present, list the requested clusters across all namespaces. Namespace in current context is ignored even if specified with --namespace.")
cmd.Flags().BoolVarP(&options.allNamespaces, "all-namespaces", "A", options.allNamespaces, "If present, list the requested clusters across all namespaces. Namespace in current context is ignored even if specified with --namespace.")
options.configFlags.AddFlags(cmd.Flags())
return cmd
}

func (options *GetClusterOptions) Complete(args []string) error {
if *options.configFlags.Namespace == "" {
options.AllNamespaces = true
options.allNamespaces = true
}

options.args = args
Expand All @@ -79,7 +81,7 @@ func (options *GetClusterOptions) Validate() error {
if err != nil {
return fmt.Errorf("Error retrieving raw config: %w", err)
}
if !util.HasKubectlContext(config, options.configFlags) {
if !options.kubeContexter.HasContext(config, options.configFlags) {
return fmt.Errorf("no context is currently set, use %q or %q to select a new one", "--context", "kubectl config use-context <context>")
}
if len(options.args) > 1 {
Expand Down Expand Up @@ -108,7 +110,7 @@ func getRayClusters(ctx context.Context, options *GetClusterOptions, k8sClient c
}
}

if options.AllNamespaces {
if options.allNamespaces {
rayclusterList, err = k8sClient.RayClient().RayV1().RayClusters("").List(ctx, listopts)
if err != nil {
return nil, fmt.Errorf("unable to retrieve raycluster for all namespaces: %w", err)
Expand All @@ -122,7 +124,7 @@ func getRayClusters(ctx context.Context, options *GetClusterOptions, k8sClient c

if len(options.args) == 1 && len(rayclusterList.Items) == 0 {
errMsg := fmt.Sprintf("Ray cluster %s not found", options.args[0])
if options.AllNamespaces {
if options.allNamespaces {
errMsg += " in any namespace"
} else {
errMsg += fmt.Sprintf(" in namespace %s", *options.configFlags.Namespace)
Expand Down
Loading

0 comments on commit aa85751

Please sign in to comment.