Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[refactor][kubectl-plugin] simplify cobra Validate() tests #3040

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[refactor][kubectl-plugin] simplify cobra Validate() tests #3040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions kubectl-plugin/pkg/cmd/create/create_cluster.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading