From b65c3e31e4e1d9cf723bc2c31d79a6f014b72a5f Mon Sep 17 00:00:00 2001 From: Hemanth Nakkina Date: Fri, 4 Nov 2022 10:02:39 +0530 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: Iabc3b12d8409d304ad3a021ebc293f0cd545e63d --- charms/horizon-k8s/pyproject.toml | 39 ++++++++ charms/horizon-k8s/requirements.txt | 8 +- charms/horizon-k8s/src/charm.py | 90 +++++++++++-------- .../src/templates/local_settings.py.j2 | 2 +- .../src/templates/parts/identity-data | 13 --- .../src/templates/parts/section-database | 3 - .../src/templates/parts/section-federation | 10 --- .../src/templates/parts/section-identity | 2 - .../src/templates/parts/section-middleware | 6 -- .../src/templates/parts/section-signing | 15 ---- charms/horizon-k8s/tests/unit/__init__.py | 15 ++++ .../tests/unit/test_dashboard_charm.py | 56 +++++++----- charms/horizon-k8s/tox.ini | 41 +++++++-- 13 files changed, 183 insertions(+), 117 deletions(-) create mode 100644 charms/horizon-k8s/pyproject.toml delete mode 100644 charms/horizon-k8s/src/templates/parts/identity-data delete mode 100644 charms/horizon-k8s/src/templates/parts/section-database delete mode 100644 charms/horizon-k8s/src/templates/parts/section-federation delete mode 100644 charms/horizon-k8s/src/templates/parts/section-identity delete mode 100644 charms/horizon-k8s/src/templates/parts/section-middleware delete mode 100644 charms/horizon-k8s/src/templates/parts/section-signing diff --git a/charms/horizon-k8s/pyproject.toml b/charms/horizon-k8s/pyproject.toml new file mode 100644 index 00000000..2896bc05 --- /dev/null +++ b/charms/horizon-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/horizon-k8s/requirements.txt b/charms/horizon-k8s/requirements.txt index b9b8fb9b..e9f975ae 100644 --- a/charms/horizon-k8s/requirements.txt +++ b/charms/horizon-k8s/requirements.txt @@ -17,11 +17,11 @@ lightkube-models ops git+https://opendev.org/openstack/charm-ops-sunbeam#egg=ops_sunbeam -python-keystoneclient # keystone-k8s +# python-keystoneclient # keystone-k8s -git+https://opendev.org/openstack/charm-ops-interface-tls-certificates#egg=interface_tls_certificates +# git+https://opendev.org/openstack/charm-ops-interface-tls-certificates#egg=interface_tls_certificates # Note: Required for cinder-k8s, cinder-ceph-k8s, glance-k8s, nova-k8s -git+https://opendev.org/openstack/charm-ops-interface-ceph-client#egg=interface_ceph_client +# git+https://opendev.org/openstack/charm-ops-interface-ceph-client#egg=interface_ceph_client # Charmhelpers is only present as interface_ceph_client uses it. -git+https://github.com/juju/charm-helpers.git#egg=charmhelpers +# git+https://github.com/juju/charm-helpers.git#egg=charmhelpers diff --git a/charms/horizon-k8s/src/charm.py b/charms/horizon-k8s/src/charm.py index ced9fb2c..7d6d0fff 100755 --- a/charms/horizon-k8s/src/charm.py +++ b/charms/horizon-k8s/src/charm.py @@ -1,4 +1,18 @@ #!/usr/bin/env python3 +# 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. + """OpenstackDashboard Operator Charm. This charm provide OpenstackDashboard services as part of an OpenStack @@ -6,36 +20,38 @@ deployment """ import logging -from typing import List +from typing import ( + List, +) import ops.framework -from ops.main import main - -import ops_sunbeam.core as sunbeam_core import ops_sunbeam.charm as sunbeam_charm import ops_sunbeam.container_handlers as sunbeam_chandlers +import ops_sunbeam.core as sunbeam_core +from ops.main import ( + main, +) logger = logging.getLogger(__name__) class WSGIDashboardPebbleHandler(sunbeam_chandlers.WSGIPebbleHandler): + """Dashboard Pebble Handler.""" def init_service(self, context: sunbeam_core.OPSCharmContexts) -> None: """Enable and start WSGI service.""" container = self.charm.unit.get_container(self.container_name) try: process = container.exec( - ['a2dissite', '000-default'], - timeout=5*60) + ["a2dissite", "000-default"], timeout=5 * 60 + ) out, warnings = process.wait_output() if warnings: for line in warnings.splitlines(): - logger.warning('a2dissite warn: %s', line.strip()) - logging.debug(f'Output from a2dissite: \n{out}') + logger.warning("a2dissite warn: %s", line.strip()) + logging.debug(f"Output from a2dissite: \n{out}") except ops.pebble.ExecError: - logger.exception( - "Failed to disable default site in apache" - ) + logger.exception("Failed to disable default site in apache") super().init_service(context) @@ -45,26 +61,30 @@ class OpenstackDashboardOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): _state = ops.framework.StoredState() service_name = "openstack-dashboard" wsgi_admin_script = ( - "/usr/share/openstack-dashboard/openstack_dashboard/wsgi/django.wsgi") + "/usr/share/openstack-dashboard/openstack_dashboard/wsgi/django.wsgi" + ) wsgi_public_script = ( - "/usr/share/openstack-dashboard/openstack_dashboard/wsgi/django.wsgi") + "/usr/share/openstack-dashboard/openstack_dashboard/wsgi/django.wsgi" + ) db_sync_cmds = [ [ - 'python3', - '/usr/share/openstack-dashboard/manage.py', - 'migrate', - '--noinput'] + "python3", + "/usr/share/openstack-dashboard/manage.py", + "migrate", + "--noinput", + ] ] mandatory_relations = { - 'database', - 'ingress-public', - 'cloud-credentials', + "database", + "ingress-public", + "cloud-credentials", } @property def default_public_ingress_port(self): + """Default public ingress port.""" return 80 @property @@ -80,23 +100,26 @@ class OpenstackDashboardOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): @property def service_user(self) -> str: """Service user file and directory ownership.""" - return 'horizon' + return "horizon" @property def service_group(self) -> str: """Service group file and directory ownership.""" - return 'horizon' + return "horizon" @property def service_endpoints(self): + """Endpoints for horizon.""" return [ { - 'service_name': 'openstack-dashboard', - 'type': 'openstack-dashboard', - 'description': "OpenStack OpenstackDashboard API", - 'internal_url': f'{self.internal_url}', - 'public_url': f'{self.public_url}', - 'admin_url': f'{self.admin_url}'}] + "service_name": "openstack-dashboard", + "type": "openstack-dashboard", + "description": "OpenStack OpenstackDashboard API", + "internal_url": f"{self.internal_url}", + "public_url": f"{self.public_url}", + "admin_url": f"{self.admin_url}", + } + ] def get_pebble_handlers(self) -> List[sunbeam_chandlers.PebbleHandler]: """Pebble handlers for the service.""" @@ -107,7 +130,6 @@ class OpenstackDashboardOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): self.service_name, self.container_configs, self.template_dir, - self.openstack_release, self.configure_charm, f"wsgi-{self.service_name}", ) @@ -117,16 +139,10 @@ class OpenstackDashboardOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): """Configure charm services.""" super().configure_charm(event) if self.bootstrapped(): - self.unit.status = ops.model.ActiveStatus( - self.ingress_public.url) - - -class OpenstackDashboardXenaOperatorCharm(OpenstackDashboardOperatorCharm): - - openstack_release = 'xena' + self.unit.status = ops.model.ActiveStatus(self.ingress_public.url) if __name__ == "__main__": # Note: use_juju_for_storage=True required per # https://github.com/canonical/operator/issues/506 - main(OpenstackDashboardXenaOperatorCharm, use_juju_for_storage=True) + main(OpenstackDashboardOperatorCharm, use_juju_for_storage=True) diff --git a/charms/horizon-k8s/src/templates/local_settings.py.j2 b/charms/horizon-k8s/src/templates/local_settings.py.j2 index d99a2a16..d65e40d8 100644 --- a/charms/horizon-k8s/src/templates/local_settings.py.j2 +++ b/charms/horizon-k8s/src/templates/local_settings.py.j2 @@ -498,7 +498,7 @@ ENFORCE_PASSWORD_CHECK = True # Path to directory containing policy.json files #POLICY_FILES_PATH = os.path.join(ROOT_PATH, "conf") -# Policies are overriden and all policies are here rather than in package conf +# Policies are overridden and all policies are here rather than in package conf # POLICY_FILES_PATH = '/etc/openstack-dashboard/policy.d/' # These are matched from the defaults + any in the overrides diff --git a/charms/horizon-k8s/src/templates/parts/identity-data b/charms/horizon-k8s/src/templates/parts/identity-data deleted file mode 100644 index f9a78684..00000000 --- a/charms/horizon-k8s/src/templates/parts/identity-data +++ /dev/null @@ -1,13 +0,0 @@ -{% if identity_service.internal_auth_url -%} -www_authenticate_uri = {{ identity_service.internal_auth_url }} -auth_url = {{ identity_service.internal_auth_url }} -{% elif identity_service.internal_host -%} -www_authenticate_uri = {{ identity_service.internal_protocol }}://{{ identity_service.internal_host }}:{{ identity_service.internal_port }} -auth_url = {{ identity_service.internal_protocol }}://{{ identity_service.internal_host }}:{{ identity_service.internal_port }} -{% endif -%} -auth_type = password -project_domain_name = {{ identity_service.service_domain_name }} -user_domain_name = {{ identity_service.service_domain_name }} -project_name = {{ identity_service.service_project_name }} -username = {{ identity_service.service_user_name }} -password = {{ identity_service.service_password }} diff --git a/charms/horizon-k8s/src/templates/parts/section-database b/charms/horizon-k8s/src/templates/parts/section-database deleted file mode 100644 index 986d9b10..00000000 --- a/charms/horizon-k8s/src/templates/parts/section-database +++ /dev/null @@ -1,3 +0,0 @@ -[database] -{% include "parts/database-connection" %} -connection_recycle_time = 200 diff --git a/charms/horizon-k8s/src/templates/parts/section-federation b/charms/horizon-k8s/src/templates/parts/section-federation deleted file mode 100644 index 65ee99ed..00000000 --- a/charms/horizon-k8s/src/templates/parts/section-federation +++ /dev/null @@ -1,10 +0,0 @@ -{% if trusted_dashboards %} -[federation] -{% for dashboard_url in trusted_dashboards -%} -trusted_dashboard = {{ dashboard_url }} -{% endfor -%} -{% endif %} -{% for sp in fid_sps -%} -[{{ sp['protocol-name'] }}] -remote_id_attribute = {{ sp['remote-id-attribute'] }} -{% endfor -%} diff --git a/charms/horizon-k8s/src/templates/parts/section-identity b/charms/horizon-k8s/src/templates/parts/section-identity deleted file mode 100644 index 7568a9a4..00000000 --- a/charms/horizon-k8s/src/templates/parts/section-identity +++ /dev/null @@ -1,2 +0,0 @@ -[keystone_authtoken] -{% include "parts/identity-data" %} diff --git a/charms/horizon-k8s/src/templates/parts/section-middleware b/charms/horizon-k8s/src/templates/parts/section-middleware deleted file mode 100644 index e65f1d98..00000000 --- a/charms/horizon-k8s/src/templates/parts/section-middleware +++ /dev/null @@ -1,6 +0,0 @@ -{% for section in sections -%} -[{{section}}] -{% for key, value in sections[section].items() -%} -{{ key }} = {{ value }} -{% endfor %} -{%- endfor %} diff --git a/charms/horizon-k8s/src/templates/parts/section-signing b/charms/horizon-k8s/src/templates/parts/section-signing deleted file mode 100644 index cb7d69ae..00000000 --- a/charms/horizon-k8s/src/templates/parts/section-signing +++ /dev/null @@ -1,15 +0,0 @@ -{% if enable_signing -%} -[signing] -{% if certfile -%} -certfile = {{ certfile }} -{% endif -%} -{% if keyfile -%} -keyfile = {{ keyfile }} -{% endif -%} -{% if ca_certs -%} -ca_certs = {{ ca_certs }} -{% endif -%} -{% if ca_key -%} -ca_key = {{ ca_key }} -{% endif -%} -{% endif -%} \ No newline at end of file diff --git a/charms/horizon-k8s/tests/unit/__init__.py b/charms/horizon-k8s/tests/unit/__init__.py index e69de29b..072d76d6 100644 --- a/charms/horizon-k8s/tests/unit/__init__.py +++ b/charms/horizon-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 Horizon operator.""" diff --git a/charms/horizon-k8s/tests/unit/test_dashboard_charm.py b/charms/horizon-k8s/tests/unit/test_dashboard_charm.py index ae0d490a..f2190a67 100644 --- a/charms/horizon-k8s/tests/unit/test_dashboard_charm.py +++ b/charms/horizon-k8s/tests/unit/test_dashboard_charm.py @@ -14,14 +14,16 @@ # See the License for the specific language governing permissions and # limitations under the License. -import mock +"""Unit tests for Horizon operator.""" -import charm +import mock import ops_sunbeam.test_utils as test_utils +import charm -class _DashboardXenaOperatorCharm( - charm.OpenstackDashboardXenaOperatorCharm): + +class _DashboardOperatorCharm(charm.OpenstackDashboardOperatorCharm): + """Test Operator Charm for Horizon Operator.""" def __init__(self, framework): self.seen_events = [] @@ -36,28 +38,32 @@ class _DashboardXenaOperatorCharm( @property def public_ingress_address(self): - return 'dashboard.juju' + return "dashboard.juju" class TestDashboardOperatorCharm(test_utils.CharmTestCase): + """Unit tests for Horizon 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): + """Setup environment for unit test.""" super().setUp(charm, self.PATCHES) self.harness = test_utils.get_harness( - _DashboardXenaOperatorCharm, - container_calls=self.container_calls) + _DashboardOperatorCharm, 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", @@ -72,27 +78,35 @@ class TestDashboardOperatorCharm(test_utils.CharmTestCase): self.harness.begin() def test_pebble_ready_handler(self): + """Test pebble ready handler.""" 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"]) def test_all_relations(self): + """Test all integrations for Operator.""" self.harness.set_leader() test_utils.set_all_pebbles_ready(self.harness) test_utils.add_all_relations(self.harness) test_utils.add_complete_ingress_relation(self.harness) setup_cmds = [ - ['a2dissite', '000-default'], - ['a2ensite', 'wsgi-openstack-dashboard'], - ['python3', '/usr/share/openstack-dashboard/manage.py', 'migrate', - '--noinput']] + ["a2dissite", "000-default"], + ["a2ensite", "wsgi-openstack-dashboard"], + [ + "python3", + "/usr/share/openstack-dashboard/manage.py", + "migrate", + "--noinput", + ], + ] for cmd in setup_cmds: self.assertIn( - cmd, - self.container_calls.execute['openstack-dashboard']) + cmd, self.container_calls.execute["openstack-dashboard"] + ) self.check_file( - 'openstack-dashboard', - '/etc/apache2/sites-available/wsgi-openstack-dashboard.conf') + "openstack-dashboard", + "/etc/apache2/sites-available/wsgi-openstack-dashboard.conf", + ) self.check_file( - 'openstack-dashboard', - '/etc/openstack-dashboard/local_settings.py') + "openstack-dashboard", "/etc/openstack-dashboard/local_settings.py" + ) diff --git a/charms/horizon-k8s/tox.ini b/charms/horizon-k8s/tox.ini index ca8b7454..0e84a516 100644 --- a/charms/horizon-k8s/tox.ini +++ b/charms/horizon-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} + [coverage:run] branch = True concurrency = multiprocessing