From c1f348d167498977462b98671c9358492f340c73 Mon Sep 17 00:00:00 2001 From: james_lin Date: Fri, 20 Sep 2024 15:42:29 +0800 Subject: [PATCH] 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 | 3 ++- charmcraft.yaml | 4 ++++ src/charm.py | 15 ++++----------- src/service.py | 7 ++----- tests/unit/test_service.py | 11 +++++++++++ 5 files changed, 23 insertions(+), 17 deletions(-) diff --git a/.github/workflows/check.yaml b/.github/workflows/check.yaml index 49fb47d..da1aa60 100644 --- a/.github/workflows/check.yaml +++ b/.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 diff --git a/charmcraft.yaml b/charmcraft.yaml index 49d0f5d..1b36f5c 100644 --- a/charmcraft.yaml +++ b/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. diff --git a/src/charm.py b/src/charm.py index 776b9dc..bf64dec 100755 --- a/src/charm.py +++ b/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 diff --git a/src/service.py b/src/service.py index c13fa77..ea3a035 100644 --- a/src/service.py +++ b/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 diff --git a/tests/unit/test_service.py b/tests/unit/test_service.py index be578d5..17b6c18 100644 --- a/tests/unit/test_service.py +++ b/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")