virtual-kubelet / node-cli

CLI library for rapid prototyping of virtual-kubelet nodes
Apache License 2.0
18 stars 30 forks source link

feat: status chan #31

Closed LeoLiuYan closed 3 years ago

LeoLiuYan commented 4 years ago

cc @cpuguy83

LeoLiuYan commented 4 years ago

This pull request add NodeControllerReady and PodControllerReady function which expose in cli, is that ok? @cpuguy83

cpuguy83 commented 4 years ago

Can we instead expose the underlying controllers directly?

e.g. cmd.NodeController() and cmd.PodController(), and the readiness can be checked from there.

LeoLiuYan commented 4 years ago

Can we instead expose the underlying controllers directly?

e.g. cmd.NodeController() and cmd.PodController(), and the readiness can be checked from there.

That's may be not appropriate since PodCtroller in virtual-kubelet include a sync.Mutex field. So, should expose a interface when create PodController or NodeController?

cpuguy83 commented 4 years ago

The mutex is not exposed, it is private to the package.

LeoLiuYan commented 4 years ago

The mutex is not exposed, it is private to the package.

image As the lint says, "copylocks: assignment copies lock value" error, because of sync.Mutex.

cpuguy83 commented 4 years ago

That's because you have it copying a value instead of passing a pointer.

LeoLiuYan commented 4 years ago

That's because you have it copying a value instead of passing a pointer.

Already fix this issue. @cpuguy83

cpuguy83 commented 4 years ago

The root command is what creates the node and pod controller. There shouldn't be any need to pass that down.

LeoLiuYan commented 4 years ago

The root command is what creates the node and pod controller. There shouldn't be any need to pass that down.

But the root command not return controllers, so pass a pointer when we create the command, and then assign the value to the pointer.

cpuguy83 commented 4 years ago

What I'm saying is, let's have the command provide accessors for those controllers.

LeoLiuYan commented 4 years ago

Simplify the implements, remove point of controllers, just provide accessors. @cpuguy83

feiskyer commented 3 years ago

Close since there's no activity for more than 5 months. Feel free to open a new PR if you think the changes are still required.