Feat/hw tool state pattern #134

jneo8 · 2023-12-11T10:57:50Z

Draft PR without unit test.

Store status in the charm _stored and use it for conditional expression
Instead of re-call the hw_white_list, use the hw_white_list stored in _stored

[EC] this PR should cover

Fix infinite event loop (remove defer)
Should not regenerate hardware list after 1st installation
Missing logic before refactor

gabrielcocenza

Hi, I left some comments that I think it can be improved.

I also would like to know why we are using # type: ignore in a lot of places of the code.

gabrielcocenza · 2023-12-11T12:53:49Z

src/charm.py

 import logging
 from time import sleep
-from typing import Any, Dict, Optional, Tuple
+from typing import Any, Dict, List, Optional, Tuple


typing Dict and List are deprecated. It's recommendable using plain list and dict

gabrielcocenza · 2023-12-11T13:04:16Z

src/charm.py

+        if len(self._stored.missing_resources) == 0:  # type: ignore
+            return False
+        return True


Maybe this would be equivalent?

Suggested change

if len(self._stored.missing_resources) == 0: # type: ignore

return False

return True

return len(self._stored.missing_resources) > 0

gabrielcocenza · 2023-12-11T13:05:22Z

src/charm.py

+        if len(self._stored.failed_strategies) == 0:  # type: ignore
+            return False
+        return True


Suggested change

if len(self._stored.failed_strategies) == 0: # type: ignore

return False

return True

return len(self._stored.failed_strategies) > 0

gabrielcocenza · 2023-12-11T13:09:05Z

src/charm.py

+        if (
+            len(self._stored.missing_resources) > 0  # type: ignore
+            or len(self._stored.failed_strategies) > 0  # type: ignore
+        ):
+            return False
+        return True


I think we can use state_missing_resource and state_failed_install_strategies and not repeat the logic

Suggested change

if (

len(self._stored.missing_resources) > 0 # type: ignore

or len(self._stored.failed_strategies) > 0 # type: ignore

):

return False

return True

return self.state_missing_resource or self.state_failed_install_strategies

I believe the meaning is different here, but I know what you want.

gabrielcocenza · 2023-12-11T13:15:35Z

src/charm.py

+        health = self.exporter.check_health()
+        if not health:
+            logger.warning("Exporter health check - failed.")
+        return health


We can use walrus notation

Suggested change

health = self.exporter.check_health()

if not health:

logger.warning("Exporter health check - failed.")

return health

if not health := self.exporter.check_health():

logger.warning("Exporter health check - failed.")

return health

gabrielcocenza · 2023-12-11T13:28:16Z

src/charm.py

+            config={},
+            exporter_installed=False,
+            installed=False,
+            hw_white_list=[],


nit: I think it's preferable using accept_list or allow_list than white_list

This terms is been used in whole project now. We can use another PR to replace it.

gabrielcocenza · 2023-12-11T13:41:53Z

src/charm.py


-        if not installed:
-            logger.error(msg)
+        self._log_stored()


Why here we don't have the same pattern of generating the message and logging as it happens on state_msg_missing_resources and state_msg_failed_install_strategies?

gabrielcocenza · 2023-12-11T13:42:43Z

src/charm.py

+    @property
+    def state_msg_missing_resources(self) -> str:
+        """Message for missing resources."""
+        return f"Missing resources: {list(self._stored.missing_resources)}"  # type: ignore


Maybe using .join as the comment bellow

gabrielcocenza · 2023-12-11T13:51:30Z

src/hw_tools.py

-        if len(failed_checks) > 0:
-            return False, f"Fail strategy checks: {failed_checks}"
-        return True, ""
+            if ok:


ok it's not a good variable name and it's not used on other places. I prefer checking directly like if strategy.check():

gabrielcocenza · 2023-12-11T13:57:34Z

src/service.py

        """Install the exporter."""
        logger.info("Installing %s.", EXPORTER_NAME)
-        success = self.template.render_config(port=port, level=level, redfish_creds=redfish_creds)
+        success = self.template.render_config(


I don't understand why we are using twice and overriding the variable success. If this fails it will not be checked and logged

rgildein

Couple of small comments, but for functionality point of view it looks good to me.

rgildein · 2023-12-11T13:35:11Z

src/charm.py

+    def _log_stored(self) -> None:
+        """Logger print _stored information."""
+        msg = (
+            "Hw tool state: "  # type: ignore


Why we need to use type: ignore here?

rgildein · 2023-12-11T13:35:34Z

src/charm.py

+        """Logger print _stored information."""
+        msg = (
+            "Hw tool state: "  # type: ignore
+            f"hw_white_list: {list(self._stored.hw_white_list)}"


It's really needed to convert list to list?

>>> a = [1, 2, 3] >>> f"without: {a} and with {list(a)}" 'without: [1, 2, 3] and with [1, 2, 3]'

rgildein · 2023-12-11T13:37:08Z

src/charm.py

+            f" installed_strategies: {list(self._stored.installed_strategies)}"
+            f" failed_strategies: {list(self._stored.failed_strategies)}"
+        )
+        logger.info(msg)


I will suggest to use lazy formatting instead of f-string, e.g. logger.log("some list: %s", some_list).

rgildein · 2023-12-11T13:43:13Z

src/charm.py

+            len(self._stored.missing_resources) > 0  # type: ignore
+            or len(self._stored.failed_strategies) > 0  # type: ignore


Why we need to have this type ignore again? The StoredState is not providing type hinting? If yes, could we report an issue about it (I check there is none).

Yes, StoredState is not providing type hinting.

I think it would be good to report an issue. Those type: ignore are enjoying :).

rgildein · 2023-12-11T13:44:05Z

src/charm.py

+        if (
+            len(self._stored.missing_resources) > 0  # type: ignore
+            or len(self._stored.failed_strategies) > 0  # type: ignore
+        ):
+            return False
+        return True


Suggested change

if (

len(self._stored.missing_resources) > 0 # type: ignore

or len(self._stored.failed_strategies) > 0 # type: ignore

):

return False

return True

return not (

len(self._stored.missing_resources) > 0 # type: ignore

or len(self._stored.failed_strategies) > 0 # type: ignore

)

rgildein · 2023-12-11T13:44:35Z

src/charm.py

+        if len(self._stored.missing_resources) == 0:  # type: ignore
+            return False
+        return True


Suggested change

if len(self._stored.missing_resources) == 0: # type: ignore

return False

return True

return not len(self._stored.missing_resources) == 0 # type: ignore

rgildein · 2023-12-11T13:57:16Z

src/hw_tools.py

            if strategy.name not in hw_white_list:
                continue


IMO we should provide some log message for skipped strategy, same in install function.

rgildein · 2023-12-11T14:00:16Z

src/charm.py

                return

            port = self.model.config.get("exporter-port", "10000")
            level = self.model.config.get("exporter-log-level", "INFO")
+            logger.info("Get redfish creds." "")


Suggested change

logger.info("Get redfish creds." "")

logger.info("Get redfish creds.")

feat: Implement hw tool state pattern

30f1cdc

jneo8 changed the base branch from master to feat/life-cycle December 11, 2023 10:58

gabrielcocenza requested changes Dec 11, 2023

View reviewed changes

rgildein suggested changes Dec 11, 2023

View reviewed changes

jneo8 closed this Dec 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/hw tool state pattern #134

Feat/hw tool state pattern #134

jneo8 commented Dec 11, 2023 •

edited by Pjack

Loading

gabrielcocenza left a comment

gabrielcocenza Dec 11, 2023

gabrielcocenza Dec 11, 2023

gabrielcocenza Dec 11, 2023

gabrielcocenza Dec 11, 2023

jneo8 Dec 12, 2023 •

edited

Loading

gabrielcocenza Dec 11, 2023

gabrielcocenza Dec 11, 2023

jneo8 Dec 12, 2023

gabrielcocenza Dec 11, 2023

gabrielcocenza Dec 11, 2023

gabrielcocenza Dec 11, 2023

gabrielcocenza Dec 11, 2023

rgildein left a comment

rgildein Dec 11, 2023

rgildein Dec 11, 2023

rgildein Dec 11, 2023

rgildein Dec 11, 2023

jneo8 Dec 12, 2023

rgildein Dec 12, 2023

rgildein Dec 11, 2023

rgildein Dec 11, 2023

rgildein Dec 11, 2023

rgildein Dec 11, 2023

		len(self._stored.missing_resources) > 0 # type: ignore
		or len(self._stored.failed_strategies) > 0 # type: ignore

	logger.info("Get redfish creds." "")
	logger.info("Get redfish creds.")

Feat/hw tool state pattern #134

Feat/hw tool state pattern #134

Conversation

jneo8 commented Dec 11, 2023 • edited by Pjack Loading

gabrielcocenza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneo8 Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgildein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneo8 commented Dec 11, 2023 •

edited by Pjack

Loading

jneo8 Dec 12, 2023 •

edited

Loading