pyright: wire up CI/CD by delgod · Pull Request #57 · canonical/valkey-operator

delgod · 2026-05-13T13:10:41Z

Summary

Adds a working static tox environment that runs pyright against src/, completing the stub previously referenced by env_list = format, lint, static, unit in tox.ini
(which had no matching [testenv:static] section).
Declares pyright in a new optional Poetry group static so the type checker is reproducible via the lockfile rather than ambient.
Adds a static job to .github/workflows/ci.yaml that mirrors the unit-test setup (libprotobuf for valkey-glide) and runs tox run -e static.

Why

The repo already had a [tool.pyright] block in pyproject.toml (include = ["src//*.py", "lib//*.py"]) but nothing actually invoked pyright — no dev dependency, no tox
env, no CI job. Ruff handles lint/format but does not type-check (F rules catch undefined names, not type mismatches). This wires the existing config into the standard
developer and CI flow so type regressions become enforceable.

Changes

pyproject.toml: new [tool.poetry.group.static] (optional) with pyright = "*".
poetry.lock: regenerated; adds pyright 1.1.409 and its nodeenv transitive dep into the static group; typing-extensions now also belongs to static.
tox.ini: new [testenv:static] — installs main,charm-libs,unit,static groups and runs poetry run pyright {src_path}. Installing the runtime + unit groups gives pyright
the imports it needs to resolve symbols (ops, charmlibs, valkey, lightkube, pytest, etc.); without them most files would surface as unresolved-import errors.
.github/workflows/ci.yaml: new static job, ubuntu-latest, 5 min timeout, installs libprotobuf-dev/protobuf-compiler to match unit-test (needed because valkey-glide is
built from source), then tox run -e static.

The env was declared in tox.ini's env_list but the [testenv:static] block was missing. Add the block, a new optional poetry group with pyright, and a static CI job mirroring unit-test. Catches kwarg-name typos and similar regressions that ruff does not flag. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Both workflows triggered only on PRs targeting main, but this repo ships from 9/edge -- so PRs to release branches skipped CLA verification (a legal-risk gap) and removed-URL detection. Extend the branch filter to "*/edge". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Address all 49 pyright errors so `tox -e static` passes cleanly: - guard Optional lightkube .spec/.metadata and Juju relations before use - cast ops config values (bool|int|float|str) to str where strings are expected, matching the existing pattern - raise clear errors when the internal CA cert/key or k8s client are unexpectedly missing instead of dereferencing None - accept an Optional private key in TLSManager.write_certificate, keeping the existing key file when none is provided (preserves prior behaviour) - cast library returns to their concrete runtime types where the upstream signatures are too loose (ResourceProviderModel, certificate relation_id) - narrow the workload to the VM implementation for the snap-only install() - scope `# pyright: ignore[reportIncompatibleVariableOverride]` to the manager `state` assignments and the intended `on` CharmEvents override, which are sound but flagged due to mutable-attribute invariance Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Mehdi-Bendriss

Thanks Mykola, I left some comments

Mehdi-Bendriss · 2026-05-22T12:18:27Z

    def bind_address(self) -> str:
        """The network binding address from the peer relation."""
-        if not (binding := self.model.get_binding(self.peer_relation)):
+        if not (peer_relation := self.peer_relation):


is this for linting or did you face this during testing?

fixed in 5602d27

Same case. The only reason None is not considered is because the places we call the property are code paths that happen after the peer relation has been established. If we want to adopt typing 100% this is a valid check.

@skourta the code paths should be predictable, we need to be able to know at any given time what is the expected state of our system. Being overly defensive masks that and makes reading and reasoning about code hard, and may allow things to fail silently (if defensive too gracefully, like (not) doing an operation conditionally etc)

Address Mehdi's PR #57 review: where type-checker errors were resolved by adding runtime behavior (silent skips, redundant guards, an unneeded cast), restore the original code and silence pyright with a targeted ignore. - k8s_client: drop `service.spec and` guard (was failing silently) - cluster_state: drop redundant peer_relation None-raise in bind/ingress_address - base_events: drop isinstance(ValkeyVmWorkload) guard around install() - external_clients: drop unreachable `relation is None` return; drop relation_id cast - config: drop overly-defensive sentinel-section isinstance fallback write_certificate's `private_key: PrivateKey | None` is left as-is: the None case is real (get_assigned_certificates returns PrivateKey | None and unit tests exercise it), so reverting it would reintroduce a latent crash. tox -e static / lint / unit all pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Mehdi clarified that in the charm flow the private key is always present when _on_certificate_available fires: the certificate_available hook can't arrive without a CSR, and a CSR requires a private key (user-provided or generated by the lib). get_assigned_certificates' PrivateKey | None return is the lib being defensive for other callers. Restore the required PrivateKey signature and original body so a missing key crashes on write instead of being silently skipped. The call site passes the lib's PrivateKey | None, so suppress the type error there with a targeted pyright ignore rather than adding a runtime check. Unit tests mock write_certificate, so they remain green; tox -e static / lint / unit all pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

skourta

I really like the direction and intent of this PR. However, it brings up a larger architectural question: should we enforce 100% typing, or continue with partial adoption? I'm a bit hesitant about merging multiple pyright ignores, as it can make the codebase feel inconsistent and slightly harder to maintain. I think we need to discuss the best path forward for our typing standards.

skourta · 2026-05-26T06:11:51Z

    def bind_address(self) -> str:
        """The network binding address from the peer relation."""
-        if not (binding := self.model.get_binding(self.peer_relation)):
+        if not (peer_relation := self.peer_relation):


Same case. The only reason None is not considered is because the places we call the property are code paths that happen after the peer relation has been established. If we want to adopt typing 100% this is a valid check.

reneradoi

Thank you Mykola for the initivative! I've got only a few minor comments, but looks good otherwise.

reneradoi · 2026-05-26T06:52:49Z

        server_info = client.info_server(hostname=self.state.endpoint)

-        return server_info.get("valkey_version")
+        return server_info["valkey_version"]


The previous code was safe against KeyError, after the change this could raise and break the charm hook in case the version is not returned from the client command. I would rather keep it as it was previously.

reneradoi · 2026-05-26T06:55:27Z

+        if self.k8s_client is None:
+            raise KubernetesClientError("K8s client is only available on the Kubernetes substrate")
+


No need for this safeguard, we check on substrate on event handler level.

reneradoi · 2026-05-26T06:55:38Z

+        if self.k8s_client is None:
+            raise KubernetesClientError("K8s client is only available on the Kubernetes substrate")
+


Same comment: No need for this safeguard, we check on substrate on event handler level.

reneradoi · 2026-05-26T06:56:57Z

            extra_sans_config := self.state.config.get("certificate-extra-sans")
        ):
-            extra_sans = [san.strip() for san in extra_sans_config.split(",")]
+            extra_sans = [san.strip() for san in str(extra_sans_config).split(",")]


The config for extra_sans is defined as string in config.yaml, no need to cast.

reneradoi · 2026-05-26T06:57:06Z

            extra_sans_config := self.state.config.get("certificate-extra-sans")
        ):
-            extra_sans = [san.strip() for san in extra_sans_config.split(",")]
+            extra_sans = [san.strip() for san in str(extra_sans_config).split(",")]


Same comment: The config for extra_sans is defined as string in config.yaml, no need to cast.

Mehdi-Bendriss

Thanks Mykola

delgod force-pushed the pyright branch 2 times, most recently from 0810789 to 2181fb5 Compare May 15, 2026 13:06

delgod and others added 3 commits May 22, 2026 10:21

delgod force-pushed the pyright branch from fb0c0a5 to 9dbe655 Compare May 22, 2026 10:21

Mehdi-Bendriss reviewed May 22, 2026

View reviewed changes

delgod and others added 2 commits May 22, 2026 16:38

skourta reviewed May 26, 2026

View reviewed changes

reneradoi reviewed May 26, 2026

View reviewed changes

Mehdi-Bendriss approved these changes May 26, 2026

View reviewed changes

		if self.k8s_client is None:
		raise KubernetesClientError("K8s client is only available on the Kubernetes substrate")

Conversation

delgod commented May 13, 2026

Summary

Why

Changes

Uh oh!

Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mehdi-Bendriss May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skourta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reneradoi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mehdi-Bendriss May 26, 2026 •

edited

Loading