From ea4a84b9ca18615dc15c2c01b18306cf7756c2f6 Mon Sep 17 00:00:00 2001 From: Liam Young Date: Fri, 4 Nov 2022 06:29:54 +0000 Subject: [PATCH] General tidy for module ready for release. Refresh charm to drop release usage in ops-sunbeam. Drop surplus template fragments. Refresh unit tests. Tidy requirements.txt. Switch to black + other linters. Tidy docstrings across operator. Change-Id: I8ca548b84ddb7abb9f6c9ada6468ad78b0031160 --- charms/glance-k8s/pyproject.toml | 39 ++++ charms/glance-k8s/src/charm.py | 211 ++++++++++-------- charms/glance-k8s/tests/unit/__init__.py | 15 ++ .../tests/unit/test_glance_charm.py | 80 ++++--- charms/glance-k8s/tox.ini | 43 +++- 5 files changed, 262 insertions(+), 126 deletions(-) create mode 100644 charms/glance-k8s/pyproject.toml diff --git a/charms/glance-k8s/pyproject.toml b/charms/glance-k8s/pyproject.toml new file mode 100644 index 00000000..2896bc05 --- /dev/null +++ b/charms/glance-k8s/pyproject.toml @@ -0,0 +1,39 @@ +# Copyright 2022 Canonical Ltd. +# See LICENSE file for licensing details. + +# Testing tools configuration +[tool.coverage.run] +branch = true + +[tool.coverage.report] +show_missing = true + +[tool.pytest.ini_options] +minversion = "6.0" +log_cli_level = "INFO" + +# Formatting tools configuration +[tool.black] +line-length = 79 + +[tool.isort] +profile = "black" +multi_line_output = 3 +force_grid_wrap = true + +# Linting tools configuration +[tool.flake8] +max-line-length = 79 +max-doc-length = 99 +max-complexity = 10 +exclude = [".git", "__pycache__", ".tox", "build", "dist", "*.egg_info", "venv"] +select = ["E", "W", "F", "C", "N", "R", "D", "H"] +# Ignore W503, E501 because using black creates errors with this +# Ignore D107 Missing docstring in __init__ +ignore = ["W503", "E501", "D107", "E402"] +per-file-ignores = [] +docstring-convention = "google" +# Check for properly formatted copyright header in each file +copyright-check = "True" +copyright-author = "Canonical Ltd." +copyright-regexp = "Copyright\\s\\d{4}([-,]\\d{4})*\\s+%(author)s" diff --git a/charms/glance-k8s/src/charm.py b/charms/glance-k8s/src/charm.py index 94b56db0..da26c188 100755 --- a/charms/glance-k8s/src/charm.py +++ b/charms/glance-k8s/src/charm.py @@ -22,22 +22,32 @@ This charm provide Glance services as part of an OpenStack deployment """ import logging -from typing import Callable -from typing import List - -from ops.framework import StoredState, EventBase -from ops.main import main -from ops.model import ( - ActiveStatus, BlockedStatus, WaitingStatus, +from typing import ( + Callable, + List, ) -from ops.charm import CharmBase import ops_sunbeam.charm as sunbeam_charm import ops_sunbeam.compound_status as compound_status -import ops_sunbeam.core as sunbeam_core -import ops_sunbeam.relation_handlers as sunbeam_rhandlers import ops_sunbeam.config_contexts as sunbeam_ctxts import ops_sunbeam.container_handlers as sunbeam_chandlers +import ops_sunbeam.core as sunbeam_core +import ops_sunbeam.relation_handlers as sunbeam_rhandlers +from ops.charm import ( + CharmBase, +) +from ops.framework import ( + EventBase, + StoredState, +) +from ops.main import ( + main, +) +from ops.model import ( + ActiveStatus, + BlockedStatus, + WaitingStatus, +) logger = logging.getLogger(__name__) @@ -47,6 +57,7 @@ logger = logging.getLogger(__name__) class GlanceAPIPebbleHandler(sunbeam_chandlers.ServicePebbleHandler): + """Handler for glance api container.""" def get_layer(self) -> dict: """Glance API service pebble layer. @@ -60,8 +71,10 @@ class GlanceAPIPebbleHandler(sunbeam_chandlers.ServicePebbleHandler): f"{self.service_name}": { "override": "replace", "summary": f"{self.service_name} standalone", - "command": ("/usr/bin/glance-api " - "--config-file /etc/glance/glance-api.conf"), + "command": ( + "/usr/bin/glance-api " + "--config-file /etc/glance/glance-api.conf" + ), "startup": "disabled", }, "apache forwarder": { @@ -94,23 +107,30 @@ class GlanceStorageRelationHandler(sunbeam_rhandlers.CephClientHandler): ) -> None: """Run constructor.""" self.juju_storage_name = juju_storage_name - super().__init__(charm, relation_name, callback_f, allow_ec_overwrites, - app_name, mandatory=True) + super().__init__( + charm, + relation_name, + callback_f, + allow_ec_overwrites, + app_name, + mandatory=True, + ) def set_status(self, status: compound_status.Status) -> None: - """ - Override the base set_status. + """Override the base set_status. Custom logic is required here since this relation handler falls back to local storage if the ceph relation isn't found. """ if ( - not self.charm.has_ceph_relation() and - not self.charm.has_local_storage() + not self.charm.has_ceph_relation() + and not self.charm.has_local_storage() ): - status.set(BlockedStatus( - "ceph integration and local storage are not available" - )) + status.set( + BlockedStatus( + "ceph integration and local storage are not available" + ) + ) elif self.ready: status.set(ActiveStatus("")) else: @@ -132,20 +152,22 @@ class GlanceStorageRelationHandler(sunbeam_rhandlers.CephClientHandler): :return: True if the storage is ready, False otherwise. """ if self.charm.has_ceph_relation(): - logger.debug('ceph relation is connected, deferring to parent') + logger.debug("ceph relation is connected, deferring to parent") return super().ready # Check to see if the storage is satisfied if self.charm.has_local_storage(): - logger.debug(f'Storage {self.juju_storage_name} is attached') + logger.debug(f"Storage {self.juju_storage_name} is attached") return True - logger.debug('Ceph relation does not exist and no local storage is ' - 'available.') + logger.debug( + "Ceph relation does not exist and no local storage is " + "available." + ) return False def context(self) -> dict: - """ + """Context for the ceph relation. :return: """ @@ -162,28 +184,36 @@ class GlanceOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): _state = StoredState() _authed = False service_name = "glance-api" - wsgi_admin_script = '/usr/bin/glance-wsgi-api' - wsgi_public_script = '/usr/bin/glance-wsgi-api' + wsgi_admin_script = "/usr/bin/glance-wsgi-api" + wsgi_public_script = "/usr/bin/glance-wsgi-api" db_sync_cmds = [ - ['sudo', '-u', 'glance', 'glance-manage', '--config-dir', - '/etc/glance', 'db', 'sync']] + [ + "sudo", + "-u", + "glance", + "glance-manage", + "--config-dir", + "/etc/glance", + "db", + "sync", + ] + ] # ceph is included in the mandatory list as the GlanceStorage # relation handler falls back to juju storage if ceph relation # is not connected. mandatory_relations = { - 'database', - 'identity-service', - 'ingress-public', - 'ceph', + "database", + "identity-service", + "ingress-public", + "ceph", } def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) self.framework.observe( - self.on.describe_status_action, - self._describe_status_action + self.on.describe_status_action, self._describe_status_action ) def _describe_status_action(self, event: EventBase) -> None: @@ -194,12 +224,15 @@ class GlanceOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): """Configuration contexts for the operator.""" contexts = super().config_contexts if self.has_ceph_relation(): - logger.debug('Application has ceph relation') + logger.debug("Application has ceph relation") contexts.append( - sunbeam_ctxts.CephConfigurationContext(self, "ceph_config")) + sunbeam_ctxts.CephConfigurationContext(self, "ceph_config") + ) contexts.append( - sunbeam_ctxts.CinderCephConfigurationContext(self, - "cinder_ceph")) + sunbeam_ctxts.CinderCephConfigurationContext( + self, "cinder_ceph" + ) + ) return contexts @property @@ -209,7 +242,7 @@ class GlanceOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): _cconfigs.extend( [ sunbeam_core.ContainerConfigFile( - '/etc/apache2/sites-enabled/glance-forwarding.conf', + "/etc/apache2/sites-enabled/glance-forwarding.conf", self.service_user, self.service_group, ), @@ -235,8 +268,8 @@ class GlanceOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): "ceph", self.configure_charm, allow_ec_overwrites=True, - app_name='rbd', - juju_storage_name='local-repository', + app_name="rbd", + juju_storage_name="local-repository", ) handlers.append(self.ceph) return handlers @@ -249,41 +282,44 @@ class GlanceOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): @property def service_user(self) -> str: """Service user file and directory ownership.""" - return 'glance' + return "glance" @property def service_group(self) -> str: """Service group file and directory ownership.""" - return 'glance' + return "glance" @property def service_endpoints(self): + """Describe the glance service endpoint.""" return [ { - 'service_name': 'glance', - 'type': 'image', - 'description': "OpenStack Image", - 'internal_url': f'{self.internal_url}', - 'public_url': f'{self.public_url}', - 'admin_url': f'{self.admin_url}'}] + "service_name": "glance", + "type": "image", + "description": "OpenStack Image", + "internal_url": f"{self.internal_url}", + "public_url": f"{self.public_url}", + "admin_url": f"{self.admin_url}", + } + ] @property - def default_public_ingress_port(self): + def default_public_ingress_port(self) -> int: + """Default ingress port.""" return 9292 @property def healthcheck_http_url(self) -> str: """Healthcheck HTTP URL for the service.""" # / returns 300 and /versions return 200 - return f'http://localhost:{self.default_public_ingress_port}/versions' + return f"http://localhost:{self.default_public_ingress_port}/versions" def has_local_storage(self) -> bool: - """Returns whether the application has been deployed with local - storage or not. + """Whether the application has been deployed with local storage or not. :return: True if local storage is present, False otherwise """ - storages = self.model.storages['local-repository'] + storages = self.model.storages["local-repository"] return len(storages) > 0 def has_ceph_relation(self) -> bool: @@ -291,7 +327,7 @@ class GlanceOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): :return: True if the ceph relation has been made, False otherwise. """ - return self.model.get_relation('ceph') is not None + return self.model.get_relation("ceph") is not None def configure_charm(self, event) -> None: """Catchall handler to configure charm services.""" @@ -301,44 +337,43 @@ class GlanceOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): if self.has_ceph_relation(): if not self.ceph.key: - logger.debug('Ceph key is not yet present, waiting.') - self.status.set( - WaitingStatus("ceph key not present yet") - ) + logger.debug("Ceph key is not yet present, waiting.") + self.status.set(WaitingStatus("ceph key not present yet")) return elif self.has_local_storage(): - logger.debug('Local storage is configured, using that.') + logger.debug("Local storage is configured, using that.") else: - logger.debug('Neither local storage nor ceph relation exists.') - self.status.set(BlockedStatus( - 'Missing storage. Relate to Ceph ' - 'or add local storage to continue.' - )) + logger.debug("Neither local storage nor ceph relation exists.") + self.status.set( + BlockedStatus( + "Missing storage. Relate to Ceph " + "or add local storage to continue." + ) + ) return ph = self.get_named_pebble_handler("glance-api") - ph.execute( - ['a2enmod', 'proxy_http'], - exception_on_error=True) + ph.execute(["a2enmod", "proxy_http"], exception_on_error=True) if ph.pebble_ready: if self.has_ceph_relation() and self.ceph.key: - logger.debug('Setting up Ceph packages in images.') + logger.debug("Setting up Ceph packages in images.") + ph.execute(["apt", "update"], exception_on_error=True) ph.execute( - ['apt', 'update'], - exception_on_error=True) - ph.execute( - ['apt', 'install', '-y', 'ceph-common'], - exception_on_error=True) + ["apt", "install", "-y", "ceph-common"], + exception_on_error=True, + ) ph.execute( [ - 'ceph-authtool', - f'/etc/ceph/ceph.client.{self.app.name}.keyring', - '--create-keyring', - f'--name=client.{self.app.name}', - f'--add-key={self.ceph.key}'], - exception_on_error=True) + "ceph-authtool", + f"/etc/ceph/ceph.client.{self.app.name}.keyring", + "--create-keyring", + f"--name=client.{self.app.name}", + f"--add-key={self.ceph.key}", + ], + exception_on_error=True, + ) else: - logger.debug('Using local storage') + logger.debug("Using local storage") ph.init_service(self.contexts()) super().configure_charm(event) @@ -356,18 +391,12 @@ class GlanceOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): self.service_name, self.container_configs, self.template_dir, - self.openstack_release, - self.configure_charm + self.configure_charm, ) ] -class GlanceXenaOperatorCharm(GlanceOperatorCharm): - - openstack_release = 'xena' - - if __name__ == "__main__": # Note: use_juju_for_storage=True required per # https://github.com/canonical/operator/issues/506 - main(GlanceXenaOperatorCharm, use_juju_for_storage=True) + main(GlanceOperatorCharm, use_juju_for_storage=True) diff --git a/charms/glance-k8s/tests/unit/__init__.py b/charms/glance-k8s/tests/unit/__init__.py index e69de29b..304f420a 100644 --- a/charms/glance-k8s/tests/unit/__init__.py +++ b/charms/glance-k8s/tests/unit/__init__.py @@ -0,0 +1,15 @@ +# Copyright 2022 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Unit tests for charm.""" diff --git a/charms/glance-k8s/tests/unit/test_glance_charm.py b/charms/glance-k8s/tests/unit/test_glance_charm.py index be385be3..b6b3021d 100644 --- a/charms/glance-k8s/tests/unit/test_glance_charm.py +++ b/charms/glance-k8s/tests/unit/test_glance_charm.py @@ -14,14 +14,15 @@ # See the License for the specific language governing permissions and # limitations under the License. -import mock +"""Tests for glance charm.""" -import charm +import mock import ops_sunbeam.test_utils as test_utils +import charm -class _GlanceXenaOperatorCharm(charm.GlanceXenaOperatorCharm): +class _GlanceOperatorCharm(charm.GlanceOperatorCharm): def __init__(self, framework): self.seen_events = [] super().__init__(framework) @@ -39,21 +40,24 @@ class _GlanceXenaOperatorCharm(charm.GlanceXenaOperatorCharm): class TestGlanceOperatorCharm(test_utils.CharmTestCase): + """Class for testing glance charm.""" PATCHES = [] def setUp(self): + """Setup Glance tests.""" super().setUp(charm, self.PATCHES) self.harness = test_utils.get_harness( - _GlanceXenaOperatorCharm, - container_calls=self.container_calls) + _GlanceOperatorCharm, container_calls=self.container_calls + ) # clean up events that were dynamically defined, # otherwise we get issues because they'll be redefined, # which is not allowed. from charms.data_platform_libs.v0.database_requires import ( - DatabaseEvents + DatabaseEvents, ) + for attr in ( "database_database_created", "database_endpoints_changed", @@ -68,47 +72,65 @@ class TestGlanceOperatorCharm(test_utils.CharmTestCase): test_utils.add_complete_ingress_relation(self.harness) @mock.patch( - 'charms.observability_libs.v0.kubernetes_service_patch.' - 'KubernetesServicePatch') + "charms.observability_libs.v0.kubernetes_service_patch." + "KubernetesServicePatch" + ) def test_pebble_ready_handler(self, svc_patch): + """Test Pebble ready event is captured.""" self.harness.begin() self.assertEqual(self.harness.charm.seen_events, []) test_utils.set_all_pebbles_ready(self.harness) - self.assertEqual(self.harness.charm.seen_events, ['PebbleReadyEvent']) + self.assertEqual(self.harness.charm.seen_events, ["PebbleReadyEvent"]) @mock.patch( - 'charms.observability_libs.v0.kubernetes_service_patch.' - 'KubernetesServicePatch') + "charms.observability_libs.v0.kubernetes_service_patch." + "KubernetesServicePatch" + ) def test_all_relations(self, svc_patch): + """Test all the charms relations.""" ceph_rel_id = self.harness.add_relation("ceph", "ceph-mon") self.harness.begin_with_initial_hooks() self.harness.add_relation_unit(ceph_rel_id, "ceph-mon/0") self.harness.update_relation_data( - ceph_rel_id, - "ceph-mon/0", - {"ingress-address": "10.0.0.33"}) + ceph_rel_id, "ceph-mon/0", {"ingress-address": "10.0.0.33"} + ) test_utils.add_ceph_relation_credentials(self.harness, ceph_rel_id) test_utils.add_api_relations(self.harness) self.harness.set_leader() test_utils.set_all_pebbles_ready(self.harness) ceph_install_cmds = [ - ['apt', 'update'], - ['apt', 'install', '-y', 'ceph-common'], - ['ceph-authtool', - '/etc/ceph/ceph.client.glance-k8s.keyring', - '--create-keyring', - '--name=client.glance-k8s', - '--add-key=AQBUfpVeNl7CHxAA8/f6WTcYFxW2dJ5VyvWmJg==']] + ["apt", "update"], + ["apt", "install", "-y", "ceph-common"], + [ + "ceph-authtool", + "/etc/ceph/ceph.client.glance-k8s.keyring", + "--create-keyring", + "--name=client.glance-k8s", + "--add-key=AQBUfpVeNl7CHxAA8/f6WTcYFxW2dJ5VyvWmJg==", + ], + ] for cmd in ceph_install_cmds: - self.assertIn(cmd, self.container_calls.execute['glance-api']) + self.assertIn(cmd, self.container_calls.execute["glance-api"]) app_setup_cmds = [ - ['a2enmod', 'proxy_http'], - ['sudo', '-u', 'glance', 'glance-manage', '--config-dir', - '/etc/glance', 'db', 'sync']] + ["a2enmod", "proxy_http"], + [ + "sudo", + "-u", + "glance", + "glance-manage", + "--config-dir", + "/etc/glance", + "db", + "sync", + ], + ] for cmd in app_setup_cmds: - self.assertIn(cmd, self.container_calls.execute['glance-api']) + self.assertIn(cmd, self.container_calls.execute["glance-api"]) - for f in ['/etc/apache2/sites-enabled/glance-forwarding.conf', - '/etc/glance/glance-api.conf', '/etc/ceph/ceph.conf']: - self.check_file('glance-api', f) + for f in [ + "/etc/apache2/sites-enabled/glance-forwarding.conf", + "/etc/glance/glance-api.conf", + "/etc/ceph/ceph.conf", + ]: + self.check_file("glance-api", f) diff --git a/charms/glance-k8s/tox.ini b/charms/glance-k8s/tox.ini index f120d78e..ea682f9a 100644 --- a/charms/glance-k8s/tox.ini +++ b/charms/glance-k8s/tox.ini @@ -15,6 +15,8 @@ minversion = 3.18.0 src_path = {toxinidir}/src/ tst_path = {toxinidir}/tests/ lib_path = {toxinidir}/lib/ +pyproject_toml = {toxinidir}/pyproject.toml +all_path = {[vars]src_path} {[vars]tst_path} [testenv] basepython = python3 @@ -33,6 +35,15 @@ allowlist_externals = deps = -r{toxinidir}/test-requirements.txt +[testenv:fmt] +description = Apply coding style standards to code +deps = + black + isort +commands = + isort {[vars]all_path} --skip-glob {[vars]lib_path} --skip {toxinidir}/.tox + black --config {[vars]pyproject_toml} {[vars]all_path} --exclude {[vars]lib_path} + [testenv:build] basepython = python3 deps = @@ -64,11 +75,6 @@ deps = {[testenv:py3]deps} basepython = python3.10 deps = {[testenv:py3]deps} -[testenv:pep8] -basepython = python3 -deps = {[testenv]deps} -commands = flake8 {posargs} {[vars]src_path} {[vars]tst_path} - [testenv:cover] basepython = python3 deps = {[testenv:py3]deps} @@ -83,6 +89,31 @@ commands = coverage xml -o cover/coverage.xml coverage report +[testenv:pep8] +description = Alias for lint +deps = {[testenv:lint]deps} +commands = {[testenv:lint]commands} + +[testenv:lint] +description = Check code against coding style standards +deps = + black + # flake8==4.0.1 # Pin version until https://github.com/csachs/pyproject-flake8/pull/14 is merged + flake8 + flake8-docstrings + flake8-copyright + flake8-builtins + pyproject-flake8 + pep8-naming + isort + codespell +commands = + codespell {[vars]all_path} + # pflake8 wrapper supports config from pyproject.toml + pflake8 --exclude {[vars]lib_path} --config {toxinidir}/pyproject.toml {[vars]all_path} + isort --check-only --diff {[vars]all_path} --skip-glob {[vars]lib_path} + black --config {[vars]pyproject_toml} --check --diff {[vars]all_path} --exclude {[vars]lib_path} + [testenv:func-noop] basepython = python3 commands = @@ -96,8 +127,8 @@ commands = [testenv:func-smoke] basepython = python3 setenv = - TEST_MAX_RESOLVE_COUNT = 5 TEST_MODEL_SETTINGS = automatically-retry-hooks=true + TEST_MAX_RESOLVE_COUNT = 5 commands = functest-run-suite --keep-model --smoke