From a0ebfbb51ef33c85fad670fe2ce7cb87736dc23f Mon Sep 17 00:00:00 2001 From: James Page Date: Wed, 18 Jan 2023 13:27:31 +0000 Subject: [PATCH] General tidy for charm Update allowlist_externals with full path to commands for tox4 compatibility. Resync charm scaffolding with other Charmed K8S operators. Fixup numerous lint warnings across codebase. Change-Id: Ic39ad1351265cb5df9813cee5d27ee0d1392c632 --- charms/cinder-k8s/pyproject.toml | 39 ++++++ charms/cinder-k8s/src/charm.py | 132 +++++++++++------- charms/cinder-k8s/tests/__init__.py | 16 +++ charms/cinder-k8s/tests/test_charm.py | 68 --------- charms/cinder-k8s/tests/tests.yaml | 2 +- charms/cinder-k8s/tests/unit/__init__.py | 16 +++ .../tests/unit/test_cinder_charm.py | 38 ++--- charms/cinder-k8s/tox.ini | 47 ++++++- 8 files changed, 211 insertions(+), 147 deletions(-) create mode 100644 charms/cinder-k8s/pyproject.toml delete mode 100644 charms/cinder-k8s/tests/test_charm.py diff --git a/charms/cinder-k8s/pyproject.toml b/charms/cinder-k8s/pyproject.toml new file mode 100644 index 00000000..2896bc05 --- /dev/null +++ b/charms/cinder-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/cinder-k8s/src/charm.py b/charms/cinder-k8s/src/charm.py index 217ed80c..474f1864 100755 --- a/charms/cinder-k8s/src/charm.py +++ b/charms/cinder-k8s/src/charm.py @@ -1,23 +1,44 @@ #!/usr/bin/env python3 + +# +# Copyright 2021 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. + + """Cinder Operator Charm. This charm provide Cinder services as part of an OpenStack deployment """ import logging -from typing import List - -import ops.pebble - -from ops.framework import StoredState -from ops.main import main - -import ops_sunbeam.charm as sunbeam_charm -import ops_sunbeam.core as sunbeam_core -import ops_sunbeam.container_handlers as sunbeam_chandlers -import ops_sunbeam.relation_handlers as sunbeam_rhandlers +from typing import ( + Dict, + List, +) import charms.cinder_k8s.v0.storage_backend as sunbeam_storage_backend # noqa +import ops.pebble +import ops_sunbeam.charm as sunbeam_charm +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.framework import ( + StoredState, +) +from ops.main import ( + main, +) logger = logging.getLogger(__name__) @@ -27,26 +48,19 @@ CINDER_SCHEDULER_CONTAINER = "cinder-scheduler" class CinderWSGIPebbleHandler(sunbeam_chandlers.WSGIPebbleHandler): + """Pebble handler for Cinder WSGI services.""" + def start_service(self): + """Start services in container.""" pass def init_service(self, context) -> None: - """Enable and start WSGI service""" + """Enable and start WSGI service.""" self.write_config(context) try: + self.execute(["a2disconf", "cinder-wsgi"], exception_on_error=True) self.execute( - [ - "a2disconf", - "cinder-wsgi" - ], - exception_on_error=True - ) - self.execute( - [ - "a2ensite", - self.wsgi_service_name - ], - exception_on_error=True + ["a2ensite", self.wsgi_service_name], exception_on_error=True ) except ops.pebble.ExecError: logger.exception( @@ -69,16 +83,17 @@ class CinderWSGIPebbleHandler(sunbeam_chandlers.WSGIPebbleHandler): "online": { "override": "replace", "level": "ready", - "exec": { - "command": "service apache2 status" - } + "exec": {"command": "service apache2 status"}, }, } } class CinderSchedulerPebbleHandler(sunbeam_chandlers.PebbleHandler): + """Pebble handler for Cinder Scheduler services.""" + def start_service(self): + """Start services in container.""" container = self.charm.unit.get_container(self.container_name) if not container: logger.debug( @@ -93,7 +108,7 @@ class CinderSchedulerPebbleHandler(sunbeam_chandlers.PebbleHandler): container.start(self.service_name) def get_layer(self) -> dict: - """Cinder Scheduler service + """Cinder Scheduler service. :returns: pebble layer configuration for wsgi services :rtype: dict @@ -122,19 +137,18 @@ class CinderSchedulerPebbleHandler(sunbeam_chandlers.PebbleHandler): "online": { "override": "replace", "level": "ready", - "exec": { - "command": "service cinder-scheduler status" - } + "exec": {"command": "service cinder-scheduler status"}, }, } } - def init_service(self, context): + def init_service(self, context) -> None: + """Initialize services and write configuration.""" self.write_config(context) self.start_service() - self._state.service_ready = True - def default_container_configs(self): + def default_container_configs(self) -> List[Dict]: + """Generate default configuration files for container.""" return [ sunbeam_core.ContainerConfigFile( "/etc/cinder/cinder.conf", @@ -145,6 +159,8 @@ class CinderSchedulerPebbleHandler(sunbeam_chandlers.PebbleHandler): class StorageBackendRequiresHandler(sunbeam_rhandlers.RelationHandler): + """Relation handler for cinder storage backends.""" + def setup_event_handler(self): """Configure event handlers for an Identity service relation.""" logger.debug("Setting up Identity Service event handler") @@ -162,10 +178,12 @@ class StorageBackendRequiresHandler(sunbeam_rhandlers.RelationHandler): self.callback_f(event) def set_ready(self) -> None: + """Flag that all services are running and ready for use.""" return self.interface.set_ready() @property def ready(self) -> bool: + """Determine whether interface is ready for use.""" return True @@ -179,11 +197,11 @@ class CinderOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): wsgi_public_script = "/usr/bin/cinder-wsgi" mandatory_relations = { - 'database', - 'amqp', - 'storage-backend', - 'identity-service', - 'ingress-public', + "database", + "amqp", + "storage-backend", + "identity-service", + "ingress-public", } def __init__(self, framework): @@ -210,7 +228,8 @@ class CinderOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): return handlers @property - def service_endpoints(self): + def service_endpoints(self) -> List[Dict]: + """Service endpoints for the Cinder API services.""" return [ { "service_name": "cinderv2", @@ -237,7 +256,7 @@ class CinderOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): _cconfigs.extend( [ sunbeam_core.ContainerConfigFile( - '/etc/cinder/api-paste.ini', + "/etc/cinder/api-paste.ini", self.service_user, self.service_group, ) @@ -245,7 +264,8 @@ class CinderOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): ) return _cconfigs - def get_pebble_handlers(self): + def get_pebble_handlers(self) -> List[sunbeam_chandlers.PebbleHandler]: + """Pebble handlers for the charm.""" pebble_handlers = [ CinderWSGIPebbleHandler( self, @@ -269,14 +289,17 @@ class CinderOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): @property def default_public_ingress_port(self): + """Public ingress port for service.""" return 8776 @property def wsgi_container_name(self): + """WSGI API service container name.""" return CINDER_API_CONTAINER def _do_bootstrap(self): - """ + """Bootstrap the service ready for use. + Starts the appropriate services in the order they are needed. If the service has not yet been bootstrapped, then this will 1. Create the database @@ -287,16 +310,18 @@ class CinderOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): pebble_handler = self.get_named_pebble_handler( CINDER_SCHEDULER_CONTAINER ) - pebble_handler.execute([ - "sudo", - "-u", - "cinder", - "cinder-manage", - "--config-dir", - "/etc/cinder", - "db", - "sync"], - exception_on_error=True + pebble_handler.execute( + [ + "sudo", + "-u", + "cinder", + "cinder-manage", + "--config-dir", + "/etc/cinder", + "db", + "sync", + ], + exception_on_error=True, ) except ops.pebble.ExecError: logger.exception("Failed to bootstrap") @@ -304,6 +329,7 @@ class CinderOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): return def configure_charm(self, event) -> None: + """Configure the charmed services.""" super().configure_charm(event) # Restarting services after bootstrap should be in aso if self._state.bootstrapped: diff --git a/charms/cinder-k8s/tests/__init__.py b/charms/cinder-k8s/tests/__init__.py index e69de29b..eb22d3fc 100644 --- a/charms/cinder-k8s/tests/__init__.py +++ b/charms/cinder-k8s/tests/__init__.py @@ -0,0 +1,16 @@ +# +# Copyright 2021 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. + +"""Tests for charmed cinder.""" diff --git a/charms/cinder-k8s/tests/test_charm.py b/charms/cinder-k8s/tests/test_charm.py deleted file mode 100644 index 27574ada..00000000 --- a/charms/cinder-k8s/tests/test_charm.py +++ /dev/null @@ -1,68 +0,0 @@ -# Copyright 2021 Canonical Ltd. -# See LICENSE file for licensing details. -# -# Learn more about testing at: https://juju.is/docs/sdk/testing - -import unittest -from unittest.mock import Mock - -from charm import CharmCinderOperatorCharm -from ops.model import ActiveStatus -from ops.testing import Harness - - -class TestCharm(unittest.TestCase): - def setUp(self): - self.harness = Harness(CharmCinderOperatorCharm) - self.addCleanup(self.harness.cleanup) - self.harness.begin() - - def test_config_changed(self): - self.assertEqual(list(self.harness.charm._stored.things), []) - self.harness.update_config({"thing": "foo"}) - self.assertEqual(list(self.harness.charm._stored.things), ["foo"]) - - def test_action(self): - # the harness doesn't (yet!) help much with actions themselves - action_event = Mock(params={"fail": ""}) - self.harness.charm._on_fortune_action(action_event) - - self.assertTrue(action_event.set_results.called) - - def test_action_fail(self): - action_event = Mock(params={"fail": "fail this"}) - self.harness.charm._on_fortune_action(action_event) - - self.assertEqual(action_event.fail.call_args, [("fail this",)]) - - def test_httpbin_pebble_ready(self): - # Check the initial Pebble plan is empty - initial_plan = self.harness.get_container_pebble_plan("httpbin") - self.assertEqual(initial_plan.to_yaml(), "{}\n") - # Expected plan after Pebble ready with default config - expected_plan = { - "services": { - "httpbin": { - "override": "replace", - "summary": "httpbin", - "command": "gunicorn -b 0.0.0.0:80 httpbin:app -k gevent", - "startup": "enabled", - "environment": {"thing": "🎁"}, - } - }, - } - # Get the httpbin container from the model - container = self.harness.model.unit.get_container("httpbin") - # Emit the PebbleReadyEvent carrying the httpbin container - self.harness.charm.on.httpbin_pebble_ready.emit(container) - # Get the plan now we've run PebbleReady - updated_plan = self.harness.get_container_pebble_plan( - "httpbin").to_dict() - # Check we've got the plan we expected - self.assertEqual(expected_plan, updated_plan) - # Check the service was started - service = self.harness.model.unit.get_container( - "httpbin").get_service("httpbin") - self.assertTrue(service.is_running()) - # Ensure we set an ActiveStatus with no message - self.assertEqual(self.harness.model.unit.status, ActiveStatus()) diff --git a/charms/cinder-k8s/tests/tests.yaml b/charms/cinder-k8s/tests/tests.yaml index 744c5f8e..bf964f00 100644 --- a/charms/cinder-k8s/tests/tests.yaml +++ b/charms/cinder-k8s/tests/tests.yaml @@ -28,7 +28,7 @@ target_deploy_status: workload-status-message-regex: '^$' mysql: workload-status: active - workload-status-message-regex: '^$' + workload-status-message-regex: '^.*$' cinder: workload-status: blocked workload-status-message-regex: '\(storage-backend\) integration missing' diff --git a/charms/cinder-k8s/tests/unit/__init__.py b/charms/cinder-k8s/tests/unit/__init__.py index e69de29b..6f9a030b 100644 --- a/charms/cinder-k8s/tests/unit/__init__.py +++ b/charms/cinder-k8s/tests/unit/__init__.py @@ -0,0 +1,16 @@ +# +# Copyright 2021 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 Cinder K8S Operator.""" diff --git a/charms/cinder-k8s/tests/unit/test_cinder_charm.py b/charms/cinder-k8s/tests/unit/test_cinder_charm.py index 31b85406..2c2a2f77 100644 --- a/charms/cinder-k8s/tests/unit/test_cinder_charm.py +++ b/charms/cinder-k8s/tests/unit/test_cinder_charm.py @@ -14,13 +14,16 @@ # See the License for the specific language governing permissions and # limitations under the License. +"""Unit tests for core Cinder charm class.""" + import mock +import ops_sunbeam.test_utils as test_utils import charm -import ops_sunbeam.test_utils as test_utils class _CinderOperatorCharm(charm.CinderOperatorCharm): + """Test implementation of Cinder operator.""" def __init__(self, framework): self.seen_events = [] @@ -30,40 +33,39 @@ class _CinderOperatorCharm(charm.CinderOperatorCharm): def _log_event(self, event): self.seen_events.append(type(event).__name__) - def renderer(self, containers, container_configs, template_dir, - adapters): + def renderer(self, containers, container_configs, template_dir, adapters): + """Intercept and record all calls to render config files.""" self.render_calls.append( - ( - containers, - container_configs, - template_dir, - adapters)) + (containers, container_configs, template_dir, adapters) + ) def configure_charm(self, event): + """Intercept and record full charm configuration events.""" super().configure_charm(event) self._log_event(event) class TestCinderOperatorCharm(test_utils.CharmTestCase): + """Unit tests for Cinder Operator.""" PATCHES = [] @mock.patch( - 'charms.observability_libs.v0.kubernetes_service_patch.' - 'KubernetesServicePatch') + "charms.observability_libs.v0.kubernetes_service_patch." + "KubernetesServicePatch" + ) def setUp(self, mock_patch): - self.container_calls = { - 'push': {}, - 'pull': [], - 'remove_path': []} + """Setup test fixtures for test.""" + self.container_calls = {"push": {}, "pull": [], "remove_path": []} super().setUp(charm, self.PATCHES) self.harness = test_utils.get_harness( - _CinderOperatorCharm, - container_calls=self.container_calls) + _CinderOperatorCharm, container_calls=self.container_calls + ) self.addCleanup(self.harness.cleanup) self.harness.begin() def test_pebble_ready_handler(self): + """Test pebble ready event handling.""" self.assertEqual(self.harness.charm.seen_events, []) - self.harness.container_pebble_ready('cinder-api') - self.assertEqual(self.harness.charm.seen_events, ['PebbleReadyEvent']) + self.harness.container_pebble_ready("cinder-api") + self.assertEqual(self.harness.charm.seen_events, ["PebbleReadyEvent"]) diff --git a/charms/cinder-k8s/tox.ini b/charms/cinder-k8s/tox.ini index f4e1790a..9606489f 100644 --- a/charms/cinder-k8s/tox.ini +++ b/charms/cinder-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 @@ -28,11 +30,20 @@ commands = stestr run --slowest {posargs} allowlist_externals = git charmcraft - fetch-libs.sh - rename.sh + {toxinidir}/fetch-libs.sh + {toxinidir}/rename.sh 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,30 @@ 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<6 + 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 = @@ -95,6 +125,9 @@ commands = [testenv:func-smoke] basepython = python3 +setenv = + TEST_MODEL_SETTINGS = automatically-retry-hooks=true + TEST_MAX_RESOLVE_COUNT = 5 commands = functest-run-suite --keep-model --smoke