From 507fbad069b78636f4a3edf7450c1e7af1c3c0d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Wed, 11 Oct 2023 18:04:11 +0300 Subject: [PATCH] Add shortname ambiguity warning in shortcut expander (#117668) * Add warning handler callback function in shortcut expander Currently, errors in client-go are propagated back to the callers via function returns. However, there is no elegant way for just warning users. For example, when user wants to get a resource with it's short name format and if there are multiple resources belonging to this short name, we need to warn user about this ambugity which one is picked and which ones are discarded. Not only to overcome this particular case mentioned above, but also propose a way for the possible warnings in the future, this commit adds a warningHandler callback function in shortcutExpander. * Add warningPrinter functionality in ConfigFlags ConfigFlags has neither warning user in a standardized format functionality nor passing warning callback functions to other upper level libraries such as client-go. This commit adds an ability that user can set warningPrinters according to their IOStreams and this warningPrinters will be used to raise possible warnings happening not only in cli-runtime but also in client-go. * Pass warning callback function in ConfigFlags to shortcutExpander This commit passes warning callback function to print possible warnings happened in shortcut expander to warn user in a standardized format. * Add integration test for CRDs having ambiguous short names This commit adds integration test to assure that warning message related to this ambiguity is printed when resources are being retrieved via their short name representations in cases where multiple resources have same short names. This integration test also ensures that the logic behind which resource will be selected hasn't been changed which may cause disperancies in clusters. * Remove defaultConfigFlag global variable * Move default config flags initialization into function * Skip warning for versions of same group/resource * Run update-vendor * Warn only once when there are multiple versions registered for ambiguous resource * Apply gocritic review * Add multi-resource multi-version ambiguity unit test Kubernetes-commit: a504aed54d028dbc8ea2508142c94d309f5f1ec6 --- go.mod | 8 ++++---- go.sum | 8 ++++---- pkg/cmd/cmd.go | 11 +++++++---- pkg/cmd/testing/fake.go | 2 +- pkg/util/term/term.go | 40 +++++++++------------------------------- 5 files changed, 25 insertions(+), 44 deletions(-) diff --git a/go.mod b/go.mod index 7c9f941f0..b37cef3ce 100644 --- a/go.mod +++ b/go.mod @@ -32,8 +32,8 @@ require ( gopkg.in/yaml.v2 v2.4.0 k8s.io/api v0.0.0-20231011070906-133964c1133a k8s.io/apimachinery v0.0.0-20231011070637-1ca6c696c8e4 - k8s.io/cli-runtime v0.0.0-20231011074005-3d1197d7c1a6 - k8s.io/client-go v0.0.0-20231011071243-cf4002bbbf85 + k8s.io/cli-runtime v0.0.0-20231011193854-a96869e0c2be + k8s.io/client-go v0.0.0-20231011150411-57d597c1d633 k8s.io/component-base v0.0.0-20231011071914-d2d2799059b6 k8s.io/component-helpers v0.0.0-20231011072036-2cfdfb492663 k8s.io/klog/v2 v2.100.1 @@ -98,8 +98,8 @@ require ( replace ( k8s.io/api => k8s.io/api v0.0.0-20231011070906-133964c1133a k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20231011070637-1ca6c696c8e4 - k8s.io/cli-runtime => k8s.io/cli-runtime v0.0.0-20231011074005-3d1197d7c1a6 - k8s.io/client-go => k8s.io/client-go v0.0.0-20231011071243-cf4002bbbf85 + k8s.io/cli-runtime => k8s.io/cli-runtime v0.0.0-20231011193854-a96869e0c2be + k8s.io/client-go => k8s.io/client-go v0.0.0-20231011150411-57d597c1d633 k8s.io/code-generator => k8s.io/code-generator v0.0.0-20231011070418-d14db18ca33c k8s.io/component-base => k8s.io/component-base v0.0.0-20231011071914-d2d2799059b6 k8s.io/component-helpers => k8s.io/component-helpers v0.0.0-20231011072036-2cfdfb492663 diff --git a/go.sum b/go.sum index d3e0bb0e3..845a95311 100644 --- a/go.sum +++ b/go.sum @@ -280,10 +280,10 @@ k8s.io/api v0.0.0-20231011070906-133964c1133a h1:ewt1Wxw8kmOLVJfAatcAQhH1e8p/RrQ k8s.io/api v0.0.0-20231011070906-133964c1133a/go.mod h1:fu7/5TpQZY/iFHh5+Ss8NiQgwuMNFajKRNuU00g+a6U= k8s.io/apimachinery v0.0.0-20231011070637-1ca6c696c8e4 h1:B+3yeqV++bsLepyIyjsMGN4EJOxwykKC/vT5aGQjYjk= k8s.io/apimachinery v0.0.0-20231011070637-1ca6c696c8e4/go.mod h1:CaWZ0SJfeNC59+1gyc+rnythykg+OHqDPEGPQmVnTiE= -k8s.io/cli-runtime v0.0.0-20231011074005-3d1197d7c1a6 h1:pZqlZKHpNXXuuXKxFvM5ehihS5TAuKqaZU90a5kmsT0= -k8s.io/cli-runtime v0.0.0-20231011074005-3d1197d7c1a6/go.mod h1:i6RamxpYFpzIDv22wjLh3KtZ5kCCrx9XHG6WwaEJRsI= -k8s.io/client-go v0.0.0-20231011071243-cf4002bbbf85 h1:PeBqhcj2ZCn4f0IQVkxdE/4x2ySX2nE5ljkyOS1PmmU= -k8s.io/client-go v0.0.0-20231011071243-cf4002bbbf85/go.mod h1:4zPB+NUOQERJBGKdq7X91PGpvAI/jiK0NZEmq5c8wx4= +k8s.io/cli-runtime v0.0.0-20231011193854-a96869e0c2be h1:8UBiMQn87pMU6yoFM/WXkO7R/m5IJQvQOfiNmfc0H0M= +k8s.io/cli-runtime v0.0.0-20231011193854-a96869e0c2be/go.mod h1:L/ZADS+dmmZguNL+w4PdIjg/ytMzAu3+1qDxy9cL7GY= +k8s.io/client-go v0.0.0-20231011150411-57d597c1d633 h1:r21mAcOlMrjKXvltpLFxAmQpr5zVCmX0CGfuC1oX070= +k8s.io/client-go v0.0.0-20231011150411-57d597c1d633/go.mod h1:4zPB+NUOQERJBGKdq7X91PGpvAI/jiK0NZEmq5c8wx4= k8s.io/component-base v0.0.0-20231011071914-d2d2799059b6 h1:DaLh50HBfqyagz99VKlApWGGTQjwTZYneZh309zm40M= k8s.io/component-base v0.0.0-20231011071914-d2d2799059b6/go.mod h1:4eHXzsEQqrbZP52drXln0tPwtwp0YyajVy/n8vp6VGI= k8s.io/component-helpers v0.0.0-20231011072036-2cfdfb492663 h1:I5tqhXbxypHvWjTc2oDErCjA1B/vvmLwRtjFRuizOVI= diff --git a/pkg/cmd/cmd.go b/pkg/cmd/cmd.go index 6b3a84d50..cc554dea1 100644 --- a/pkg/cmd/cmd.go +++ b/pkg/cmd/cmd.go @@ -92,15 +92,18 @@ type KubectlOptions struct { genericiooptions.IOStreams } -var defaultConfigFlags = genericclioptions.NewConfigFlags(true).WithDeprecatedPasswordFlag().WithDiscoveryBurst(300).WithDiscoveryQPS(50.0) +func defaultConfigFlags() *genericclioptions.ConfigFlags { + return genericclioptions.NewConfigFlags(true).WithDeprecatedPasswordFlag().WithDiscoveryBurst(300).WithDiscoveryQPS(50.0) +} // NewDefaultKubectlCommand creates the `kubectl` command with default arguments func NewDefaultKubectlCommand() *cobra.Command { + ioStreams := genericiooptions.IOStreams{In: os.Stdin, Out: os.Stdout, ErrOut: os.Stderr} return NewDefaultKubectlCommandWithArgs(KubectlOptions{ PluginHandler: NewDefaultPluginHandler(plugin.ValidPluginFilenamePrefixes), Arguments: os.Args, - ConfigFlags: defaultConfigFlags, - IOStreams: genericiooptions.IOStreams{In: os.Stdin, Out: os.Stdout, ErrOut: os.Stderr}, + ConfigFlags: defaultConfigFlags().WithWarningPrinter(ioStreams), + IOStreams: ioStreams, }) } @@ -364,7 +367,7 @@ func NewKubectlCommand(o KubectlOptions) *cobra.Command { kubeConfigFlags := o.ConfigFlags if kubeConfigFlags == nil { - kubeConfigFlags = defaultConfigFlags + kubeConfigFlags = defaultConfigFlags().WithWarningPrinter(o.IOStreams) } kubeConfigFlags.AddFlags(flags) matchVersionKubeConfigFlags := cmdutil.NewMatchVersionFlags(kubeConfigFlags) diff --git a/pkg/cmd/testing/fake.go b/pkg/cmd/testing/fake.go index b10d17393..d5e78a72e 100644 --- a/pkg/cmd/testing/fake.go +++ b/pkg/cmd/testing/fake.go @@ -641,7 +641,7 @@ func testRESTMapper() meta.RESTMapper { } fakeDs := NewFakeCachedDiscoveryClient() - expander := restmapper.NewShortcutExpander(mapper, fakeDs) + expander := restmapper.NewShortcutExpander(mapper, fakeDs, nil) return expander } diff --git a/pkg/util/term/term.go b/pkg/util/term/term.go index 6bcda59d7..93a992fe3 100644 --- a/pkg/util/term/term.go +++ b/pkg/util/term/term.go @@ -19,7 +19,8 @@ package term import ( "io" "os" - "runtime" + + "k8s.io/cli-runtime/pkg/printers" "github.com/moby/term" @@ -56,46 +57,23 @@ type TTY struct { // IsTerminalIn returns true if t.In is a terminal. Does not check /dev/tty // even if TryDev is set. func (t TTY) IsTerminalIn() bool { - return IsTerminal(t.In) + return printers.IsTerminal(t.In) } // IsTerminalOut returns true if t.Out is a terminal. Does not check /dev/tty // even if TryDev is set. func (t TTY) IsTerminalOut() bool { - return IsTerminal(t.Out) + return printers.IsTerminal(t.Out) } -// IsTerminal returns whether the passed object is a terminal or not -func IsTerminal(i interface{}) bool { - _, terminal := term.GetFdInfo(i) - return terminal -} +// IsTerminal returns whether the passed object is a terminal or not. +// Deprecated: use printers.IsTerminal instead. +var IsTerminal = printers.IsTerminal // AllowsColorOutput returns true if the specified writer is a terminal and // the process environment indicates color output is supported and desired. -func AllowsColorOutput(w io.Writer) bool { - if !IsTerminal(w) { - return false - } - - // https://en.wikipedia.org/wiki/Computer_terminal#Dumb_terminals - if os.Getenv("TERM") == "dumb" { - return false - } - - // https://no-color.org/ - if _, nocolor := os.LookupEnv("NO_COLOR"); nocolor { - return false - } - - // On Windows WT_SESSION is set by the modern terminal component. - // Older terminals have poor support for UTF-8, VT escape codes, etc. - if runtime.GOOS == "windows" && os.Getenv("WT_SESSION") == "" { - return false - } - - return true -} +// Deprecated: use printers.AllowsColorOutput instead. +var AllowsColorOutput = printers.AllowsColorOutput // Safe invokes the provided function and will attempt to ensure that when the // function returns (or a termination signal is sent) that the terminal state