Unwanted console output in the versiongetter API

[bart0sh]

API to get version information(ClusterVersion, KubeadmVersion and VersionFromCILabel) contain unwanted calls of fmt.Print functions:

https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/upgrade/versiongetter.go#L66
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/upgrade/versiongetter.go#L78
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/upgrade/versiongetter.go#L95

They should be moved outside of the API. It should be done carefully in order to not degrade user experience, i.e. kubeadm console output should stay the same.

[bart0sh]

/good-first-issue

[k8s-ci-robot]

@bart0sh:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

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.

[neolit123]

@bart0sh please link to code examples for such fmt.Print* calls in the first post.

[SataQiu]

/cc

[bart0sh]

@neolit123

please link to code examples for such fmt.Print* calls in the first post.

done

[jfbai]

I can help fix this.

@bart0sh @neolit123 Move them to caller side?

/assign

[bart0sh]

@jfbai Yes, that's pretty much what needs to be done. However, it should be done carefully to make sure user visible output doesn't change.

[jfbai]

@jfbai Yes, that's pretty much what needs to be done. However, it should be done carefully to make sure user visible output doesn't change.

I have an idea about move them to caller side, something like this:

// ClusterVersion gets API server version
func (g *KubeVersionGetter) ClusterVersion() (string, *versionutil.Version, error) {
	clusterVersionInfo, err := g.client.Discovery().ServerVersion()
	if err != nil {
		return "", nil, errors.Wrap(err, "Couldn't fetch cluster version from the API Server")
	}
	fmt.Fprintf(g.w, "[upgrade/versions] Cluster version: %s\n", clusterVersionInfo.String())

	clusterVersion, err := versionutil.ParseSemantic(clusterVersionInfo.String())
	if err != nil {
-               return "", nil, errors.Wrap(err, "Couldn't parse cluster version")
+		return clusterVersionInfo.String(), nil, errors.Wrap(err, "Couldn't parse cluster version")
	}
	return clusterVersionInfo.String(), clusterVersion, nil
}

// caller side
	kubeadmVersionStr, kubeadmVersion, err := versionGetterImpl.KubeadmVersion()
	if err != nil {
+		if len(kubeadmVersionStr) > 0 {
+			fmt.Printf("[upgrade/versions] kubeadm version: %s\n", kubeadmVersionStr)
+		}
		return upgrades, err
	}

Could this be fine?

[bart0sh]

@jfbai Not sure why do you use ClusterVersion() API and KubeadmVersion() on the caller side in your example, but I think I've got your idea. It's a bit implicit to my taste, but could work. You can also consider splitting ClusterVersion API to 2 APIs: ClusterVersionInfo() and ClusterVersion() It'd result in a bit more code duplication on the caller side, but it's more readable in my opinion.

[jfbai]

@jfbai Not sure why do you use ClusterVersion() API and KubeadmVersion() on the caller side in your example, but I think I've got your idea. It's a bit implicit to my taste, but could work.

@bart0sh My mistake.

You can also consider splitting ClusterVersion API to 2 APIs: ClusterVersionInfo() and ClusterVersion() It'd result in a bit more code duplication on the caller side, but it's more readable in my opinion.

IMO, your idea makes sense and I will implement in this way.