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
2022-11-04 10:02:39 +05:30 · 2022-11-04 10:02:39 +05:30 · b65c3e31e4
commit b65c3e31e4
parent d703a24e3b
13 changed files with 183 additions and 117 deletions
--- a/charms/horizon-k8s/pyproject.toml
+++ 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"
--- 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
--- 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)
--- 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
--- a/charms/horizon-k8s/src/templates/parts/identity-data
+++ b/charms/horizon-k8s/src/templates/parts/identity-data
@ -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 }}
--- a/charms/horizon-k8s/src/templates/parts/section-database
+++ b/charms/horizon-k8s/src/templates/parts/section-database
@ -1,3 +0,0 @@
-[database]
-{% include "parts/database-connection" %}
-connection_recycle_time = 200
--- a/charms/horizon-k8s/src/templates/parts/section-federation
+++ b/charms/horizon-k8s/src/templates/parts/section-federation
@ -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 -%}
--- a/charms/horizon-k8s/src/templates/parts/section-identity
+++ b/charms/horizon-k8s/src/templates/parts/section-identity
@ -1,2 +0,0 @@
-[keystone_authtoken]
-{% include "parts/identity-data" %}
--- a/charms/horizon-k8s/src/templates/parts/section-middleware
+++ b/charms/horizon-k8s/src/templates/parts/section-middleware
@ -1,6 +0,0 @@
-{% for section in sections -%}
-[{{section}}]
-{% for key, value in sections[section].items() -%}
-{{ key }} = {{ value }}
-{% endfor %}
-{%- endfor %}
--- a/charms/horizon-k8s/src/templates/parts/section-signing
+++ b/charms/horizon-k8s/src/templates/parts/section-signing
@ -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 -%}
--- 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."""
--- 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"
+        )
--- 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