refactor: Address review comments on #100 (#101)

* refactor: Address review comments on #100

- Remove `# TODO: can we remove the base64 encoding?` on `.github/workflows/check.yaml`
- Add explanation when both `openstack-exporter` and `snap_channel` are set.
- Change string-formatting to f-string
- Remove double negative
- Raise SnapError directly
- Add explanation why we need snap local installation

.github/workflows/check.yaml

@@ -72,7 +72,8 @@ jobs:
       - name: external controller juju config
         run: |
           mkdir -p $HOME/.local/share/juju
-          # TODO: can we remove the base64 encoding?
+          # The credential are stored in repository's secret as base64 encoded string.
+          # This help to avoid special characters that can interface with the parsing of YAML/JSON files.
           echo ${{ secrets.JUJU_CONTROLLERS_YAML }} | base64 -d > $HOME/.local/share/juju/controllers.yaml
           echo ${{ secrets.JUJU_ACCOUNTS_YAML }} | base64 -d > $HOME/.local/share/juju/accounts.yaml
 

charmcraft.yaml

@@ -69,6 +69,8 @@ config:
         The charmed-openstack-exporter snap is by default installed from the latest/stable channel.
         This option allows the selection of a different channel.
 
+        In case the openStack-exporter resource has been attached, this option has no effect.
+
 
 links:
   documentation: https://discourse.charmhub.io/t/openstack-exporter-docs-index/13876
@@ -89,3 +91,5 @@ resources:
       scraping. The snap resource on Charmhub is intended to be an empty file. The charm will
       ignore an empty file resource and normally install the snap from the snap store, but a
       custom snap can also be provided here if needed.
+
+      This resource has top priority. If the resource is present, the snap_channel option will have no effect.

src/charm.py

@@ -17,12 +17,7 @@
 import yaml
 from charms.grafana_agent.v0.cos_agent import COSAgentProvider
 from charms.operator_libs_linux.v2.snap import SnapError
-from ops.model import (
-    ActiveStatus,
-    BlockedStatus,
-    ModelError,
-    WaitingStatus,
-)
+from ops.model import ActiveStatus, BlockedStatus, ModelError, WaitingStatus
 
 from service import SNAP_NAME, UPSTREAM_SNAP, get_installed_snap_service, snap_install_or_refresh
 
@@ -95,10 +90,8 @@ def _write_cloud_config(self, data: dict[str, str]) -> None:
         OS_CLIENT_CONFIG.parent.mkdir(parents=True, exist_ok=True)
         OS_CLIENT_CONFIG_CACERT.write_text(self.config["ssl_ca"])
 
-        auth_url = "{protocol}://{hostname}:{port}/v3".format(
-            protocol=data["service_protocol"],
-            hostname=data["service_hostname"],
-            port=data["service_port"],
+        auth_url = (
+            f"{data['service_protocol']}://{data['service_hostname']}:{data['service_port']}/v3"
         )
         contents = {
             "clouds": {
@@ -147,7 +140,7 @@ def get_resource(self) -> Optional[str]:
             logger.debug("cannot fetch charm resource")
             return None
 
-        if not os.path.getsize(snap_path) > 0:
+        if os.path.getsize(snap_path) <= 0:
             logger.debug("resource is an empty file")
             return None
 

src/service.py

@@ -44,11 +44,6 @@ def stop(self) -> None:
 
     def configure(self, snap_config: dict[str, Any]) -> None:
         """Configure the snap service."""
-        # Bail out or it will crash on self.snap_client.set() method
-        if not snap_config:
-            logger.warning("empty snap config: %s, skipping...", snap_config)
-            return
-
         self.snap_client.set(snap_config, typed=True)
 
     @property
@@ -73,6 +68,8 @@ def snap_install_or_refresh(resource: Optional[str], channel: str) -> None:
         if resource:
             logger.debug("installing %s from resource.", SNAP_NAME)
             # installing from a resource if installed from snap store previously is not problematic
+            # We allow manually attaching because some environment don't have the snap proxy.
+            # It should be deprecated for long term.
             snap.install_local(resource, dangerous=True)
         else:
             # installing from snap store if previously installed from resource is problematic, so

tests/unit/test_service.py

@@ -8,6 +8,17 @@
 import service
 
 
+class TestSnapService:
+    """Test SnapService class."""
+
+    def test_configure(self, mocker):
+        mock_client = mocker.Mock()
+        snap_service = service.SnapService(mock_client)
+        config = {"config-a": "a", "config-b": "b"}
+        snap_service.configure(config)
+        mock_client.set.assert_called_with(config, typed=True)
+
+
 @mock.patch("service.remove_upstream_snap")
 @mock.patch("service.remove_snap_as_resource")
 @mock.patch("service.workaround_bug_268")