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

Jump to bottom

use genericclioptions package to handle kubeconfig #909

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

use genericclioptions package to handle kubeconfig #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5b4ced
use genericclioptions package to handle kubeconfig
bewing May 13, 2024
3b3f94d
use genericclioptions package to handle kubeconfig
bewing May 14, 2024
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
13 changes: 8 additions & 5 deletions cmd/directpv/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/minio/directpv/pkg/installer"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/klog/v2"
)

Expand All @@ -46,13 +47,13 @@ var (
zone = "default"
region = "default"
csiEndpoint = installer.UnixCSIEndpoint
kubeconfig = ""
conversionHealthzURL = ""
readinessPort = consts.ReadinessPort

nodeID directpvtypes.NodeID
nodeID directpvtypes.NodeID
)

var genericOptions *genericclioptions.ConfigFlags

var mainCmd = &cobra.Command{
Use: consts.AppName,
Short: "Start " + consts.AppPrettyName + " controller and driver. This binary is usually executed by Kubernetes.",
Expand All @@ -72,7 +73,7 @@ var mainCmd = &cobra.Command{

nodeID = directpvtypes.NodeID(kubeNodeName)

client.Init()
client.Init(genericOptions)
return nil
},
}
Expand All @@ -94,7 +95,9 @@ func init() {
flag.Set("logtostderr", "true")
flag.Set("alsologtostderr", "true")

mainCmd.PersistentFlags().StringVarP(&kubeconfig, "kubeconfig", "k", kubeconfig, "Path to the kubeconfig file to use for Kubernetes requests.")
genericOptions = genericclioptions.NewConfigFlags(true)
genericOptions.AddFlags(mainCmd.PersistentFlags())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not going to work. We use flags which colliding with genericcliflags. In fact we don't need support flags like kubectl.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use flags which colliding with genericcliflags

Then use the flags from the flagset where they collide? I'm happy to work with you to identify where this is and see if it's trivial to access it via the flagset. I'm not sure if viper is able to access it as I haven't tested

In fact we don't need support flags like kubectl.

As referenced by #884, your tool does not work for those that are working with multiple kubeconfig files very easily. The flagset and its associated loaders provide a much more robust cluster and namespace selection, rather than having to juggle multiple kubeconfig directories.

Copy link
Author

@bewing bewing May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use flags which colliding with genericcliflags

Apologies -- looking at the test output, it's not that you're using the same flags, there's reused shortnames. That is more of an issue.

It may be possible to rebuild just enough of the RESTClientGetter interface to allow for cluster selection, without conflicting flags.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The right way to fix the issue is to accept rest.Config from the user. However this is not a priority now.


mainCmd.PersistentFlags().StringVar(&identity, "identity", identity, "Identity of "+consts.AppPrettyName+" instances")
mainCmd.PersistentFlags().StringVar(&csiEndpoint, "csi-endpoint", csiEndpoint, "CSI endpoint")
mainCmd.PersistentFlags().StringVar(&kubeNodeName, "kube-node-name", kubeNodeName, "Kubernetes node name (MUST BE SET)")
Expand Down
1 change: 0 additions & 1 deletion cmd/kubectl-directpv/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ var volumeStatusValues = []string{
}

var (
kubeconfig string // --kubeconfig flag
quietFlag bool // --quiet flag
outputFormat string // --output flag
noHeaders bool // --no-headers flag
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubectl-directpv/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ func getLegacyFlag(ctx context.Context) bool {
return true
}

legacyclient.Init()
legacyclient.Init(genericOptions)

for result := range legacyclient.ListVolumes(ctx) {
if result.Err != nil {
Expand Down
18 changes: 9 additions & 9 deletions cmd/kubectl-directpv/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,18 @@ import (
"github.com/minio/directpv/pkg/utils"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/klog/v2"
)

// Version of this application populated by `go build`
// e.g. $ go build -ldflags="-X main.Version=v4.0.1"
var Version string

var disableInit bool
var (
genericOptions *genericclioptions.ConfigFlags
disableInit bool
)

var mainCmd = &cobra.Command{
Use: consts.AppName,
Expand All @@ -48,7 +52,7 @@ var mainCmd = &cobra.Command{
Version: Version,
PersistentPreRunE: func(_ *cobra.Command, _ []string) error {
if !disableInit {
client.Init()
client.Init(genericOptions)
}
return nil
},
Expand Down Expand Up @@ -106,13 +110,6 @@ Use "{{.CommandPath}} [command] --help" for more information about this command.
flag.Set("logtostderr", "true")
flag.Set("alsologtostderr", "true")

mainCmd.PersistentFlags().StringVarP(
&kubeconfig,
"kubeconfig",
"",
kubeconfig,
"Path to the kubeconfig file to use for CLI requests",
)
mainCmd.PersistentFlags().BoolVarP(
&quietFlag,
"quiet",
Expand All @@ -121,6 +118,9 @@ Use "{{.CommandPath}} [command] --help" for more information about this command.
"Suppress printing error messages",
)

genericOptions = genericclioptions.NewConfigFlags(true)
genericOptions.AddFlags(mainCmd.PersistentFlags())

mainCmd.PersistentFlags().MarkHidden("alsologtostderr")
mainCmd.PersistentFlags().MarkHidden("add_dir_header")
mainCmd.PersistentFlags().MarkHidden("log_file")
Expand Down
5 changes: 3 additions & 2 deletions cmd/kubectl-directpv/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@ func init() {

func migrateMain(ctx context.Context) {
if err := installer.Migrate(ctx, &installer.Args{
Quiet: quietFlag,
Legacy: true,
Quiet: quietFlag,
Legacy: true,
GenericOptions: genericOptions,
}, false); err != nil {
utils.Eprintf(quietFlag, true, "migration failed; %v", err)
os.Exit(1)
Expand Down
12 changes: 12 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ require (
k8s.io/api v0.28.7
k8s.io/apiextensions-apiserver v0.28.7
k8s.io/apimachinery v0.28.7
k8s.io/cli-runtime v0.28.7
k8s.io/client-go v0.28.7
k8s.io/klog/v2 v2.120.1
k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340
Expand All @@ -42,22 +43,27 @@ require (
github.com/emicklei/go-restful/v3 v3.12.0 // indirect
github.com/evanphx/json-patch v5.9.0+incompatible // indirect
github.com/fsnotify/fsnotify v1.7.0 // indirect
github.com/go-errors/errors v1.4.2 // indirect
github.com/go-logr/logr v1.4.1 // indirect
github.com/go-openapi/jsonpointer v0.21.0 // indirect
github.com/go-openapi/jsonreference v0.21.0 // indirect
github.com/go-openapi/swag v0.23.0 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/golang/protobuf v1.5.4 // indirect
github.com/google/btree v1.0.1 // indirect
github.com/google/gnostic-models v0.6.9-0.20230804172637-c7be7c783f49 // indirect
github.com/google/go-cmp v0.6.0 // indirect
github.com/google/gofuzz v1.2.0 // indirect
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect
github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7 // indirect
github.com/hashicorp/hcl v1.0.0 // indirect
github.com/imdario/mergo v0.3.16 // indirect
github.com/inconshreveable/mousetrap v1.1.0 // indirect
github.com/josharian/intern v1.0.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/klauspost/cpuid/v2 v2.2.7 // indirect
github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de // indirect
github.com/lucasb-eyer/go-colorful v1.2.0 // indirect
github.com/magiconair/properties v1.8.7 // indirect
github.com/mailru/easyjson v0.7.7 // indirect
Expand All @@ -68,12 +74,14 @@ require (
github.com/mitchellh/mapstructure v1.5.0 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 // indirect
github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect
github.com/muesli/cancelreader v0.2.2 // indirect
github.com/muesli/reflow v0.3.0 // indirect
github.com/muesli/termenv v0.15.2 // indirect
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/pelletier/go-toml/v2 v2.1.1 // indirect
github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
github.com/pkg/errors v0.9.1 // indirect
github.com/prometheus/common v0.50.0 // indirect
github.com/prometheus/procfs v0.13.0 // indirect
Expand All @@ -85,6 +93,8 @@ require (
github.com/spf13/cast v1.6.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/subosito/gotenv v1.6.0 // indirect
github.com/xlab/treeprint v1.2.0 // indirect
go.starlark.net v0.0.0-20230525235612-a134d8f9ddca // indirect
go.uber.org/multierr v1.11.0 // indirect
golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 // indirect
golang.org/x/net v0.23.0 // indirect
Expand All @@ -102,5 +112,7 @@ require (
k8s.io/component-base v0.28.7 // indirect
k8s.io/utils v0.0.0-20240310230437-4693a0247e57 // indirect
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
sigs.k8s.io/kustomize/api v0.13.5-0.20230601165947-6ce0bf390ce3 // indirect
sigs.k8s.io/kustomize/kyaml v0.14.3-0.20230601165947-6ce0bf390ce3 // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
)
Loading
Loading