Adi146 / home-assistant-kubernetes

Home Assistant Custom Component for monitoring Kubernetes Clusters
2 stars 4 forks source link

Attribute accessibility improvements, and icons #4

Open cerebrate opened 2 years ago

cerebrate commented 2 years ago

I made some modifications to this integration for my own use, so thought I'd share them back in case you think they're worth including:

Small stuff, but hopefully worthwhile!

Adi146 commented 2 years ago

Hey, thanks for your Pull Request. I like changes with the raw attribute and the icon but I'm not sure if we really need the ok attribute. The node and pod sensor basically already represent these in the state of the sensor. Only the daemonset and deployment sensor could use this attribute but I think we should rather also display this in the state. What do you think about this?

cerebrate commented 2 years ago

Well, I'm not wedded to the idea of the ok attribute. I added it mostly because I already needed to write a simple boolean is-everything-okay check to power the icon, and having done that, thought I might as well expose it. For basic automations (such as my "hey, this is broken, go check on it" announcement, having a simple true/false check is handy, and means I don't have to remember when writing them if it's "KubeletReady" or "Ready" or "Running", or whatever.

(I didn't put it in the state because it seemed like a breaking change that wasn't needed; if people are already relying on the state being the number of ready pods for daemonset/deployment, or the text state in node and pod, I didn't want to break their automation.)

But like I said, I'm not wedded to the idea. If you think it'd be better in the state, I have no issue with that. I'd just like to have that bool available somewhere.

Adi146 commented 2 years ago

Yeah, I think we should rather use this check in the state for daemonsets and deployments. For Nodes and Pods we should keep the sensor state as it is right now, this way it is more flexible to use. I don't care about breaking changes at this point, because as far as I know are we the only two using this at the moment. What do you think about that? Could you please adjust your Pull Request to that?

cerebrate commented 2 years ago

Sounds good to me. I've made the changes.