-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
kubeadm: Remove the usage to print the default component configs for reset
and join
#119346
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I'm wondering if we should disable the |
print for join also doesn’t need it? instead...we can keep it enabled everywhere but a No-op for reset, join. i might be missing some context. |
Yes, these default configurations are only printed when the |
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
seems ok to clean these
I prefer to deprecate it as it is not needed(harmless as well).
It will make users confused like the below.
+1 However, all can be a follow-up. This PR lgtm. |
/hold this needs to be polished, we can do more instead of just remove those testcases. |
c414e90
to
cc163c6
Compare
/retitle kubeadm: Deprecate the default values of component config for /unhold |
ResetConfiguration
defaultsreset
and join
/test pull-kubernetes-e2e-gce |
cmd/kubeadm/app/cmd/config.go
Outdated
@@ -115,6 +115,7 @@ func newCmdConfigPrintResetDefaults(out io.Writer) *cobra.Command { | |||
|
|||
func newCmdConfigPrintActionDefaults(out io.Writer, action string, configBytesProc func() ([]byte, error)) *cobra.Command { | |||
kinds := []string{} | |||
deprecatedMsg := fmt.Sprintf("Print the default value of component config API objects is deprecated for action: %s, it will be ingored if set and will be removed in a future release.", action) |
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.
typo: ignore.
a newline at the end? Or the first line of the configuration will be print just after this line.
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.
Done.
print it to stdout use fmt.Fprintln
instead.
/assign @neolit123 |
cmd/kubeadm/app/cmd/config.go
Outdated
@@ -115,6 +115,7 @@ func newCmdConfigPrintResetDefaults(out io.Writer) *cobra.Command { | |||
|
|||
func newCmdConfigPrintActionDefaults(out io.Writer, action string, configBytesProc func() ([]byte, error)) *cobra.Command { | |||
kinds := []string{} | |||
deprecatedMsg := fmt.Sprintf("Print the default value of component config API objects is deprecated for action: %s, it will be ignored if set and will be removed in a future release.", action) |
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.
i may have misunderstood something around our discussion for UpgradeConfiguration?
we should continue printing the kubeadm's understanding of default component config, but we can stop managing component config via --config on upgrade.
EDIT: never mind. i'm losing context on what these changes are about.
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.
can we go without deprecation here and just remove the usage of component-configs for anything but init?
it feels like a bug to allow join, reset...etc to use these.
also, this needs a release note, will add it in the next iteration. |
/kind bug
Just remove it if this is a bug 😄. |
072ab51
to
6e62f8f
Compare
reset
and join
reset
and join
Done |
/kind bug |
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
SGTM
please always prefix kubeadm release notes with kubeadm:
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chendave, neolit123 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 |
Done. |
/hold testcase need to be updated as well. |
…`reset` and `join` component configs is only needed for `kubeadm init`, the `join` and `reset` doesn't need to provid the config with component configs. Signed-off-by: Dave Chen <[email protected]>
6e62f8f
to
879dad9
Compare
/unhold updated testcases since the |
/lgtm |
LGTM label has been added. Git tree hash: fd8fc23c0895bd03e652d01fa44e9953367235b5
|
/test pull-kubernetes-e2e-gce |
component configs is only needed for
kubeadm init
, thejoin
andreset
doesn't need to provid the config with component configs./kind bug
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: