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 upgrade plan
support json/yaml output
#108447
Kubeadm upgrade plan
support json/yaml output
#108447
Conversation
Skipping CI for Draft Pull Request. |
- cherry-pick 83941 and rebase
64da4c1
to
e117531
Compare
e117531
to
0476877
Compare
Kubeadm upgrade plan
support json/yaml outputKubeadm upgrade plan
support json/yaml output
/priority important-longterm |
} | ||
|
||
printLineSeparator(w) | ||
printLineSeparator(writer, printer) | ||
printer.Close(writer) |
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.
related to the comments about Close(), printing on close is a bit strange and this is confusing me.
would it make sense to print the object explicitly with PrintObj.
the p.Buffer = []outputapiv1alpha2.ComponentUpgradePlan{}
for the JSON/YAML printer makes more sense to be in a Flush()
do we have other usage of a Close() that actually makes sense related to a "close" operation?
e.g. closing open file handles.
be161a4
to
df462eb
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.
here is another review pass here.
- there are some minor typos in comments
- the Close() is better but i think there is a problem in Flush()
- there are some strange new lines here and there, we should make these consistent if we can
- if a text is printed without arguments it should use
{F}println()
and not{F}printf()
also as mentioned before, i'd really appreciate at least another reviewer for this...
see if you like to ping someone for review.
fmt.Println("") | ||
printer.Printf("[upgrade/config] In order to upgrade, a ConfigMap called %q in the %s namespace must exist.\n", constants.KubeadmConfigConfigMap, metav1.NamespaceSystem) | ||
printer.Printf("[upgrade/config] Without this information, 'kubeadm upgrade' won't know how to configure your upgraded cluster.\n") | ||
printer.Printf("\n") |
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.
we can add the Println() method to the interface and use it in all cases when we are printing a line without arguments. (similarly we have the Fprintf())
we have instances of this in common.go and more places.
Co-authored-by: Lubomir I. Ivanov <neolit123@gmail.com>
df462eb
to
22fb3be
Compare
/cc @Klaven @fabriziopandini |
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 for all the updates @pacoxu !
a bit disappointing that nobody else wants to review this...
it looks fine to me, but i can do another quick pass and maybe we can merge it in early 1.25.
/approve
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123, pacoxu 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 |
/hold cancel |
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Based on #83941.
Which issue(s) this PR fixes:
xref kubernetes/kubeadm#494
Special notes for your reviewer:
1. text print will not be changed.
2. json example: upgrade from v1.23.3 to v1.23.4
3. yaml example
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: