boolean parameter clean up

[chuckha]

Is this a BUG REPORT or FEATURE REQUEST?

/kind cleanup

Weaving this boolean parameter through is misleading:

https://github.com/kubernetes/kubernetes/blob/69f5f5eff2803317c794bb545705185fed74c201/cmd/kubeadm/app/util/config/cluster.go#L44

The fact that newControlPlane dictates behavior of this code is a problem because this is an assumption the implementation is making for the caller.

Consider the situation where I want to fetch the init configuration but not execute the code bound up in the control flow dictated by the newControlPlane boolean. I cannot do this without forcing a true/false. In some cases, the wrong value will end up returning an error because of assumptions made by the code being executed (files existing).

I recommend extracting that logic and moving it out to after this function is called rather than being nested inside this specific function.

This refactor would both clean up several function signatures as well as make the upgrade code more explicit by having it set the necessary values.

If nothing else, renaming that boolean to "upgrading" would be an improvement.

[fabriziopandini]

@chuckha
After thinking a bit about this IMO fetch init configuration should be limited to the kubeadm init method, while in all the other place of the code we should fetch only ClusterConfiguration

[chuckha]

What you are suggesting sounds like a bigger refactor, perhaps worthy of its own ticket. This ticket is to address the difficulties around using boolean parameters in functions. Perhaps you meant to comment on kubernetes/kubernetes#69662 ? That is fetching InitConfiguration during a Join.

[fabriziopandini]

@chuckha +1 on doing small improvements.

Sorry I skipped some detail in my comment, but if I remember well the boolean flag we are discussing on was implemented when kubeadm started to fetch init configuration in the context of the kubeadm join --control-plane workflow (and this should be probably fixed by fetching ClusterConfiguration only).

[rosti]

I would suggest to rename this specific boolean to clusterConfigOnly, since what it does is to prevent filling in NodeRegistration and LocalAPIEndpoint in InitConfiguration, thus leaving only ClusterConfiguration valid.
My point is, that if we rename it to upgrade we'll have to set it to true during reset too (which is clearly not an upgrade).

Anyway, I am doing a set of changes that restructure, cleanup the interface and increase code sharing in the config APIs (as part of #1084 effort), however it would take some time prior to having a PR for this specifically. In these changes the bool is gone completely, replaced by a couple of separate functions.

[chuckha]

Let's close this in favor of @rosti's work. I think the point was missed as the variable name change was not very interesting of a ticket.