-
Notifications
You must be signed in to change notification settings - Fork 4
Attribute accessibility improvements, and icons #4
base: main
Are you sure you want to change the base?
Conversation
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. |
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. |
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? |
Sounds good to me. I've made the changes. |
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!