-
Notifications
You must be signed in to change notification settings - Fork 893
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.
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
deprecate --master and --karmada-config flags for aggregrated-apiserver #1336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I use the redundant parameters.
LGTM
There are some errcheck
need to be resolved.
@@ -57,7 +56,9 @@ func (o *Options) AddFlags(flags *pflag.FlagSet) { | |||
o.RecommendedOptions.AddFlags(flags) | |||
|
|||
flags.StringVar(&o.karmadaConfig, "karmada-config", o.karmadaConfig, "Path to a karmada-apiserver KubeConfig.") | |||
flags.MarkDeprecated("karmada-config", "This flag is currently no-op and will be deleted.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment for when to remove it, such as:
// Remove it when we are in v1.2(+).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice finding!
I found the description of --kubeconfig
is weird:
--kubeconfig string kubeconfig file pointing at the 'core' kubernetes server.
Can we update it?
diff --git a/cmd/aggregated-apiserver/app/options/options.go b/cmd/aggregated-apiserver/app/options/options.go
index b730778c..de9c9e0f 100644
--- a/cmd/aggregated-apiserver/app/options/options.go
+++ b/cmd/aggregated-apiserver/app/options/options.go
@@ -55,6 +55,7 @@ func NewOptions() *Options {
// AddFlags adds flags to the specified FlagSet.
func (o *Options) AddFlags(flags *pflag.FlagSet) {
o.RecommendedOptions.AddFlags(flags)
+ flags.Lookup("kubeconfig").Usage = "Path to karmada control plane kubeconfig file."
flags.StringVar(&o.karmadaConfig, "karmada-config", o.karmadaConfig, "Path to a karmada-apiserver KubeConfig.")
The |
…-apiserver Signed-off-by: carlory <[email protected]>
94390ce
to
679081c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
Leave LGTM to @XiShanYongYe-Chang
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Signed-off-by: carlory [email protected]
What type of PR is this?
/kind deprecation
/kind feature
What this PR does / why we need it:
informerfactory uses --kubeconfig to communicate with coreapiserver, we should reuse the same configuration.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: