search

canonical / hardware-observer-operator

A charm to setup prometheus exporter for IPMI, RedFish and RAID devices from different vendors.
Apache License 2.0
7 stars 14 forks source link

Store hw tool list in _stored and reuse it. #217

Closed sudeephb closed 3 months ago

sudeephb commented 3 months ago

The list will be created only once, during the install hook, and the same list will be reused everywhere else. This avoids generating a new list everytime it's needed, and potentially losing a hw from the list when it can't be temporarily accessed. This way, the list of hardware being monitored is predictable. This helps avoid issues like #202 This also changes the 'white' list to 'enable' list. An action to refresh the hardware list will be provided as a future enhancement, so the operators can regenerate the hw tool list if needed.

Setting the target as a dev branch because this feature needs an accompanying action to regenerate hw list before it's merged to main.

sudeephb commented 3 months ago

Could we also replace whilte_list with allowlist in this refactor?

I think enable_list makes more sense here, since we're not allowing or denying anything. We are choosing what to enable or disable. What do you think?

Pjack commented 3 months ago

Could we also replace whilte_list with allowlist in this refactor?

I think enable_list makes more sense here, since we're not allowing or denying anything. We are choosing what to enable or disable. What do you think?

Good with enable/disable , so that should be enabled_list ?

dashmage commented 3 months ago

I realize this PR is merged but after checking out the changes, this usage to get the hwtool objects looks quite weird

self.hw_tool_helper.check_installed(
     self.get_hw_tools_from_values(self.get_enabled_hw_tool_list_values())
)

It would be nice if we could refactor later on to be more like

self.hw_tool_helper.check_installed(self.get_enabled_hw_tools())

We could make the other methods private if needed and make the usage a bit more cleaner.

sudeephb commented 3 months ago

I realize this PR is merged but after checking out the changes, this usage to get the hwtool objects looks quite weird

self.hw_tool_helper.check_installed(
     self.get_hw_tools_from_values(self.get_enabled_hw_tool_list_values())
)

It would be nice if we could refactor later on to be more like

self.hw_tool_helper.check_installed(self.get_enabled_hw_tools())

We could make the other methods private if needed and make the usage a bit more cleaner.

Thank you. This is still in dev branch, I will address these while creating a PR for the main branch.