[MISC] Enforce type checking (#146)

Batalex · web-flow · commit 7b33406f9f89 · 2026-03-02T12:48:34.000+01:00
* chore: Add type checker configuration
* chore: Type check tests
* chore: Add S3 env vars to pass_env in tox.ini
diff --git a/poetry.lock b/poetry.lock
diff --git a/pyproject.toml b/pyproject.toml
@@ -43,7 +43,10 @@ optional = true
 [tool.poetry.group.lint.dependencies]
 ruff = ">=0.14.7"
 codespell = ">=2.4.1"
-boto3-stubs = { extras = ["s3"], version = "^1.41.5" }
+mypy = "^1.17.1"
+boto3-stubs = { extras = ["s3"], version = "^1.42.0" }
+types-pyyaml = "^6.0.12.20250915"
+types-requests = "^2.32.4.20260107"
 
 [tool.poetry.group.unit]
 optional = true
@@ -91,9 +94,28 @@ extend-ignore = [
   "D413",
 ]
 ignore = ["E501", "D107"]
-per-file-ignores = { "tests/*" = ["D100", "D101", "D102", "D103", "D104", "E999"], "src/literals.py" = ["D101"] }
+per-file-ignores = { "tests/*" = ["D100", "D101", "D102", "D103", "D104", "E999"], "src/literals.py" = ["D101"], "src/*/__init__.py" = ["D104"] }
 mccabe.max-complexity = 10
 
+[tool.mypy]
+exclude = [
+  "lib",
+]
+follow_imports = "silent"
+
+[[tool.mypy.overrides]]
+module = [
+  "charms.data_platform_libs.*",
+  "charms.grafana_k8s.*",
+  "charms.loki_k8s.*",
+  "charms.prometheus_k8s.*",
+  "charms.tls_certificates_interface.*",
+  "charms.oauth2_proxy_k8s.*",
+  "charms.oathkeeper.*",
+  "charms.traefik_k8s.*",
+]
+ignore_missing_imports = true
+
 # Testing tools configuration
 [tool.coverage.run]
 branch = true
diff --git a/src/common/__init__.py b/src/common/__init__.py
diff --git a/src/core/__init__.py b/src/core/__init__.py
diff --git a/src/core/context.py b/src/core/context.py
@@ -53,7 +53,7 @@ def __init__(self, charm: CharmBase):
     def authorized_users(self) -> str | None:
         """The comma-separated list of authorized users."""
         return (
-            self.charm.config[AUTHORIZED_USERS]
+            str(self.charm.config[AUTHORIZED_USERS])
             if (self._oathkeeper_relation or self._oauth2_proxy_relation)
             else None
         )
diff --git a/src/events/__init__.py b/src/events/__init__.py
diff --git a/src/events/base.py b/src/events/base.py
@@ -70,9 +70,7 @@ def get_app_status(
         return Status.ACTIVE.value
 
 
-def compute_status(
-    hook: Callable[[BaseEventHandler, EventBase], None],
-) -> Callable[[BaseEventHandler, EventBase], None]:
+def compute_status(hook: Callable) -> Callable[[BaseEventHandler, EventBase], None]:
     """Decorator to automatically compute statuses at the end of the hook."""
 
     @wraps(hook)
@@ -100,7 +98,7 @@ def wrapper_hook(event_handler: BaseEventHandler, event: EventBase):
 
 
 def defer_when_not_ready(
-    hook: Callable[[BaseEventHandler, EventBase], None],
+    hook: Callable,
 ) -> Callable[[BaseEventHandler, EventBase], None]:
     """Decorator to automatically compute statuses at the end of the hook."""
 
diff --git a/src/events/ingress.py b/src/events/ingress.py
@@ -84,10 +84,9 @@ def _on_ingress_ready(self, event: IngressPerAppReadyEvent):
     @defer_when_not_ready
     def _on_ingress_revoked(self, _: IngressPerAppRevokedEvent):
         """Handle the `IngressPerAppRevokedEvent`."""
-        self.log_result("This app no longer has ingress")(
-            self.history_server.update(
-                self.context.s3, self.context.azure_storage, None, self.context.authorized_users
-            )
+        self.logger.info("This app no longer has ingress")
+        self.history_server.update(
+            self.context.s3, self.context.azure_storage, None, self.context.authorized_users
         )
 
         self.charm.unit.status = self.get_app_status(
diff --git a/src/managers/__init__.py b/src/managers/__init__.py
diff --git a/src/managers/history_server.py b/src/managers/history_server.py
@@ -5,8 +5,7 @@
 """History Server manager."""
 
 import os
-import re
-from urllib.parse import ParseResult, urlparse
+from urllib.parse import ParseResult, urlparse, urlunparse
 
 from common.utils import WithLogging, is_proxy_skipped
 from core.context import (
@@ -26,8 +25,6 @@
 class HistoryServerConfig(WithLogging):
     """Class representing the Spark Properties configuration file."""
 
-    _ingress_pattern = re.compile("http://.*?/|https://.*?/")
-
     _base_conf: dict[str, str] = {
         "spark.hadoop.fs.s3a.path.style.access": "true",
         "spark.eventLog.enabled": "true",
@@ -60,13 +57,14 @@ def _ingress_proxy_conf(self) -> dict[str, str]:
         if not self.ingress:
             return {}
 
-        # Get DNS and skip the trailing /
-        proxy_dns = self._ingress_pattern.match(f"{self.ingress.url}/").group()[:-1]
-        proxy_base = str(self.ingress.url).removeprefix(proxy_dns)
+        parsed_ingress = urlparse(self.ingress.url)
+        redirect_uri = urlunparse((parsed_ingress.scheme, parsed_ingress.netloc, "", "", "", ""))
+        ingress_properties = {"spark.ui.proxyRedirectUri": redirect_uri}
 
-        return {"spark.ui.proxyRedirectUri": proxy_dns} | (
-            {"spark.ui.proxyBase": proxy_base} if proxy_base != "/" else {}
-        )
+        if base := parsed_ingress.path.strip("/"):
+            ingress_properties["spark.ui.proxyBase"] = base
+
+        return ingress_properties
 
     @property
     def _s3_conf(self) -> dict[str, str]:
diff --git a/src/workload.py b/src/workload.py
@@ -39,7 +39,7 @@ def __init__(self, container: Container, user: User):
             jmx_exporter=self.jmx_file,
         )
 
-        self._envs = None
+        self._envs: dict[str, str] | None = None
 
     @property
     def jmx_file(self):
diff --git a/tests/integration/oauth_tools/external_idp.py b/tests/integration/oauth_tools/external_idp.py
@@ -73,7 +73,7 @@ def update_redirect_uri(self, redirect_uri: str) -> None:
         ...
 
     @abc.abstractmethod
-    def complete_user_login(self, page: Page) -> None:
+    async def complete_user_login(self, page: Page) -> None:
         """Get a page on the IDP login page and login the user."""
         ...
 
@@ -121,7 +121,7 @@ def _get_dex_manifest(self) -> list[codecs.AnyResource]:
         except ApiError:
             logger.info("No service found for identity provider")
 
-        temp_redirect_url = self._redirect_uri
+        temp_redirect_url: str | None = self._redirect_uri
         if not temp_redirect_url:
             temp_redirect_url = None
 
diff --git a/tests/integration/test_helpers.py b/tests/integration/test_helpers.py
@@ -63,18 +63,21 @@ def prometheus_exporter_data(host: str) -> str | None:
         logger.info(f"Response: {response.text}")
         print(response)
     except requests.exceptions.RequestException:
-        return
+        return None
+
     if response.status_code == 200:
         return response.text
 
+    return None
+
 
 def all_prometheus_exporters_data(juju: jubilant.Juju, check_field) -> bool:
     """Check if a all units has metric service available and publishing."""
     result = True
     status = juju.status()
     for unit in status.apps[APP_NAME].units.values():
         unit_ip = unit.address
-        result = result and check_field in prometheus_exporter_data(unit_ip)
+        result = result and check_field in (prometheus_exporter_data(unit_ip) or "")
     return result
 
 
@@ -86,11 +89,13 @@ def published_prometheus_alerts(juju: jubilant.Juju, host: str) -> dict | None:
     try:
         response = requests.get(url)
     except requests.exceptions.RequestException:
-        return
+        return None
 
     if response.status_code == 200:
         return response.json()
 
+    return None
+
 
 def published_prometheus_data(juju: jubilant.Juju, host: str, field: str) -> dict | None:
     """Check the existence of field among Prometheus published data."""
@@ -100,11 +105,13 @@ def published_prometheus_data(juju: jubilant.Juju, host: str, field: str) -> dic
     try:
         response = requests.get(url)
     except requests.exceptions.RequestException:
-        return
+        return None
 
     if response.status_code == 200:
         return response.json()
 
+    return None
+
 
 def published_grafana_dashboards(juju: jubilant.Juju) -> dict | None:
     """Get the list of dashboards published to Grafana."""
@@ -116,10 +123,13 @@ def published_grafana_dashboards(juju: jubilant.Juju) -> dict | None:
         session.auth = ("admin", pw)
         response = session.get(url)
     except requests.exceptions.RequestException:
-        return
+        return None
+
     if response.status_code == 200:
         return response.json()
 
+    return None
+
 
 def get_cos_address(juju: jubilant.Juju) -> str:
     """Retrieve the URL where COS services are available."""
diff --git a/tests/integration/test_oauth2proxy.py b/tests/integration/test_oauth2proxy.py
@@ -68,7 +68,7 @@ async def get_cookie_from_browser_by_name(
 
 async def complete_auth_code_login(
     page: Page,
-    external_idp_service: Optional[ExternalIdpService],
+    external_idp_service: ExternalIdpService,
 ) -> None:
     """Take a page that is in the identity-platform's login page and login the user.
 
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
@@ -395,11 +395,9 @@ def test_azure_relation_no_path_ko(
     assert out.unit_status == Status.MISSING_STORAGE_PATH.value
 
 
-@patch("managers.s3.S3Manager.verify", return_value=True)
 @patch("workload.SparkHistoryServer.exec")
 def test_azure_storage_relation_broken(
     exec_calls,
-    verify_call,
     tmp_path: Path,
     history_server_ctx: Context[SparkHistoryServerCharm],
     history_server_container: Container,
@@ -442,6 +440,7 @@ def test_both_azure_storage_and_s3_relation_together(
     s3_relation: Relation,
     azure_storage_relation: Relation,
 ) -> None:
+    # Patching verify speeds up the test
     state = State(
         relations=[s3_relation, azure_storage_relation],
         containers=[history_server_container],
diff --git a/tox.ini b/tox.ini
@@ -4,7 +4,17 @@
 [tox]
 no_package = True
 skip_missing_interpreters = True
-env_list = lint, unit
+env_list =
+    refresh
+    format
+    lint
+    unit
+    integration-charm
+    integration-tls
+    integration-azure
+    integration-logs
+    integration-auth
+    integration-oathkeeper
 
 [vars]
 application = spark-history-server-k8s
@@ -64,8 +74,21 @@ commands =
         --skip {tox_root}/.mypy_cache \
         --skip {tox_root}/icon.svg
 
-    poetry run ruff check {[vars]all_path}
-    poetry run ruff format --check --diff {[vars]all_path}
+    poetry run ruff check \
+        --config pyproject.toml \
+        --exclude {tox_root}/lib/charms/data_platform_libs \
+        --exclude {tox_root}/tests/integration/app-charm/lib \
+        --fix {[vars]all_path}
+    poetry run ruff format \
+        --config pyproject.toml \
+        --exclude {tox_root}/lib/charms/data_platform_libs \
+        --exclude {tox_root}/tests/integration/app-charm/lib \
+        --check {[vars]all_path}
+
+    poetry install --with lint
+    poetry run mypy src
+    poetry run mypy tests/integration --ignore-missing-imports
+
 
 [testenv:unit]
 description = Run unit tests
@@ -87,6 +110,9 @@ pass_env =
     CI
     AZURE_STORAGE_ACCOUNT
     AZURE_STORAGE_KEY
+    S3_ACCESS_KEY
+    S3_SECRET_KEY
+    S3_SERVER_URL
 commands =
     poetry install --with integration
     playwright install

Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,7 @@ def __init__(self, charm: CharmBase):`
`53`	`53`	`def authorized_users(self) -> str \| None:`
`54`	`54`	`"""The comma-separated list of authorized users."""`
`55`	`55`	`return (`
`56`		`- self.charm.config[AUTHORIZED_USERS]`
	`56`	`+ str(self.charm.config[AUTHORIZED_USERS])`
`57`	`57`	`if (self._oathkeeper_relation or self._oauth2_proxy_relation)`
`58`	`58`	`else None`
`59`	`59`	`)`