Skip to content

Conversation

@allencloud
Copy link
Member

@allencloud allencloud commented Sep 28, 2022

Signed-off-by: Allen Sun [email protected]

Describe what this PR does / why we need it

  1. remove several const values which are never used: RemoteApplyYaml, RemoteCmdGetNetworkInterface, RemoteCmdExistNetworkInterface;
  2. Use built-in package k8s.Client when executing kubectl command, like kubectl delete node xxx, kubectl get nodes; After this pull request, there are still some kubectl command there, like kubectl drain and kubectl uncordon in upgrade operation. I found that k8s.Client has not contained drain and uncordon command, and we should use another kubernetes/kubectl package to cover the above both.
  3. rename function isHostName into isHostInExistingCluster to be more readable.
  4. flatten the struct runtime.Runtime to avoid useless embedding.

Does this pull request fix one issue?

none

Describe how you did it

none

Describe how to verify it

none

Special notes for reviews

@allencloud allencloud added the refactor sealer code refactor related label Sep 29, 2022
// Its data is from KubeadmConfig input from Clusterfile and KubeadmConfig in ClusterImage.
*kubeadm.KubeadmConfig
*Config
// *Config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel confused about the KubeadmConfigFromClusterfile. I notice that "*kubeadm.KubeadmConfig" above is used to setup the final cluster, but what is KubeadmConfigFromClusterfile capable of?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is because that we lack the design of how many ways we allow to input KubeadmConfig to a cluster instance. If the sealer policy allows to input KubeadmConfig from Clusterfile, then KubeadmConfigFromClusterfile is useful.

I wish to make it strictly enough when we are evolving, otherwise it may be out of control. Still be design problem, I think. @justadogistaken

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor sealer code refactor related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants