From 80cd7eaf7f925613d08e3149a98e85673c35f00c Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 26 Aug 2026 18:16:51 +0300 Subject: [PATCH 01/16] feat(resource)!: require DynamicClient client BREAKING CHANGE: client is mandatory on Resource/NamespacedResource/Event APIs. Remove dyn_client and config_file/config_dict/context from Resource APIs; build clients via get_client() instead. Co-authored-by: Cursor --- AGENTS.md | 4 +- examples/validation_demo.py | 16 +++- examples/validation_troubleshooting.py | 12 ++- ocp_resources/event.py | 40 +++----- ocp_resources/resource.py | 124 +++++++++---------------- tests/test_resource.py | 67 +++++++++++++ 6 files changed, 151 insertions(+), 112 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6683132a6d..88a709fa01 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -58,14 +58,16 @@ Code between markers is auto-generated — do NOT modify manually. Use `class-ge ## When Writing Code +- **Always pass `client` when instantiating a resource** — `client: DynamicClient` is required on `Resource` / `NamespacedResource` (and on `.get()` / `Event` APIs). Create it with `get_client()` (or `get_client(fake=True)` in tests); do not rely on implicit kubeconfig/client creation. - No client-side validation in `to_dict()` — let the K8s/OCP API server return errors. Helper functions in resource classes CAN validate. -- Context managers for auto-cleanup: `with Pod(name="test", namespace="default") as pod:` +- Context managers for auto-cleanup: `with Pod(client=client, name="test", namespace="default") as pod:` - Wait utilities: `wait_for_status()`, `wait_for_condition()` via timeout-sampler - Sensitive data: add keys to `keys_to_hash` property for automatic log hashing - Fake client for tests: `get_client(fake=True)` — no cluster required ## When Reviewing Code +- Verify resources are constructed with an explicit `client=` (and `.get()` / `Event` calls pass `client`) - Verify generated code markers are intact — no manual edits between markers - Check type hints on all new functions/parameters - Verify tests exist for new helper methods (not needed for generated `__init__`/`to_dict`) diff --git a/examples/validation_demo.py b/examples/validation_demo.py index a03407f06a..b78cd87ea7 100644 --- a/examples/validation_demo.py +++ b/examples/validation_demo.py @@ -15,6 +15,8 @@ from ocp_resources.resource import get_client from ocp_resources.service import Service +FAKE_CLIENT = get_client(fake=True) + def print_section(title): """Print a section header""" @@ -30,6 +32,7 @@ def demo_basic_validation(): # Create a valid pod print("1. Creating a valid pod and validating...") pod = Pod( + client=FAKE_CLIENT, name="nginx-pod", namespace="default", containers=[{"name": "nginx", "image": "nginx:latest", "ports": [{"containerPort": 80}]}], @@ -44,6 +47,7 @@ def demo_basic_validation(): # Create an invalid pod (missing required fields) print("\n2. Creating an invalid pod (missing image)...") invalid_pod = Pod( + client=FAKE_CLIENT, name="invalid-pod", namespace="default", containers=[{"name": "nginx"}], # Missing required 'image' field @@ -61,7 +65,7 @@ def demo_auto_validation(): print_section(title="Auto-validation During Create") # Get a fake client for demo - client = get_client(fake=True) + client = FAKE_CLIENT # Create pod with auto-validation enabled print("1. Creating pod with auto-validation enabled...") @@ -141,6 +145,7 @@ def demo_different_resources(): # Service validation print("1. Validating a Service...") service = Service( + client=FAKE_CLIENT, name="nginx-service", namespace="default", selector={"app": "nginx"}, @@ -156,6 +161,7 @@ def demo_different_resources(): # ConfigMap validation print("\n2. Validating a ConfigMap...") config_map = ConfigMap( + client=FAKE_CLIENT, name="app-config", namespace="default", data={"app.properties": "debug=true\nport=8080", "database.url": "postgresql://localhost:5432/mydb"}, @@ -176,7 +182,12 @@ def demo_performance(): pods = [] for i in range(5): pods.append( - Pod(name=f"perf-test-pod-{i}", namespace="default", containers=[{"name": "nginx", "image": "nginx:latest"}]) + Pod( + client=FAKE_CLIENT, + name=f"perf-test-pod-{i}", + namespace="default", + containers=[{"name": "nginx", "image": "nginx:latest"}], + ) ) # First validation (loads and caches schema) @@ -208,6 +219,7 @@ def demo_error_details(): # Create pod with multiple errors print("Creating a pod with multiple validation errors...") pod = Pod( + client=FAKE_CLIENT, name="invalid-name-", # Names must start and end with alphanumeric, can only contain lowercase alphanumeric or hyphens namespace="default", containers=[ diff --git a/examples/validation_troubleshooting.py b/examples/validation_troubleshooting.py index 39934e8d37..37e80e97e1 100644 --- a/examples/validation_troubleshooting.py +++ b/examples/validation_troubleshooting.py @@ -9,8 +9,11 @@ from ocp_resources.deployment import Deployment from ocp_resources.exceptions import ValidationError from ocp_resources.pod import Pod +from ocp_resources.resource import get_client from ocp_resources.service import Service +FAKE_CLIENT = get_client(fake=True) + def print_error_case(title, description): """Print an error case header""" @@ -35,6 +38,7 @@ def case_1_missing_required_fields(): print("Problem code:") print(""" pod = Pod( + client=client, name="my-pod", namespace="default", containers=[{"name": "nginx"}] # Missing 'image' @@ -42,7 +46,7 @@ def case_1_missing_required_fields(): """) try: - pod = Pod(name="my-pod", namespace="default", containers=[{"name": "nginx"}]) + pod = Pod(client=FAKE_CLIENT, name="my-pod", namespace="default", containers=[{"name": "nginx"}]) pod.validate() except ValidationError as e: print(f"Error: {e}") @@ -52,6 +56,7 @@ def case_1_missing_required_fields(): print("Fixed code:") print(""" pod = Pod( + client=client, name="my-pod", namespace="default", containers=[{ @@ -119,6 +124,7 @@ def case_3_invalid_field_values(): print("Problem code:") print(""" pod = Pod( + client=client, name="My-Pod-123", # Capital letters not allowed namespace="default" ) @@ -126,6 +132,7 @@ def case_3_invalid_field_values(): try: pod = Pod( + client=FAKE_CLIENT, name="My-Pod-123", # Invalid DNS name namespace="default", containers=[{"name": "nginx", "image": "nginx:latest"}], @@ -139,6 +146,7 @@ def case_3_invalid_field_values(): print("Fixed code:") print(""" pod = Pod( + client=client, name="my-pod-123", # Valid DNS name namespace="default" ) @@ -155,6 +163,7 @@ def case_4_invalid_structure(): print("Problem code:") print(""" service = Service( + client=client, name="my-service", namespace="default", selector={"app": "nginx"}, @@ -182,6 +191,7 @@ def case_4_invalid_structure(): print("Fixed code:") print(""" service = Service( + client=client, name="my-service", namespace="default", selector={"app": "nginx"}, diff --git a/ocp_resources/event.py b/ocp_resources/event.py index c74f1afde9..35561c436c 100644 --- a/ocp_resources/event.py +++ b/ocp_resources/event.py @@ -1,4 +1,3 @@ -import warnings from collections.abc import Generator from datetime import UTC, datetime, timedelta from typing import Any @@ -16,29 +15,10 @@ class Event: api_version = "v1" - # TODO: remove once `client` is mandatory - @staticmethod - def _resolve_client( - client: DynamicClient | None, - dyn_client: DynamicClient | None, - ) -> DynamicClient: - """Resolve client from new or deprecated parameter with deprecation warning.""" - if client is None and dyn_client is not None: - warnings.warn( - "`dyn_client` arg will be renamed to `client` and will be mandatory in the next major release.", - FutureWarning, - stacklevel=3, # Adjusted for helper function call - ) - - resolved = client or dyn_client - assert resolved is not None, "Either 'client' or 'dyn_client' must be provided" - return resolved - @classmethod def get( cls, - client: DynamicClient | None = None, # TODO: make mandatory in the next major release - dyn_client: DynamicClient | None = None, # TODO: remove in the next major release + client: DynamicClient, namespace: str | None = None, name: str | None = None, label_selector: str | None = None, @@ -51,7 +31,6 @@ def get( Args: client (DynamicClient): K8s client - dyn_client (DynamicClient): K8s client namespace (str): event namespace name (str): event name label_selector (str): filter events by labels; comma separated string of key=value @@ -72,7 +51,8 @@ def get( ): print(event.object) """ - _client = cls._resolve_client(client, dyn_client) + if client is None: + raise TypeError("client is required") LOGGER.info("Reading events") LOGGER.debug( @@ -81,7 +61,7 @@ def get( f" resource_version={resource_version}, timeout={timeout}" ) - event_listener = _client.resources.get(api_version=cls.api_version, kind=cls.__name__) + event_listener = client.resources.get(api_version=cls.api_version, kind=cls.__name__) yield from event_listener.watch( namespace=namespace, name=name, @@ -137,6 +117,9 @@ def list( field_selector="type==Warning", ) """ + if client is None: + raise TypeError("client is required") + if since_seconds < 0: raise ValueError("since_seconds must be >= 0") @@ -172,8 +155,7 @@ def list( @classmethod def delete_events( cls, - client: DynamicClient | None = None, # TODO: make mandatory in the next major release - dyn_client: DynamicClient | None = None, # TODO: remove in the next major release + client: DynamicClient, namespace: str | None = None, name: str | None = None, label_selector: str | None = None, @@ -187,7 +169,6 @@ def delete_events( Args: client (DynamicClient): K8s client - dyn_client (DynamicClient): K8s client namespace (str): event namespace name (str): event name label_selector (str): filter events by labels; comma separated string of key=value @@ -200,7 +181,8 @@ def delete_events( def delete_events_before_test(client): Event.delete_events(client=client, namespace="my-namespace", field_selector="reason=AnEventReason") """ - _client = cls._resolve_client(client, dyn_client) + if client is None: + raise TypeError("client is required") LOGGER.info("Deleting events") LOGGER.debug( @@ -209,7 +191,7 @@ def delete_events_before_test(client): f" resource_version={resource_version}, timeout={timeout}" ) - _client.resources.get(api_version=cls.api_version, kind=cls.__name__).delete( + client.resources.get(api_version=cls.api_version, kind=cls.__name__).delete( namespace=namespace, name=name, label_selector=label_selector, diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index 86e0d3a79d..0d7664631b 100644 --- a/ocp_resources/resource.py +++ b/ocp_resources/resource.py @@ -6,7 +6,6 @@ import re import sys import threading -import warnings from abc import ABC, abstractmethod from collections.abc import Callable, Generator from io import StringIO @@ -193,6 +192,19 @@ def _exchange_code_for_token( ) +_REMOVED_CLIENT_KWARGS: frozenset[str] = frozenset({"dyn_client", "config_file", "config_dict", "context"}) + + +def _reject_removed_client_kwargs(kwargs: dict[str, Any]) -> None: + """Fail fast if deprecated client-config kwargs are passed via **kwargs.""" + removed = _REMOVED_CLIENT_KWARGS.intersection(kwargs) + if removed: + names = ", ".join(sorted(removed)) + raise TypeError( + f"Unsupported argument(s): {names}. Pass a DynamicClient via client= from get_client() instead." + ) + + def get_client( config_file: str | None = None, config_dict: dict[str, Any] | None = None, @@ -619,7 +631,7 @@ class ApiVersion: def __init__( self, - client: DynamicClient | None = None, # TODO: make mandatory in the next major release + client: DynamicClient, name: str | None = None, teardown: bool = True, yaml_file: str | None = None, @@ -627,9 +639,6 @@ def __init__( dry_run: bool = False, node_selector: dict[str, Any] | None = None, node_selector_labels: dict[str, str] | None = None, - config_file: str | None = None, - config_dict: dict[str, Any] | None = None, - context: str | None = None, label: dict[str, str] | None = None, annotations: dict[str, str] | None = None, api_group: str = "", @@ -645,16 +654,14 @@ def __init__( If `yaml_file` or `kind_dict` are passed, logic in `to_dict` is bypassed. Args: - name (str): Resource name client (DynamicClient): Dynamic client for connecting to a remote cluster + name (str): Resource name teardown (bool): Indicates if this resource would need to be deleted yaml_file (str): yaml file for the resource delete_timeout (int): timeout associated with delete action dry_run (bool): dry run node_selector (dict): node selector node_selector_labels (str): node selector labels - config_file (str): Path to config file for connecting to remote cluster. - context (str): Context name for connecting to remote cluster. label (dict): Resource labels annotations (dict[str, str] | None): Resource annotations api_group (str): Resource API group; will overwrite API group definition in resource class @@ -669,6 +676,9 @@ def __init__( if yaml_file and kind_dict: raise ValueError("yaml_file and resource_dict are mutually exclusive") + if client is None: + raise TypeError("client is required") + self.name = name self.teardown = teardown self.yaml_file = yaml_file @@ -677,19 +687,9 @@ def __init__( self.dry_run = dry_run self.node_selector = node_selector self.node_selector_labels = node_selector_labels - self.config_file = config_file - self.config_dict = config_dict or {} - self.context = context self.label = label self.annotations = annotations - if not client: - warnings.warn( - "'client' arg will be mandatory in the next major release. " - "`config_file` and `context` args will be removed.", - FutureWarning, - stacklevel=2, - ) - self.client: DynamicClient = client or get_client(config_file=self.config_file, context=self.context) + self.client = client self.api_group: str = api_group or self.api_group self.hash_log_data = hash_log_data @@ -1185,14 +1185,11 @@ def retry_cluster_exceptions( @classmethod def get( cls, - client: DynamicClient | None = None, # TODO: make mandatory in the next major release - dyn_client: DynamicClient | None = None, # TODO: remove in the next major release - config_file: str = "", + client: DynamicClient, + *, singular_name: str = "", exceptions_dict: dict[type[Exception], list[str]] = DEFAULT_CLUSTER_RETRY_EXCEPTIONS, raw: bool = False, - context: str | None = None, - *args: Any, **kwargs: Any, ) -> Generator[Any, None, None]: """ @@ -1200,9 +1197,6 @@ def get( Args: client (DynamicClient): k8s client - dyn_client (DynamicClient): Open connection to remote cluster. - config_file (str): Path to config file for connecting to remote cluster. - context (str): Context name for connecting to remote cluster. singular_name (str): Resource kind (in lowercase), in use where we have multiple matches for resource. raw (bool): If True return raw object. exceptions_dict (dict): Exceptions dict for TimeoutSampler @@ -1210,31 +1204,24 @@ def get( Returns: generator: Generator of Resources of cls.kind. """ - _client = client or dyn_client - - if not _client: - warnings.warn( - "`dyn_client` arg will be renamed to `client` and will be mandatory in the next major release. " - "`config_file` and `context` will be removed.", - FutureWarning, - stacklevel=2, - ) - _client = get_client(config_file=config_file, context=context) + if client is None: + raise TypeError("client is required") + _reject_removed_client_kwargs(kwargs) def _get() -> Generator["Resource|ResourceInstance", None, None]: - _resources = cls._prepare_resources(*args, client=_client, singular_name=singular_name, **kwargs) # type: ignore[misc] + _resources = cls._prepare_resources(client=client, singular_name=singular_name, **kwargs) try: for resource_field in _resources.items: if raw: yield _resources else: - yield cls(client=_client, name=resource_field.metadata.name) + yield cls(client=client, name=resource_field.metadata.name) except TypeError: if raw: yield _resources else: - yield cls(client=_client, name=_resources.metadata.name) + yield cls(client=client, name=_resources.metadata.name) return Resource.retry_cluster_exceptions(func=_get, exceptions_dict=exceptions_dict) @@ -1445,10 +1432,7 @@ def events( @staticmethod def get_all_cluster_resources( - client: DynamicClient | None = None, # TODO: make mandatory in the next major release - config_file: str = "", - context: str | None = None, - config_dict: dict[str, Any] | None = None, + client: DynamicClient, *args: Any, **kwargs: Any, ) -> Generator[ResourceField, None, None]: @@ -1457,31 +1441,26 @@ def get_all_cluster_resources( Args: client (DynamicClient): k8s client - config_file (str): path to a kubeconfig file. - config_dict (dict): dict with kubeconfig configuration. - context (str): name of the context to use. - *args (tuple): args to pass to client.get() **kwargs (dict): kwargs to pass to client.get() Yields: kubernetes.dynamic.resource.ResourceField: Cluster resource. Example: - for resource in get_all_cluster_resources(label_selector="my-label=value"): + for resource in get_all_cluster_resources(client=client, label_selector="my-label=value"): print(f"Resource: {resource}") """ - if not client: - warnings.warn( - "'client' arg will be mandatory in the next major release. " - "`config_file`, `config_dict` and `context` will be removed.", - FutureWarning, - stacklevel=2, + if client is None: + raise TypeError("client is required") + if args: + raise TypeError( + "get_all_cluster_resources() takes no positional arguments after client; use keyword arguments only" ) - client = get_client(config_file=config_file, config_dict=config_dict, context=context) + _reject_removed_client_kwargs(kwargs) for _resource in client.resources.search(): try: - _resources = client.get(_resource, *args, **kwargs) + _resources = client.get(_resource, **kwargs) yield from _resources.items except (NotFoundError, TypeError, MethodNotAllowedError): @@ -1629,12 +1608,12 @@ class NamespacedResource(Resource): def __init__( self, + client: DynamicClient, name: str | None = None, namespace: str | None = None, teardown: bool = True, yaml_file: str | None = None, delete_timeout: int = TIMEOUT_4MINUTES, - client: DynamicClient | None = None, ensure_exists: bool = False, **kwargs: Any, ): @@ -1656,14 +1635,11 @@ def __init__( @classmethod def get( cls, - client: DynamicClient | None = None, # TODO: make mandatory in the next major release - dyn_client: DynamicClient | None = None, # TODO: remove in the next major release - config_file: str = "", + client: DynamicClient, + *, singular_name: str = "", exceptions_dict: dict[type[Exception], list[str]] = DEFAULT_CLUSTER_RETRY_EXCEPTIONS, raw: bool = False, - context: str | None = None, - *args: Any, **kwargs: Any, ) -> Generator[Any, None, None]: """ @@ -1671,9 +1647,6 @@ def get( Args: client (DynamicClient): k8s client - dyn_client (DynamicClient): Open connection to remote cluster - config_file (str): Path to config file for connecting to remote cluster. - context (str): Context name for connecting to remote cluster. singular_name (str): Resource kind (in lowercase), in use where we have multiple matches for resource. raw (bool): If True return raw object. exceptions_dict (dict): Exceptions dict for TimeoutSampler @@ -1681,26 +1654,19 @@ def get( Returns: generator: Generator of Resources of cls.kind """ - _client = client or dyn_client - - if not _client: - warnings.warn( - "`dyn_client` arg will be renamed to `client` and will be mandatory in the next major release. " - "`config_file` and `context` will be removed.", - FutureWarning, - stacklevel=2, - ) - _client = get_client(config_file=config_file, context=context) + if client is None: + raise TypeError("client is required") + _reject_removed_client_kwargs(kwargs) def _get() -> Generator["NamespacedResource|ResourceInstance", None, None]: - _resources = cls._prepare_resources(*args, client=_client, singular_name=singular_name, **kwargs) # type: ignore[misc] + _resources = cls._prepare_resources(client=client, singular_name=singular_name, **kwargs) try: for resource_field in _resources.items: if raw: yield resource_field else: yield cls( - client=_client, + client=client, name=resource_field.metadata.name, namespace=resource_field.metadata.namespace, ) @@ -1709,7 +1675,7 @@ def _get() -> Generator["NamespacedResource|ResourceInstance", None, None]: yield _resources else: yield cls( - client=_client, + client=client, name=_resources.metadata.name, namespace=_resources.metadata.namespace, ) diff --git a/tests/test_resource.py b/tests/test_resource.py index bd409eca9e..6cdb7bfc5a 100644 --- a/tests/test_resource.py +++ b/tests/test_resource.py @@ -5,6 +5,7 @@ import pytest import yaml +from ocp_resources.event import Event from ocp_resources.exceptions import ResourceTeardownError from ocp_resources.namespace import Namespace from ocp_resources.pod import Pod @@ -96,6 +97,72 @@ def test_get_all_cluster_resources(self, fake_client): if _resources: break + def test_client_is_required(self, fake_client): + with pytest.raises(TypeError, match="client"): + Pod(name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) + + with pytest.raises(TypeError, match="client"): + Pod(client=None, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) # type: ignore[arg-type] + + with pytest.raises(TypeError, match="client"): + list(Namespace.get()) + + with pytest.raises(TypeError, match="client"): + list(Namespace.get(client=None)) # type: ignore[arg-type] + + with pytest.raises(TypeError, match="client"): + list(Resource.get_all_cluster_resources()) + + with pytest.raises(TypeError, match="client"): + list(Resource.get_all_cluster_resources(client=None)) # type: ignore[arg-type] + + # Valid construction still works with an explicit client + pod = Pod(client=fake_client, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) + assert pod.client is fake_client + + def test_removed_client_kwargs_rejected(self, fake_client): + with pytest.raises(TypeError, match="Unsupported argument"): + list(Namespace.get(client=fake_client, dyn_client=fake_client)) + + with pytest.raises(TypeError, match="Unsupported argument"): + list(Namespace.get(client=fake_client, config_file="/tmp/kubeconfig")) + + with pytest.raises(TypeError, match="Unsupported argument"): + list(Resource.get_all_cluster_resources(client=fake_client, context="default")) + + with pytest.raises(TypeError, match="Unsupported argument"): + list(Resource.get_all_cluster_resources(client=fake_client, config_dict={})) + + # Positional args after client are rejected (keyword-only API) + with pytest.raises(TypeError, match="positional"): + list(Namespace.get(fake_client, fake_client)) + + with pytest.raises(TypeError, match="positional"): + list(Resource.get_all_cluster_resources(fake_client, "extra")) + + # Resource.__init__ rejects removed kwargs as unexpected keyword arguments + with pytest.raises(TypeError, match="config_file"): + Pod( + client=fake_client, + name=BASE_POD_NAME, + namespace="default", + containers=POD_CONTAINERS, + config_file="/tmp/kubeconfig", + ) + + def test_event_client_is_required(self): + with pytest.raises(TypeError, match="client"): + list(Event.get()) + + with pytest.raises(TypeError, match="client"): + list(Event.get(client=None)) # type: ignore[arg-type] + + with pytest.raises(TypeError, match="client"): + Event.delete_events() + + with pytest.raises(TypeError, match="client"): + Event.delete_events(client=None) # type: ignore[arg-type] + def test_get_condition_message(self, pod): assert pod.get_condition_message( condition_type=pod.Condition.READY, condition_status=pod.Condition.Status.FALSE From 2429e3b246f6b6740f820ca1aba32b317aee481c Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 26 Aug 2026 18:54:12 +0300 Subject: [PATCH 02/16] fix(resource): address Qodo type-hint and docstring findings Co-authored-by: Cursor --- ocp_resources/resource.py | 55 ++++++++++++++++++++++++--------------- tests/test_resource.py | 7 ++--- 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index 0d7664631b..280ab8cb72 100644 --- a/ocp_resources/resource.py +++ b/ocp_resources/resource.py @@ -196,7 +196,14 @@ def _exchange_code_for_token( def _reject_removed_client_kwargs(kwargs: dict[str, Any]) -> None: - """Fail fast if deprecated client-config kwargs are passed via **kwargs.""" + """Fail fast if deprecated client-config kwargs are passed via **kwargs. + + Args: + kwargs: Keyword arguments that may include removed client-config keys. + + Raises: + TypeError: If any of dyn_client, config_file, config_dict, or context are present. + """ removed = _REMOVED_CLIENT_KWARGS.intersection(kwargs) if removed: names = ", ".join(sorted(removed)) @@ -647,31 +654,37 @@ def __init__( kind_dict: dict[Any, Any] | None = None, wait_for_resource: bool = False, schema_validation_enabled: bool = False, - ): + ) -> None: """ - Create an API resource + Create an API resource. If `yaml_file` or `kind_dict` are passed, logic in `to_dict` is bypassed. Args: - client (DynamicClient): Dynamic client for connecting to a remote cluster - name (str): Resource name - teardown (bool): Indicates if this resource would need to be deleted - yaml_file (str): yaml file for the resource - delete_timeout (int): timeout associated with delete action - dry_run (bool): dry run - node_selector (dict): node selector - node_selector_labels (str): node selector labels - label (dict): Resource labels - annotations (dict[str, str] | None): Resource annotations - api_group (str): Resource API group; will overwrite API group definition in resource class - hash_log_data (bool): Hash resource content based on resource keys_to_hash property - (example: Secret resource) - ensure_exists (bool): Whether to check if the resource exists before when initializing the resource, raise if not. - kind_dict (dict): dict which represents the resource object - wait_for_resource (bool): Waits for the resource to be created - schema_validation_enabled (bool): Enable automatic schema validation for this instance. + client: Dynamic client for connecting to a remote cluster. + name: Resource name. + teardown: Indicates if this resource would need to be deleted. + yaml_file: Yaml file for the resource. + delete_timeout: Timeout associated with delete action. + dry_run: Dry run. + node_selector: Node selector. + node_selector_labels: Node selector labels. + label: Resource labels. + annotations: Resource annotations. + api_group: Resource API group; will overwrite API group definition in resource class. + hash_log_data: Hash resource content based on resource keys_to_hash property + (example: Secret resource). + ensure_exists: Whether to check if the resource exists before when initializing the resource, raise if not. + kind_dict: Dict which represents the resource object. + wait_for_resource: Waits for the resource to be created. + schema_validation_enabled: Enable automatic schema validation for this instance. Defaults to False. Set to True to validate on create/update operations. + + Raises: + ValueError: If both yaml_file and kind_dict are provided. + TypeError: If client is None. + NotImplementedError: If neither api_group nor api_version is defined on the class. + MissingRequiredArgumentError: If name is missing when yaml_file and kind_dict are not set. """ if yaml_file and kind_dict: raise ValueError("yaml_file and resource_dict are mutually exclusive") @@ -1616,7 +1629,7 @@ def __init__( delete_timeout: int = TIMEOUT_4MINUTES, ensure_exists: bool = False, **kwargs: Any, - ): + ) -> None: super().__init__( name=name, client=client, diff --git a/tests/test_resource.py b/tests/test_resource.py index 6cdb7bfc5a..54073c8a53 100644 --- a/tests/test_resource.py +++ b/tests/test_resource.py @@ -5,6 +5,7 @@ import pytest import yaml +from fake_kubernetes_client.dynamic_client import FakeDynamicClient from ocp_resources.event import Event from ocp_resources.exceptions import ResourceTeardownError from ocp_resources.namespace import Namespace @@ -97,7 +98,7 @@ def test_get_all_cluster_resources(self, fake_client): if _resources: break - def test_client_is_required(self, fake_client): + def test_client_is_required(self, fake_client: FakeDynamicClient) -> None: with pytest.raises(TypeError, match="client"): Pod(name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) @@ -120,7 +121,7 @@ def test_client_is_required(self, fake_client): pod = Pod(client=fake_client, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) assert pod.client is fake_client - def test_removed_client_kwargs_rejected(self, fake_client): + def test_removed_client_kwargs_rejected(self, fake_client: FakeDynamicClient) -> None: with pytest.raises(TypeError, match="Unsupported argument"): list(Namespace.get(client=fake_client, dyn_client=fake_client)) @@ -150,7 +151,7 @@ def test_removed_client_kwargs_rejected(self, fake_client): config_file="/tmp/kubeconfig", ) - def test_event_client_is_required(self): + def test_event_client_is_required(self) -> None: with pytest.raises(TypeError, match="client"): list(Event.get()) From 635c5f37534d98f648b7f354acb99ce3c3dace0a Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 1 Sep 2026 15:28:59 +0300 Subject: [PATCH 03/16] test(resource): tighten client=None assertions and cover Event.list Tighten client=None guard assertions to the precise "client is required" message across Resource.__init__, Namespace.get, Resource.get_all_cluster_resources, NamespacedResource.get, Event.get, and Event.delete_events. Add coverage for NamespacedResource.get (via Pod.get) and Event.list (no-arg and client=None). Drop /tmp path from config_file test values to satisfy security-suspicious-paths CI, and clarify comments on generator list() semantics and kwarg rejection. --- tests/test_resource.py | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/tests/test_resource.py b/tests/test_resource.py index 54073c8a53..d039310a87 100644 --- a/tests/test_resource.py +++ b/tests/test_resource.py @@ -99,24 +99,34 @@ def test_get_all_cluster_resources(self, fake_client): break def test_client_is_required(self, fake_client: FakeDynamicClient) -> None: + # Note: .get() methods raise eagerly at call time, but get_all_cluster_resources + # is a true generator whose client=None guard only fires on iteration — list() + # is required to trigger it (and is kept on .get() calls for consistency). with pytest.raises(TypeError, match="client"): Pod(name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) - with pytest.raises(TypeError, match="client"): + with pytest.raises(TypeError, match="client is required"): Pod(client=None, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) # type: ignore[arg-type] with pytest.raises(TypeError, match="client"): list(Namespace.get()) - with pytest.raises(TypeError, match="client"): + with pytest.raises(TypeError, match="client is required"): list(Namespace.get(client=None)) # type: ignore[arg-type] with pytest.raises(TypeError, match="client"): list(Resource.get_all_cluster_resources()) - with pytest.raises(TypeError, match="client"): + with pytest.raises(TypeError, match="client is required"): list(Resource.get_all_cluster_resources(client=None)) # type: ignore[arg-type] + # NamespacedResource.get is a separate override with its own client guard + with pytest.raises(TypeError, match="client"): + list(Pod.get()) + + with pytest.raises(TypeError, match="client is required"): + list(Pod.get(client=None)) # type: ignore[arg-type] + # Valid construction still works with an explicit client pod = Pod(client=fake_client, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) assert pod.client is fake_client @@ -126,7 +136,8 @@ def test_removed_client_kwargs_rejected(self, fake_client: FakeDynamicClient) -> list(Namespace.get(client=fake_client, dyn_client=fake_client)) with pytest.raises(TypeError, match="Unsupported argument"): - list(Namespace.get(client=fake_client, config_file="/tmp/kubeconfig")) + # value is an arbitrary placeholder; the kwarg name triggers rejection + list(Namespace.get(client=fake_client, config_file="kubeconfig")) with pytest.raises(TypeError, match="Unsupported argument"): list(Resource.get_all_cluster_resources(client=fake_client, context="default")) @@ -134,34 +145,43 @@ def test_removed_client_kwargs_rejected(self, fake_client: FakeDynamicClient) -> with pytest.raises(TypeError, match="Unsupported argument"): list(Resource.get_all_cluster_resources(client=fake_client, config_dict={})) - # Positional args after client are rejected (keyword-only API) + # Namespace.get uses * in its signature; Python rejects extra positional args with pytest.raises(TypeError, match="positional"): list(Namespace.get(fake_client, fake_client)) + # get_all_cluster_resources accepts *args but manually raises TypeError when any are passed with pytest.raises(TypeError, match="positional"): list(Resource.get_all_cluster_resources(fake_client, "extra")) - # Resource.__init__ rejects removed kwargs as unexpected keyword arguments + # Resource.__init__ has no config_file parameter (removed from the API); Python raises TypeError for any unexpected keyword argument naturally with pytest.raises(TypeError, match="config_file"): Pod( client=fake_client, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS, - config_file="/tmp/kubeconfig", + # value is an arbitrary placeholder; the kwarg name triggers rejection + config_file="kubeconfig", ) def test_event_client_is_required(self) -> None: with pytest.raises(TypeError, match="client"): list(Event.get()) - with pytest.raises(TypeError, match="client"): + # list() is required here: Event.get is a generator, so the client=None guard only fires on first iteration + with pytest.raises(TypeError, match="client is required"): list(Event.get(client=None)) # type: ignore[arg-type] with pytest.raises(TypeError, match="client"): - Event.delete_events() + Event.list() + + with pytest.raises(TypeError, match="client is required"): + Event.list(client=None) # type: ignore[arg-type] with pytest.raises(TypeError, match="client"): + Event.delete_events() + + with pytest.raises(TypeError, match="client is required"): Event.delete_events(client=None) # type: ignore[arg-type] def test_get_condition_message(self, pod): From 418986344c9cd33c4f96915d43d5f5959c4620a2 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 8 Oct 2026 16:09:00 +0300 Subject: [PATCH 04/16] add client to NodeNetworkState --- ocp_resources/node_network_configuration_policy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ocp_resources/node_network_configuration_policy.py b/ocp_resources/node_network_configuration_policy.py index 5e1731b4e3..b1498ec235 100644 --- a/ocp_resources/node_network_configuration_policy.py +++ b/ocp_resources/node_network_configuration_policy.py @@ -236,7 +236,7 @@ def _get_port_from_nns(self, port_name): if not self.nodes: return None - nns = NodeNetworkState(name=self.nodes[0].name) + nns = NodeNetworkState(name=self.nodes[0].name, client=self.client) _port = [_iface for _iface in nns.interfaces if _iface["name"] == port_name] return _port[0] if _port else None From ae6a182c17f1213adbd8bc77b8300c749d28abf5 Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 4 Nov 2026 14:24:18 +0200 Subject: [PATCH 05/16] feat(resource)!: make client keyword-only and required - add MissingClientError(TypeError) carrying a migration hint; all client guards now raise it - make client a required keyword-only argument on Resource and NamespacedResource; name/namespace lead the positional parameters - reject removed client kwargs (dyn_client, config_file, config_dict, context) in constructors and name the instantiated class on unknown kwargs - validate the client eagerly in get_all_cluster_resources and Event.get; drop the dead *args escape hatch - remove the optional client parameter from 42 hand-written __init__s so client flows through **kwargs - document Raises for the client APIs; update AGENTS.md and the examples Signed-off-by: rnetser --- AGENTS.md | 3 +- examples/validation_demo.py | 7 +- examples/validation_troubleshooting.py | 12 +-- ocp_resources/endpoint_slice.py | 2 - ocp_resources/endpoints.py | 2 - ocp_resources/event.py | 39 +++++--- ocp_resources/exceptions.py | 10 ++ ocp_resources/host.py | 2 - ocp_resources/hostpath_provisioner.py | 2 - ocp_resources/image_stream.py | 2 - ocp_resources/ip_address_pool.py | 2 - ocp_resources/job.py | 2 - ocp_resources/l2_advertisement.py | 2 - ocp_resources/lease.py | 2 - ocp_resources/machine_health_check.py | 2 - ocp_resources/machine_set.py | 2 - ocp_resources/metallb.py | 2 - ocp_resources/migration.py | 4 - .../network_attachment_definition.py | 15 ++- ocp_resources/network_map.py | 2 - ocp_resources/node_maintenance.py | 2 - .../node_network_configuration_policy.py | 2 - ocp_resources/node_network_state.py | 2 - ocp_resources/ocs_initialization.py | 2 - ocp_resources/operator_group.py | 2 - ocp_resources/operator_source.py | 2 - ocp_resources/persistent_volume_claim.py | 2 - ocp_resources/pod_disruption_budget.py | 2 - ocp_resources/priority_class.py | 2 - ocp_resources/resource.py | 96 ++++++++++++++----- ocp_resources/restore.py | 2 - ocp_resources/role.py | 3 - ocp_resources/role_binding.py | 2 - ocp_resources/route.py | 2 - ocp_resources/secret.py | 2 - ocp_resources/sriov_network.py | 2 - ocp_resources/sriov_network_node_policy.py | 2 - ocp_resources/storage_map.py | 2 - ocp_resources/subscription.py | 2 - ocp_resources/upload_token_request.py | 2 - ocp_resources/virtual_machine.py | 2 - ocp_resources/virtual_machine_import.py | 4 - ocp_resources/virtual_machine_restore.py | 2 - ocp_resources/virtual_machine_snapshot.py | 2 - tests/test_resource.py | 64 +++++++------ tests/test_resources/test_migration.py | 11 +-- 46 files changed, 165 insertions(+), 171 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 88a709fa01..410616c6fd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -58,7 +58,8 @@ Code between markers is auto-generated — do NOT modify manually. Use `class-ge ## When Writing Code -- **Always pass `client` when instantiating a resource** — `client: DynamicClient` is required on `Resource` / `NamespacedResource` (and on `.get()` / `Event` APIs). Create it with `get_client()` (or `get_client(fake=True)` in tests); do not rely on implicit kubeconfig/client creation. +- **Always pass `client` when instantiating a resource** — `client: DynamicClient` is a required keyword argument on `Resource` / `NamespacedResource` and required on their `.get()` / `get_all_cluster_resources()` methods, plus the `Event` APIs. Create it with `get_client()` (or `get_client(fake=True)` in tests); do not rely on implicit kubeconfig/client creation. +- **`dyn_client`, `config_file`, `config_dict` and `context` are no longer accepted** on resource APIs — they raise `TypeError`. Pass them to `get_client()` instead. - No client-side validation in `to_dict()` — let the K8s/OCP API server return errors. Helper functions in resource classes CAN validate. - Context managers for auto-cleanup: `with Pod(client=client, name="test", namespace="default") as pod:` - Wait utilities: `wait_for_status()`, `wait_for_condition()` via timeout-sampler diff --git a/examples/validation_demo.py b/examples/validation_demo.py index b78cd87ea7..e0e0a29c95 100644 --- a/examples/validation_demo.py +++ b/examples/validation_demo.py @@ -64,13 +64,10 @@ def demo_auto_validation(): """Demonstrate automatic validation during create""" print_section(title="Auto-validation During Create") - # Get a fake client for demo - client = FAKE_CLIENT - # Create pod with auto-validation enabled print("1. Creating pod with auto-validation enabled...") pod = Pod( - client=client, + client=FAKE_CLIENT, name="auto-validated-pod", namespace="default", containers=[{"name": "nginx", "image": "nginx:latest"}], @@ -86,7 +83,7 @@ def demo_auto_validation(): # Try to create invalid pod with auto-validation print("\n2. Creating invalid pod with auto-validation...") invalid_pod = Pod( - client=client, + client=FAKE_CLIENT, name="invalid-auto-pod", namespace="default", containers=[{"name": "nginx"}], # Missing image diff --git a/examples/validation_troubleshooting.py b/examples/validation_troubleshooting.py index 37e80e97e1..e7b38c1c0d 100644 --- a/examples/validation_troubleshooting.py +++ b/examples/validation_troubleshooting.py @@ -38,7 +38,7 @@ def case_1_missing_required_fields(): print("Problem code:") print(""" pod = Pod( - client=client, + client=FAKE_CLIENT, name="my-pod", namespace="default", containers=[{"name": "nginx"}] # Missing 'image' @@ -56,7 +56,7 @@ def case_1_missing_required_fields(): print("Fixed code:") print(""" pod = Pod( - client=client, + client=FAKE_CLIENT, name="my-pod", namespace="default", containers=[{ @@ -124,7 +124,7 @@ def case_3_invalid_field_values(): print("Problem code:") print(""" pod = Pod( - client=client, + client=FAKE_CLIENT, name="My-Pod-123", # Capital letters not allowed namespace="default" ) @@ -146,7 +146,7 @@ def case_3_invalid_field_values(): print("Fixed code:") print(""" pod = Pod( - client=client, + client=FAKE_CLIENT, name="my-pod-123", # Valid DNS name namespace="default" ) @@ -163,7 +163,7 @@ def case_4_invalid_structure(): print("Problem code:") print(""" service = Service( - client=client, + client=FAKE_CLIENT, name="my-service", namespace="default", selector={"app": "nginx"}, @@ -191,7 +191,7 @@ def case_4_invalid_structure(): print("Fixed code:") print(""" service = Service( - client=client, + client=FAKE_CLIENT, name="my-service", namespace="default", selector={"app": "nginx"}, diff --git a/ocp_resources/endpoint_slice.py b/ocp_resources/endpoint_slice.py index df4349082c..c21c487d27 100644 --- a/ocp_resources/endpoint_slice.py +++ b/ocp_resources/endpoint_slice.py @@ -15,7 +15,6 @@ def __init__( self, name=None, namespace=None, - client=None, address_type=None, endpoints=None, ports=None, @@ -39,7 +38,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/endpoints.py b/ocp_resources/endpoints.py index 982a72727d..5c97e4c5ae 100644 --- a/ocp_resources/endpoints.py +++ b/ocp_resources/endpoints.py @@ -15,7 +15,6 @@ def __init__( self, name=None, namespace=None, - client=None, addresses=None, ports=None, teardown=True, @@ -37,7 +36,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/event.py b/ocp_resources/event.py index 35561c436c..453863a7db 100644 --- a/ocp_resources/event.py +++ b/ocp_resources/event.py @@ -5,6 +5,8 @@ from kubernetes.dynamic import DynamicClient from simple_logger.logger import get_logger +from ocp_resources.exceptions import MissingClientError + LOGGER = get_logger(name=__name__) @@ -42,6 +44,9 @@ def get( Returns list: event objects + Raises: + MissingClientError: If client is None. + example: reading all CSV Warning events in namespace "my-namespace", with reason of "AnEventReason" for event in Event.get( default_client, @@ -52,7 +57,7 @@ def get( print(event.object) """ if client is None: - raise TypeError("client is required") + raise MissingClientError() LOGGER.info("Reading events") LOGGER.debug( @@ -61,15 +66,18 @@ def get( f" resource_version={resource_version}, timeout={timeout}" ) - event_listener = client.resources.get(api_version=cls.api_version, kind=cls.__name__) - yield from event_listener.watch( - namespace=namespace, - name=name, - label_selector=label_selector, - field_selector=field_selector, - resource_version=resource_version, - timeout=timeout, - ) + def _get() -> Generator[Any, None, None]: + event_listener = client.resources.get(api_version=cls.api_version, kind=cls.__name__) + yield from event_listener.watch( + namespace=namespace, + name=name, + label_selector=label_selector, + field_selector=field_selector, + resource_version=resource_version, + timeout=timeout, + ) + + return _get() @staticmethod def _parse_timestamp(event: Any) -> datetime | None: @@ -108,6 +116,10 @@ def list( Returns: List of event resource objects, sorted by ``lastTimestamp`` descending (most recent first). + Raises: + MissingClientError: If client is None. + ValueError: If ``since_seconds`` is negative. + Example: List Warning events from the last 5 minutes in a namespace:: @@ -118,7 +130,7 @@ def list( ) """ if client is None: - raise TypeError("client is required") + raise MissingClientError() if since_seconds < 0: raise ValueError("since_seconds must be >= 0") @@ -177,12 +189,15 @@ def delete_events( resource_version (str): filter events by their resource's version timeout (int): timeout in seconds + Raises: + MissingClientError: If client is None. + example: deleting all the events with a reason of "AnEventReason", from "my-namespace" namespace def delete_events_before_test(client): Event.delete_events(client=client, namespace="my-namespace", field_selector="reason=AnEventReason") """ if client is None: - raise TypeError("client is required") + raise MissingClientError() LOGGER.info("Deleting events") LOGGER.debug( diff --git a/ocp_resources/exceptions.py b/ocp_resources/exceptions.py index 8eb5f365de..6caae2eff2 100644 --- a/ocp_resources/exceptions.py +++ b/ocp_resources/exceptions.py @@ -10,6 +10,16 @@ def __str__(self) -> str: return f"Missing required argument/s. Either provide yaml_file, kind_dict or pass {self.argument}" +class MissingClientError(TypeError): + """Raised when a Resource or Event API is used without a DynamicClient. + + Subclasses TypeError so callers that already catch TypeError keep working. + """ + + def __str__(self) -> str: + return "client is required. Pass a DynamicClient from get_client() or get_client(fake=True)." + + class MissingResourceError(Exception): def __init__(self, name: str) -> None: self.resource_name = name diff --git a/ocp_resources/host.py b/ocp_resources/host.py index 8cdb990a7a..4b23dbd0dc 100644 --- a/ocp_resources/host.py +++ b/ocp_resources/host.py @@ -19,7 +19,6 @@ def __init__( provider_namespace=None, secret_name=None, secret_namespace=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -28,7 +27,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/hostpath_provisioner.py b/ocp_resources/hostpath_provisioner.py index 35497477e6..3a93a50ec1 100644 --- a/ocp_resources/hostpath_provisioner.py +++ b/ocp_resources/hostpath_provisioner.py @@ -17,7 +17,6 @@ def __init__( name=None, path=None, image_pull_policy=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -25,7 +24,6 @@ def __init__( ): super().__init__( name=name, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/image_stream.py b/ocp_resources/image_stream.py index 78d4b351cd..013e7793dd 100644 --- a/ocp_resources/image_stream.py +++ b/ocp_resources/image_stream.py @@ -14,7 +14,6 @@ def __init__( self, name=None, namespace=None, - client=None, lookup_policy=False, tags=None, teardown=True, @@ -25,7 +24,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/ip_address_pool.py b/ocp_resources/ip_address_pool.py index 3d357b3c74..3adbb69afa 100644 --- a/ocp_resources/ip_address_pool.py +++ b/ocp_resources/ip_address_pool.py @@ -15,7 +15,6 @@ def __init__( self, name=None, namespace=None, - client=None, addresses=None, auto_assign=True, avoid_buggy_ips=False, @@ -33,7 +32,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, **kwargs, ) self.addresses = addresses diff --git a/ocp_resources/job.py b/ocp_resources/job.py index 9f0dbd5356..172e9eedc3 100644 --- a/ocp_resources/job.py +++ b/ocp_resources/job.py @@ -35,7 +35,6 @@ def __init__( self, name=None, namespace=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -50,7 +49,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/l2_advertisement.py b/ocp_resources/l2_advertisement.py index d83103f747..1dba16ea94 100644 --- a/ocp_resources/l2_advertisement.py +++ b/ocp_resources/l2_advertisement.py @@ -14,7 +14,6 @@ def __init__( self, name=None, namespace=None, - client=None, interfaces=None, ip_address_pools=None, ip_address_pools_selectors=None, @@ -33,7 +32,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, **kwargs, ) self.interfaces = interfaces diff --git a/ocp_resources/lease.py b/ocp_resources/lease.py index 6a7b7bf167..54c01ee6e8 100644 --- a/ocp_resources/lease.py +++ b/ocp_resources/lease.py @@ -14,7 +14,6 @@ def __init__( self, name=None, namespace=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -42,7 +41,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/machine_health_check.py b/ocp_resources/machine_health_check.py index 1999bc10c0..071cf1b097 100644 --- a/ocp_resources/machine_health_check.py +++ b/ocp_resources/machine_health_check.py @@ -15,7 +15,6 @@ def __init__( namespace=None, cluster_name=None, machineset_name=None, - client=None, machine_role="worker", machine_type="worker", node_startup_timeout="120m", @@ -30,7 +29,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/machine_set.py b/ocp_resources/machine_set.py index df2ff77723..ada3cb7ead 100644 --- a/ocp_resources/machine_set.py +++ b/ocp_resources/machine_set.py @@ -54,7 +54,6 @@ def __init__( name=None, namespace=None, teardown=True, - client=None, machine_role="worker", machine_type="worker", replicas=1, @@ -66,7 +65,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/metallb.py b/ocp_resources/metallb.py index 9c556ce5f1..eb3bef3a61 100644 --- a/ocp_resources/metallb.py +++ b/ocp_resources/metallb.py @@ -14,7 +14,6 @@ def __init__( self, name=None, namespace=None, - client=None, log_level="info", speaker_config=None, speaker_tolerations=None, @@ -33,7 +32,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, **kwargs, ) self.log_level = log_level diff --git a/ocp_resources/migration.py b/ocp_resources/migration.py index e21b9807a7..2bdbc8a3c7 100644 --- a/ocp_resources/migration.py +++ b/ocp_resources/migration.py @@ -1,8 +1,6 @@ from datetime import date from typing import Any -from kubernetes.dynamic import DynamicClient - from ocp_resources.resource import NamespacedResource from ocp_resources.utils.constants import TIMEOUT_4MINUTES @@ -29,7 +27,6 @@ def __init__( plan_name: str | None = None, plan_namespace: str | None = None, cut_over: date | None = None, - client: DynamicClient | None = None, teardown: bool = True, yaml_file: str | None = None, delete_timeout: int = TIMEOUT_4MINUTES, @@ -40,7 +37,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/network_attachment_definition.py b/ocp_resources/network_attachment_definition.py index 93fa29f937..5caba09a9b 100644 --- a/ocp_resources/network_attachment_definition.py +++ b/ocp_resources/network_attachment_definition.py @@ -18,7 +18,6 @@ def __init__( self, name=None, namespace=None, - client=None, cni_type=None, cni_version=DEFAULT_CNI_VERSION, config=None, @@ -31,7 +30,6 @@ def __init__( Args: name (str): Name of the NetworkAttachmentDefinition. namespace (str): Namespace of the NetworkAttachmentDefinition - client: (DynamicClient): DynamicClient to use. cni_type (str): NetworkAttachmentDefinition CNI. cni_version (str): NetworkAttachmentDefinition CNI version. config (dict): NetworkAttachmentDefinition spec["config"] to use, @@ -41,7 +39,6 @@ def __init__( *args, name=name, namespace=namespace, - client=client, **kwargs, ) self.cni_type = cni_type @@ -74,22 +71,22 @@ def __init__( cni_type, cni_version=DEFAULT_CNI_VERSION, vlan=None, - client=None, mtu=None, macspoofchk=None, teardown=True, old_nad_format=False, add_resource_name=True, dry_run=None, + **kwargs, ): super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, dry_run=dry_run, cni_version=cni_version, cni_type=cni_type, + **kwargs, ) self.old_nad_format = old_nad_format self.bridge_name = bridge_name @@ -125,13 +122,13 @@ def __init__( cni_type="cnv-bridge", cni_version=DEFAULT_CNI_VERSION, vlan=None, - client=None, mtu=None, tuning_type=None, teardown=True, macspoofchk=None, add_resource_name=True, dry_run=None, + **kwargs, ): super().__init__( name=name, @@ -139,13 +136,13 @@ def __init__( bridge_name=bridge_name, cni_type=cni_type, vlan=vlan, - client=client, mtu=mtu, teardown=teardown, macspoofchk=macspoofchk, add_resource_name=add_resource_name, dry_run=dry_run, cni_version=cni_version, + **kwargs, ) self.tuning_type = tuning_type @@ -170,11 +167,11 @@ def __init__( namespace, bridge_name, vlan=None, - client=None, mtu=None, teardown=True, dry_run=None, cni_version=DEFAULT_CNI_VERSION, + **kwargs, ): super().__init__( name=name, @@ -182,11 +179,11 @@ def __init__( bridge_name=bridge_name, cni_type="ovs", vlan=vlan, - client=client, mtu=mtu, teardown=teardown, dry_run=dry_run, cni_version=cni_version, + **kwargs, ) def to_dict(self) -> None: diff --git a/ocp_resources/network_map.py b/ocp_resources/network_map.py index 842ccb7245..2422503274 100644 --- a/ocp_resources/network_map.py +++ b/ocp_resources/network_map.py @@ -35,7 +35,6 @@ def __init__( source_provider_namespace=None, destination_provider_name=None, destination_provider_namespace=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -44,7 +43,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/node_maintenance.py b/ocp_resources/node_maintenance.py index eb4d1e327b..074d62f358 100644 --- a/ocp_resources/node_maintenance.py +++ b/ocp_resources/node_maintenance.py @@ -12,7 +12,6 @@ class NodeMaintenance(Resource): def __init__( self, name=None, - client=None, node=None, reason="TEST Reason", teardown=True, @@ -22,7 +21,6 @@ def __init__( ): super().__init__( name=name, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/node_network_configuration_policy.py b/ocp_resources/node_network_configuration_policy.py index b1498ec235..ececaaee68 100644 --- a/ocp_resources/node_network_configuration_policy.py +++ b/ocp_resources/node_network_configuration_policy.py @@ -34,7 +34,6 @@ class Reason: def __init__( self, name=None, - client=None, capture=None, node_selector=None, node_selector_labels=None, @@ -72,7 +71,6 @@ def __init__( """ super().__init__( name=name, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/node_network_state.py b/ocp_resources/node_network_state.py index 11fbaaabe1..430460a708 100644 --- a/ocp_resources/node_network_state.py +++ b/ocp_resources/node_network_state.py @@ -15,7 +15,6 @@ class NodeNetworkState(Resource): def __init__( self, name=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -23,7 +22,6 @@ def __init__( ): super().__init__( name=name, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/ocs_initialization.py b/ocp_resources/ocs_initialization.py index 9f282f4052..af820b5e8c 100644 --- a/ocp_resources/ocs_initialization.py +++ b/ocp_resources/ocs_initialization.py @@ -9,7 +9,6 @@ def __init__( self, name=None, namespace=None, - client=None, teardown=False, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -30,7 +29,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/operator_group.py b/ocp_resources/operator_group.py index 7bb7c8e577..a364146803 100644 --- a/ocp_resources/operator_group.py +++ b/ocp_resources/operator_group.py @@ -11,7 +11,6 @@ def __init__( namespace=None, target_namespaces=None, teardown=True, - client=None, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, **kwargs, @@ -23,7 +22,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/operator_source.py b/ocp_resources/operator_source.py index 16acef8b39..d3d3813b7a 100644 --- a/ocp_resources/operator_source.py +++ b/ocp_resources/operator_source.py @@ -13,7 +13,6 @@ def __init__( display_name=None, publisher=None, secret=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -22,7 +21,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/persistent_volume_claim.py b/ocp_resources/persistent_volume_claim.py index 37d0ba43b2..9b0b9a901a 100644 --- a/ocp_resources/persistent_volume_claim.py +++ b/ocp_resources/persistent_volume_claim.py @@ -33,7 +33,6 @@ def __init__( self, name=None, namespace=None, - client=None, storage_class=None, accessmodes=None, volume_mode=VolumeMode.FILE, @@ -48,7 +47,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/pod_disruption_budget.py b/ocp_resources/pod_disruption_budget.py index 26e64c1c23..2026d535ad 100644 --- a/ocp_resources/pod_disruption_budget.py +++ b/ocp_resources/pod_disruption_budget.py @@ -13,7 +13,6 @@ def __init__( self, name=None, namespace=None, - client=None, min_available=None, max_unavailable=None, selector=None, @@ -25,7 +24,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/priority_class.py b/ocp_resources/priority_class.py index 04d0f4d0a5..ac3a7e27cf 100644 --- a/ocp_resources/priority_class.py +++ b/ocp_resources/priority_class.py @@ -12,7 +12,6 @@ class PriorityClass(Resource): def __init__( self, name=None, - client=None, teardown=True, value=None, global_default=False, @@ -24,7 +23,6 @@ def __init__( ): super().__init__( name=name, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index 280ab8cb72..2a90738b75 100644 --- a/ocp_resources/resource.py +++ b/ocp_resources/resource.py @@ -42,6 +42,7 @@ from ocp_resources.exceptions import ( ClientWithBasicAuthError, ConditionError, + MissingClientError, MissingRequiredArgumentError, MissingResourceResError, ResourceTeardownError, @@ -638,8 +639,9 @@ class ApiVersion: def __init__( self, - client: DynamicClient, name: str | None = None, + *, + client: DynamicClient, teardown: bool = True, yaml_file: str | None = None, delete_timeout: int = TIMEOUT_4MINUTES, @@ -654,6 +656,7 @@ def __init__( kind_dict: dict[Any, Any] | None = None, wait_for_resource: bool = False, schema_validation_enabled: bool = False, + **kwargs: Any, ) -> None: """ Create an API resource. @@ -661,8 +664,9 @@ def __init__( If `yaml_file` or `kind_dict` are passed, logic in `to_dict` is bypassed. Args: - client: Dynamic client for connecting to a remote cluster. name: Resource name. + client: Mandatory dynamic client for connecting to a remote cluster. + Obtain it from `get_client()` (or `get_client(fake=True)` in tests). teardown: Indicates if this resource would need to be deleted. yaml_file: Yaml file for the resource. delete_timeout: Timeout associated with delete action. @@ -679,18 +683,25 @@ def __init__( wait_for_resource: Waits for the resource to be created. schema_validation_enabled: Enable automatic schema validation for this instance. Defaults to False. Set to True to validate on create/update operations. + **kwargs: Must be empty; removed client kwargs and unknown arguments are rejected. Raises: ValueError: If both yaml_file and kind_dict are provided. - TypeError: If client is None. + TypeError: If removed client kwargs (dyn_client, config_file, config_dict, context) + or unknown keyword arguments are passed. + MissingClientError: If client is None. NotImplementedError: If neither api_group nor api_version is defined on the class. MissingRequiredArgumentError: If name is missing when yaml_file and kind_dict are not set. """ + _reject_removed_client_kwargs(kwargs) + if kwargs: + raise TypeError(f"{type(self).__name__}() got unexpected keyword argument(s): {', '.join(sorted(kwargs))}") + if yaml_file and kind_dict: raise ValueError("yaml_file and resource_dict are mutually exclusive") if client is None: - raise TypeError("client is required") + raise MissingClientError() self.name = name self.teardown = teardown @@ -1209,16 +1220,21 @@ def get( Get resources Args: - client (DynamicClient): k8s client + client (DynamicClient): Mandatory k8s client; obtain it from get_client(). singular_name (str): Resource kind (in lowercase), in use where we have multiple matches for resource. raw (bool): If True return raw object. exceptions_dict (dict): Exceptions dict for TimeoutSampler Returns: generator: Generator of Resources of cls.kind. + + Raises: + MissingClientError: If client is None. + TypeError: If removed client kwargs (dyn_client, config_file, config_dict, context) + are passed. """ if client is None: - raise TypeError("client is required") + raise MissingClientError() _reject_removed_client_kwargs(kwargs) def _get() -> Generator["Resource|ResourceInstance", None, None]: @@ -1446,16 +1462,23 @@ def events( @staticmethod def get_all_cluster_resources( client: DynamicClient, - *args: Any, **kwargs: Any, ) -> Generator[ResourceField, None, None]: """ Get all cluster resources Args: - client (DynamicClient): k8s client + client (DynamicClient): Mandatory k8s client; obtain it from get_client(). **kwargs (dict): kwargs to pass to client.get() + Returns: + generator: Generator of cluster resources. + + Raises: + MissingClientError: If client is None. + TypeError: If removed client kwargs (dyn_client, config_file, config_dict, context) + are passed, or if a positional argument is passed. + Yields: kubernetes.dynamic.resource.ResourceField: Cluster resource. @@ -1464,20 +1487,19 @@ def get_all_cluster_resources( print(f"Resource: {resource}") """ if client is None: - raise TypeError("client is required") - if args: - raise TypeError( - "get_all_cluster_resources() takes no positional arguments after client; use keyword arguments only" - ) + raise MissingClientError() _reject_removed_client_kwargs(kwargs) - for _resource in client.resources.search(): - try: - _resources = client.get(_resource, **kwargs) - yield from _resources.items + def _get_all_cluster_resources() -> Generator[ResourceField, None, None]: + for _resource in client.resources.search(): + try: + _resources = client.get(_resource, **kwargs) + yield from _resources.items + + except (NotFoundError, TypeError, MethodNotAllowedError): + continue - except (NotFoundError, TypeError, MethodNotAllowedError): - continue + return _get_all_cluster_resources() def to_yaml(self) -> str: """ @@ -1621,15 +1643,40 @@ class NamespacedResource(Resource): def __init__( self, - client: DynamicClient, name: str | None = None, namespace: str | None = None, + *, + client: DynamicClient, teardown: bool = True, yaml_file: str | None = None, delete_timeout: int = TIMEOUT_4MINUTES, ensure_exists: bool = False, **kwargs: Any, ) -> None: + """ + Create a namespaced API resource. + + Args: + name: Resource name. + namespace: Resource namespace. + client: Mandatory dynamic client for connecting to a remote cluster. + Obtain it from `get_client()` (or `get_client(fake=True)` in tests). + teardown: Indicates if this resource would need to be deleted. + yaml_file: Yaml file for the resource. + delete_timeout: Timeout associated with delete action. + ensure_exists: Whether to check if the resource exists before when initializing the resource, raise if not. + **kwargs: Forwarded to `Resource.__init__`; removed client kwargs are rejected there + with the name of the instantiated class. + + Raises: + TypeError: If removed client kwargs (dyn_client, config_file, config_dict, context) + or unknown keyword arguments are passed. + MissingClientError: If client is None. + MissingRequiredArgumentError: If name or namespace is missing when yaml_file and + kind_dict are not set. + """ + _reject_removed_client_kwargs(kwargs) + super().__init__( name=name, client=client, @@ -1659,16 +1706,21 @@ def get( Get resources Args: - client (DynamicClient): k8s client + client (DynamicClient): Mandatory k8s client; obtain it from get_client(). singular_name (str): Resource kind (in lowercase), in use where we have multiple matches for resource. raw (bool): If True return raw object. exceptions_dict (dict): Exceptions dict for TimeoutSampler Returns: generator: Generator of Resources of cls.kind + + Raises: + MissingClientError: If client is None. + TypeError: If removed client kwargs (dyn_client, config_file, config_dict, context) + are passed. """ if client is None: - raise TypeError("client is required") + raise MissingClientError() _reject_removed_client_kwargs(kwargs) def _get() -> Generator["NamespacedResource|ResourceInstance", None, None]: diff --git a/ocp_resources/restore.py b/ocp_resources/restore.py index 2626c4b641..facfd9fa39 100644 --- a/ocp_resources/restore.py +++ b/ocp_resources/restore.py @@ -15,7 +15,6 @@ def __init__( namespace=None, included_namespaces=None, backup_name=None, - client=None, teardown=False, yaml_file=None, **kwargs, @@ -23,7 +22,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, **kwargs, diff --git a/ocp_resources/role.py b/ocp_resources/role.py index 8d12e73be5..6a4aa674f8 100644 --- a/ocp_resources/role.py +++ b/ocp_resources/role.py @@ -16,7 +16,6 @@ def __init__( self, name=None, namespace=None, - client=None, rules=None, teardown=True, yaml_file=None, @@ -27,7 +26,6 @@ def __init__( Args: name (str): ClusterRole name. namespace (str): Namespace name. - client (DynamicClient): DynamicClient to use. rules (list): list of dicts of rules. In the dict: permissions_to_resources (list): List of string with resource/s to which you want to add permissions to. Verbs (list): Determine the action/s (permissions) applicable on a specific resource. @@ -37,7 +35,6 @@ def __init__( delete_timeout (int, default: 4 minutes): timeout associated with delete action. """ super().__init__( - client=client, namespace=namespace, name=name, teardown=teardown, diff --git a/ocp_resources/role_binding.py b/ocp_resources/role_binding.py index 6860e2e5e7..2c681b1ae1 100644 --- a/ocp_resources/role_binding.py +++ b/ocp_resources/role_binding.py @@ -13,7 +13,6 @@ def __init__( self, name=None, namespace=None, - client=None, subjects_kind=None, subjects_name=None, subjects_namespace=None, @@ -28,7 +27,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/route.py b/ocp_resources/route.py index 05b1f4e470..fab8b63f92 100644 --- a/ocp_resources/route.py +++ b/ocp_resources/route.py @@ -13,7 +13,6 @@ def __init__( self, name=None, namespace=None, - client=None, service=None, destination_ca_cert=None, teardown=True, @@ -24,7 +23,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/secret.py b/ocp_resources/secret.py index 5ecd09be46..905aef3aca 100644 --- a/ocp_resources/secret.py +++ b/ocp_resources/secret.py @@ -13,7 +13,6 @@ def __init__( self, name=None, namespace=None, - client=None, accesskeyid=None, secretkey=None, htpasswd=None, @@ -28,7 +27,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/sriov_network.py b/ocp_resources/sriov_network.py index 91336648ec..0c56dc9e31 100644 --- a/ocp_resources/sriov_network.py +++ b/ocp_resources/sriov_network.py @@ -14,7 +14,6 @@ def __init__( name=None, namespace=None, network_namespace=None, - client=None, resource_name=None, vlan=None, ipam=None, @@ -27,7 +26,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/sriov_network_node_policy.py b/ocp_resources/sriov_network_node_policy.py index d5e6186582..516866fbf4 100644 --- a/ocp_resources/sriov_network_node_policy.py +++ b/ocp_resources/sriov_network_node_policy.py @@ -17,7 +17,6 @@ def __init__( root_devices=None, num_vfs=None, resource_name=None, - client=None, priority=None, mtu=None, node_selector=None, @@ -29,7 +28,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/storage_map.py b/ocp_resources/storage_map.py index 4963734cf4..79dc420ae7 100644 --- a/ocp_resources/storage_map.py +++ b/ocp_resources/storage_map.py @@ -38,7 +38,6 @@ def __init__( destination_provider_name=None, destination_provider_namespace=None, mapping=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -47,7 +46,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/subscription.py b/ocp_resources/subscription.py index 9cf3e305b9..a5d78ae6f8 100644 --- a/ocp_resources/subscription.py +++ b/ocp_resources/subscription.py @@ -9,7 +9,6 @@ def __init__( self, name=None, namespace=None, - client=None, source=None, source_namespace=None, package_name=None, @@ -24,7 +23,6 @@ def __init__( **kwargs, ): super().__init__( - client=client, name=name, namespace=namespace, teardown=teardown, diff --git a/ocp_resources/upload_token_request.py b/ocp_resources/upload_token_request.py index e6fb7d8d67..bd5009ffa5 100644 --- a/ocp_resources/upload_token_request.py +++ b/ocp_resources/upload_token_request.py @@ -13,7 +13,6 @@ def __init__( self, name=None, namespace=None, - client=None, pvc_name=None, teardown=True, yaml_file=None, @@ -23,7 +22,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/virtual_machine.py b/ocp_resources/virtual_machine.py index dce4bf96bf..98970c23f8 100644 --- a/ocp_resources/virtual_machine.py +++ b/ocp_resources/virtual_machine.py @@ -46,7 +46,6 @@ def __init__( self, name=None, namespace=None, - client=None, body=None, teardown=True, yaml_file=None, @@ -56,7 +55,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/virtual_machine_import.py b/ocp_resources/virtual_machine_import.py index ff87a2c110..ecf8f9cc07 100644 --- a/ocp_resources/virtual_machine_import.py +++ b/ocp_resources/virtual_machine_import.py @@ -88,7 +88,6 @@ def __init__( provider_credentials_secret_name=None, provider_type=None, provider_credentials_secret_namespace=None, - client=None, teardown=True, vm_id=None, vm_name=None, @@ -108,7 +107,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, @@ -242,7 +240,6 @@ def __init__( name=None, namespace=None, mapping=None, - client=None, teardown=True, yaml_file=None, **kwargs, @@ -250,7 +247,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, **kwargs, diff --git a/ocp_resources/virtual_machine_restore.py b/ocp_resources/virtual_machine_restore.py index d239d35d27..a769a45011 100644 --- a/ocp_resources/virtual_machine_restore.py +++ b/ocp_resources/virtual_machine_restore.py @@ -19,7 +19,6 @@ def __init__( namespace=None, vm_name=None, snapshot_name=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -29,7 +28,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/ocp_resources/virtual_machine_snapshot.py b/ocp_resources/virtual_machine_snapshot.py index fd0d16571f..e4a0add308 100644 --- a/ocp_resources/virtual_machine_snapshot.py +++ b/ocp_resources/virtual_machine_snapshot.py @@ -18,7 +18,6 @@ def __init__( name=None, namespace=None, vm_name=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -27,7 +26,6 @@ def __init__( super().__init__( name=name, namespace=namespace, - client=client, teardown=teardown, yaml_file=yaml_file, delete_timeout=delete_timeout, diff --git a/tests/test_resource.py b/tests/test_resource.py index d039310a87..56bf4ce7f6 100644 --- a/tests/test_resource.py +++ b/tests/test_resource.py @@ -7,7 +7,7 @@ from fake_kubernetes_client.dynamic_client import FakeDynamicClient from ocp_resources.event import Event -from ocp_resources.exceptions import ResourceTeardownError +from ocp_resources.exceptions import MissingClientError, ResourceTeardownError from ocp_resources.namespace import Namespace from ocp_resources.pod import Pod from ocp_resources.resource import NamespacedResourceList, Resource, ResourceList @@ -99,33 +99,32 @@ def test_get_all_cluster_resources(self, fake_client): break def test_client_is_required(self, fake_client: FakeDynamicClient) -> None: - # Note: .get() methods raise eagerly at call time, but get_all_cluster_resources - # is a true generator whose client=None guard only fires on iteration — list() - # is required to trigger it (and is kept on .get() calls for consistency). + # All client guards are eager: they raise at call time, before any iteration. with pytest.raises(TypeError, match="client"): Pod(name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) - with pytest.raises(TypeError, match="client is required"): + with pytest.raises(MissingClientError, match="client is required"): Pod(client=None, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) # type: ignore[arg-type] with pytest.raises(TypeError, match="client"): - list(Namespace.get()) + Namespace.get() - with pytest.raises(TypeError, match="client is required"): - list(Namespace.get(client=None)) # type: ignore[arg-type] + with pytest.raises(MissingClientError, match="client is required"): + Namespace.get(client=None) # type: ignore[arg-type] with pytest.raises(TypeError, match="client"): - list(Resource.get_all_cluster_resources()) + Resource.get_all_cluster_resources() - with pytest.raises(TypeError, match="client is required"): - list(Resource.get_all_cluster_resources(client=None)) # type: ignore[arg-type] + # get_all_cluster_resources validates eagerly even though it returns a generator + with pytest.raises(MissingClientError, match="client is required"): + Resource.get_all_cluster_resources(client=None) # type: ignore[arg-type] # NamespacedResource.get is a separate override with its own client guard with pytest.raises(TypeError, match="client"): - list(Pod.get()) + Pod.get() - with pytest.raises(TypeError, match="client is required"): - list(Pod.get(client=None)) # type: ignore[arg-type] + with pytest.raises(MissingClientError, match="client is required"): + Pod.get(client=None) # type: ignore[arg-type] # Valid construction still works with an explicit client pod = Pod(client=fake_client, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) @@ -133,55 +132,58 @@ def test_client_is_required(self, fake_client: FakeDynamicClient) -> None: def test_removed_client_kwargs_rejected(self, fake_client: FakeDynamicClient) -> None: with pytest.raises(TypeError, match="Unsupported argument"): - list(Namespace.get(client=fake_client, dyn_client=fake_client)) + Namespace.get(client=fake_client, dyn_client=fake_client) with pytest.raises(TypeError, match="Unsupported argument"): # value is an arbitrary placeholder; the kwarg name triggers rejection - list(Namespace.get(client=fake_client, config_file="kubeconfig")) + Namespace.get(client=fake_client, config_file="kubeconfig") with pytest.raises(TypeError, match="Unsupported argument"): - list(Resource.get_all_cluster_resources(client=fake_client, context="default")) + Resource.get_all_cluster_resources(client=fake_client, context="default") with pytest.raises(TypeError, match="Unsupported argument"): - list(Resource.get_all_cluster_resources(client=fake_client, config_dict={})) + Resource.get_all_cluster_resources(client=fake_client, config_dict={}) # Namespace.get uses * in its signature; Python rejects extra positional args with pytest.raises(TypeError, match="positional"): - list(Namespace.get(fake_client, fake_client)) + Namespace.get(fake_client, fake_client) - # get_all_cluster_resources accepts *args but manually raises TypeError when any are passed + # get_all_cluster_resources takes client as its only positional argument with pytest.raises(TypeError, match="positional"): - list(Resource.get_all_cluster_resources(fake_client, "extra")) + Resource.get_all_cluster_resources(fake_client, "extra") - # Resource.__init__ has no config_file parameter (removed from the API); Python raises TypeError for any unexpected keyword argument naturally - with pytest.raises(TypeError, match="config_file"): + # Constructors reject removed client kwargs with the migration hint + with pytest.raises(TypeError, match="Unsupported argument"): Pod( client=fake_client, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS, - # value is an arbitrary placeholder; the kwarg name triggers rejection - config_file="kubeconfig", + dyn_client=fake_client, ) + # Unknown kwargs are rejected by name, naming the instantiated class + with pytest.raises(TypeError, match="Pod.*unexpected keyword argument"): + Pod(client=fake_client, name=BASE_POD_NAME, namespace="default", bogus_kwarg=True) + def test_event_client_is_required(self) -> None: with pytest.raises(TypeError, match="client"): - list(Event.get()) + Event.get() - # list() is required here: Event.get is a generator, so the client=None guard only fires on first iteration - with pytest.raises(TypeError, match="client is required"): - list(Event.get(client=None)) # type: ignore[arg-type] + # Event.get returns a generator but validates the client eagerly + with pytest.raises(MissingClientError, match="client is required"): + Event.get(client=None) # type: ignore[arg-type] with pytest.raises(TypeError, match="client"): Event.list() - with pytest.raises(TypeError, match="client is required"): + with pytest.raises(MissingClientError, match="client is required"): Event.list(client=None) # type: ignore[arg-type] with pytest.raises(TypeError, match="client"): Event.delete_events() - with pytest.raises(TypeError, match="client is required"): + with pytest.raises(MissingClientError, match="client is required"): Event.delete_events(client=None) # type: ignore[arg-type] def test_get_condition_message(self, pod): diff --git a/tests/test_resources/test_migration.py b/tests/test_resources/test_migration.py index b13fdfa23b..b1a13590fd 100644 --- a/tests/test_resources/test_migration.py +++ b/tests/test_resources/test_migration.py @@ -2,7 +2,6 @@ from fake_kubernetes_client.dynamic_client import FakeDynamicClient from ocp_resources.migration import Migration -from ocp_resources.utils.constants import TIMEOUT_4MINUTES @pytest.mark.incremental @@ -65,7 +64,10 @@ def test_06_migration_with_resume_conversion(self, fake_client: FakeDynamicClien deployed_resource.clean_up(wait=False) def test_07_migration_with_positional_client(self, fake_client: FakeDynamicClient) -> None: - """Preserve the existing positional constructor arguments. + """Keep the documented positional constructor arguments. + + `name`, `namespace`, `plan_name`, `plan_namespace` and `cut_over` stay positional; + `client` is a required keyword-only argument. Args: fake_client (FakeDynamicClient): Fake Kubernetes client supplied by the fixture. @@ -76,10 +78,7 @@ def test_07_migration_with_positional_client(self, fake_client: FakeDynamicClien "test-plan", "default", None, - fake_client, - True, - None, - TIMEOUT_4MINUTES, + client=fake_client, ) assert migration.client is fake_client deployed_resource = migration.deploy() From 40149608787f2ef4d4dfe4c84f2949a74dc13fec Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 4 Nov 2026 15:42:29 +0200 Subject: [PATCH 06/16] fix(resource): address review findings on client API - drop stale `client` docstring entries in resource classes that no longer declare the parameter - remove the duplicate removed-kwargs check in NamespacedResource; the single check in Resource.__init__ rejects and names the class - drop the dead *args from NetworkAttachmentDefinition and annotate the NetworkAttachmentDefinition __init__s - correct Event.get/list docstrings (generator returns, mandatory client) and drop the redundant Returns from get_all_cluster_resources - lock the MissingClientError/TypeError contract with a test assertion Co-authored-by: PI (rits/zai-org/glm-5-3) Signed-off-by: rnetser --- AGENTS.md | 2 +- ocp_resources/endpoint_slice.py | 1 - ocp_resources/endpoints.py | 1 - ocp_resources/event.py | 10 +- ocp_resources/ip_address_pool.py | 1 - ocp_resources/job.py | 1 - ocp_resources/l2_advertisement.py | 1 - ocp_resources/lease.py | 1 - ocp_resources/metallb.py | 1 - .../network_attachment_definition.py | 93 +++++++++---------- ocp_resources/resource.py | 14 ++- tests/test_resource.py | 3 + 12 files changed, 61 insertions(+), 68 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 410616c6fd..e7d1706205 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,7 +59,7 @@ Code between markers is auto-generated — do NOT modify manually. Use `class-ge ## When Writing Code - **Always pass `client` when instantiating a resource** — `client: DynamicClient` is a required keyword argument on `Resource` / `NamespacedResource` and required on their `.get()` / `get_all_cluster_resources()` methods, plus the `Event` APIs. Create it with `get_client()` (or `get_client(fake=True)` in tests); do not rely on implicit kubeconfig/client creation. -- **`dyn_client`, `config_file`, `config_dict` and `context` are no longer accepted** on resource APIs — they raise `TypeError`. Pass them to `get_client()` instead. +- **`dyn_client`, `config_file`, `config_dict` and `context` are no longer accepted** on resource APIs — they raise `TypeError`. Use `client=` instead; `config_file`, `config_dict` and `context` still belong to `get_client()`. - No client-side validation in `to_dict()` — let the K8s/OCP API server return errors. Helper functions in resource classes CAN validate. - Context managers for auto-cleanup: `with Pod(client=client, name="test", namespace="default") as pod:` - Wait utilities: `wait_for_status()`, `wait_for_condition()` via timeout-sampler diff --git a/ocp_resources/endpoint_slice.py b/ocp_resources/endpoint_slice.py index c21c487d27..b7b432d4ec 100644 --- a/ocp_resources/endpoint_slice.py +++ b/ocp_resources/endpoint_slice.py @@ -27,7 +27,6 @@ def __init__( Args: name (str): Name of the EndpointSlice resource namespace (str): Namespace of EndpointSlice resource - client: (DynamicClient): DynamicClient for api calls address_type (string): Type of address carried by this endpoint endpoints (list): List of unique endpoints in this slice ports (list, optional): List of port numbers available on the related ip addresses diff --git a/ocp_resources/endpoints.py b/ocp_resources/endpoints.py index 5c97e4c5ae..a7bc51b362 100644 --- a/ocp_resources/endpoints.py +++ b/ocp_resources/endpoints.py @@ -26,7 +26,6 @@ def __init__( Args: name (str): Name of the endpoints resource namespace (str): Namespace of endpoints resource - client: (DynamicClient): DynamicClient for api calls addresses (list): List of ip addresses which offers the related ports that are marked as ready ports (list): List of port numbers available on the related ip addresses teardown (bool): Indicates if the resource should be torn down at the end diff --git a/ocp_resources/event.py b/ocp_resources/event.py index 453863a7db..ee17704b2e 100644 --- a/ocp_resources/event.py +++ b/ocp_resources/event.py @@ -32,7 +32,7 @@ def get( get - retrieves K8s events. Args: - client (DynamicClient): K8s client + client (DynamicClient): Mandatory K8s client; obtain it from get_client(). namespace (str): event namespace name (str): event name label_selector (str): filter events by labels; comma separated string of key=value @@ -41,8 +41,8 @@ def get( resource_version (str): filter events by their resource's version timeout (int): timeout in seconds - Returns - list: event objects + Returns: + generator: Yields watched event objects; the client is validated before iteration. Raises: MissingClientError: If client is None. @@ -107,7 +107,7 @@ def list( this method returns already-existing events immediately. Args: - client: K8s dynamic client. + client: Mandatory K8s dynamic client; obtain it from get_client(). namespace: Filter events to this namespace. field_selector: Filter events by fields (e.g. ``"type==Warning"``). label_selector: Filter events by labels. @@ -180,7 +180,7 @@ def delete_events( events in the test, in order to prevent false positive test. Args: - client (DynamicClient): K8s client + client (DynamicClient): Mandatory K8s client; obtain it from get_client(). namespace (str): event namespace name (str): event name label_selector (str): filter events by labels; comma separated string of key=value diff --git a/ocp_resources/ip_address_pool.py b/ocp_resources/ip_address_pool.py index 3adbb69afa..9c623028ea 100644 --- a/ocp_resources/ip_address_pool.py +++ b/ocp_resources/ip_address_pool.py @@ -24,7 +24,6 @@ def __init__( Args: name (str): Name of the MetalLB or it's CR's. namespace (str): Namespace of the MetalLB - client: (DynamicClient): DynamicClient to use. addresses (list): List of IP addresses. auto_assign (bool): IP address assignment. (default: True) avoid_buggy_ips (bool): Avoid buggy IP address (default: False) diff --git a/ocp_resources/job.py b/ocp_resources/job.py index 172e9eedc3..b3ead21d55 100644 --- a/ocp_resources/job.py +++ b/ocp_resources/job.py @@ -11,7 +11,6 @@ class Job(NamespacedResource): Args: name (str): Job name. namespace (str): Namespace name. - client (DynamicClient): Dynamic client for connecting to a remote cluster. teardown (bool): Indicates if this resource would need to be deleted. yaml_file (str): Yaml file for the resource. delete_timeout (int): Timeout associated with delete action. diff --git a/ocp_resources/l2_advertisement.py b/ocp_resources/l2_advertisement.py index 1dba16ea94..fb0b8a76e8 100644 --- a/ocp_resources/l2_advertisement.py +++ b/ocp_resources/l2_advertisement.py @@ -23,7 +23,6 @@ def __init__( Args: name (str): Name of the MetalLB or it's CR's. namespace (str): Namespace of the MetalLB - client: (DynamicClient): DynamicClient to use. interfaces (list): List of interfaces. ip_address_pools (list): The list of IPAddressPools to advertise via this advertisement, selected by name. ip_address_pools_selectors (list): A selector for the IPAddressPools which would get advertised diff --git a/ocp_resources/lease.py b/ocp_resources/lease.py index 54c01ee6e8..7fa036e1a3 100644 --- a/ocp_resources/lease.py +++ b/ocp_resources/lease.py @@ -28,7 +28,6 @@ def __init__( Args: name (str): Name of the Lease resource namespace (str): Namespace of Lease resource - client: (DynamicClient): DynamicClient for api calls teardown (bool): Indicates if the resource should be torn down at the end holder_identity (str, optional): identify of the holder of the current lease lease_duration_seconds (int, optional): duration for which candidate of a lease needs to wait to acquire it diff --git a/ocp_resources/metallb.py b/ocp_resources/metallb.py index eb3bef3a61..9a14b530b7 100644 --- a/ocp_resources/metallb.py +++ b/ocp_resources/metallb.py @@ -23,7 +23,6 @@ def __init__( Args: name (str): Name of the MetalLB or it's CR's. namespace (str): Namespace of the MetalLB - client: (DynamicClient): DynamicClient to use. log_level(str): The verbosity of the controller and the speaker logging. Allowed values are: all, debug, info, warn, error, none. (default: info) speaker_config (dict): Additional configs to be applied on MetalLB Speaker daemonset. diff --git a/ocp_resources/network_attachment_definition.py b/ocp_resources/network_attachment_definition.py index 5caba09a9b..cf24199a47 100644 --- a/ocp_resources/network_attachment_definition.py +++ b/ocp_resources/network_attachment_definition.py @@ -1,4 +1,5 @@ import json +from typing import Any from ocp_resources.exceptions import MissingRequiredArgumentError from ocp_resources.resource import NamespacedResource @@ -16,14 +17,13 @@ class NetworkAttachmentDefinition(NamespacedResource): def __init__( self, - name=None, - namespace=None, - cni_type=None, - cni_version=DEFAULT_CNI_VERSION, - config=None, - *args, - **kwargs, - ): + name: str | None = None, + namespace: str | None = None, + cni_type: str | None = None, + cni_version: str = DEFAULT_CNI_VERSION, + config: dict[str, Any] | None = None, + **kwargs: Any, + ) -> None: """ Create and manage NetworkAttachmentDefinition @@ -36,7 +36,6 @@ def __init__( if config is None basic config will be created. """ super().__init__( - *args, name=name, namespace=namespace, **kwargs, @@ -65,20 +64,20 @@ def to_dict(self) -> None: class BridgeNetworkAttachmentDefinition(NetworkAttachmentDefinition): def __init__( self, - name, - namespace, - bridge_name, - cni_type, - cni_version=DEFAULT_CNI_VERSION, - vlan=None, - mtu=None, - macspoofchk=None, - teardown=True, - old_nad_format=False, - add_resource_name=True, - dry_run=None, - **kwargs, - ): + name: str, + namespace: str, + bridge_name: str, + cni_type: str, + cni_version: str = DEFAULT_CNI_VERSION, + vlan: int | None = None, + mtu: int | None = None, + macspoofchk: bool | None = None, + teardown: bool = True, + old_nad_format: bool | None = False, + add_resource_name: bool = True, + dry_run: bool | None = None, + **kwargs: Any, + ) -> None: super().__init__( name=name, namespace=namespace, @@ -116,20 +115,20 @@ def to_dict(self) -> None: class LinuxBridgeNetworkAttachmentDefinition(BridgeNetworkAttachmentDefinition): def __init__( self, - name, - namespace, - bridge_name, - cni_type="cnv-bridge", - cni_version=DEFAULT_CNI_VERSION, - vlan=None, - mtu=None, - tuning_type=None, - teardown=True, - macspoofchk=None, - add_resource_name=True, - dry_run=None, - **kwargs, - ): + name: str, + namespace: str, + bridge_name: str, + cni_type: str = "cnv-bridge", + cni_version: str = DEFAULT_CNI_VERSION, + vlan: int | None = None, + mtu: int | None = None, + tuning_type: str | None = None, + teardown: bool = True, + macspoofchk: bool | None = None, + add_resource_name: bool = True, + dry_run: bool | None = None, + **kwargs: Any, + ) -> None: super().__init__( name=name, namespace=namespace, @@ -163,16 +162,16 @@ def resource_name(self): class OvsBridgeNetworkAttachmentDefinition(BridgeNetworkAttachmentDefinition): def __init__( self, - name, - namespace, - bridge_name, - vlan=None, - mtu=None, - teardown=True, - dry_run=None, - cni_version=DEFAULT_CNI_VERSION, - **kwargs, - ): + name: str, + namespace: str, + bridge_name: str, + vlan: int | None = None, + mtu: int | None = None, + teardown: bool = True, + dry_run: bool | None = None, + cni_version: str = DEFAULT_CNI_VERSION, + **kwargs: Any, + ) -> None: super().__init__( name=name, namespace=namespace, diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index 2a90738b75..8c757bfc4a 100644 --- a/ocp_resources/resource.py +++ b/ocp_resources/resource.py @@ -1224,6 +1224,7 @@ def get( singular_name (str): Resource kind (in lowercase), in use where we have multiple matches for resource. raw (bool): If True return raw object. exceptions_dict (dict): Exceptions dict for TimeoutSampler + **kwargs (Any): Filtering arguments passed to the resource list call. Returns: generator: Generator of Resources of cls.kind. @@ -1471,9 +1472,6 @@ def get_all_cluster_resources( client (DynamicClient): Mandatory k8s client; obtain it from get_client(). **kwargs (dict): kwargs to pass to client.get() - Returns: - generator: Generator of cluster resources. - Raises: MissingClientError: If client is None. TypeError: If removed client kwargs (dyn_client, config_file, config_dict, context) @@ -1665,8 +1663,9 @@ def __init__( yaml_file: Yaml file for the resource. delete_timeout: Timeout associated with delete action. ensure_exists: Whether to check if the resource exists before when initializing the resource, raise if not. - **kwargs: Forwarded to `Resource.__init__`; removed client kwargs are rejected there - with the name of the instantiated class. + **kwargs: Forwarded to `Resource.__init__`, which rejects removed client kwargs + (dyn_client, config_file, config_dict, context) and unknown arguments, naming the + instantiated class. Raises: TypeError: If removed client kwargs (dyn_client, config_file, config_dict, context) @@ -1675,8 +1674,6 @@ def __init__( MissingRequiredArgumentError: If name or namespace is missing when yaml_file and kind_dict are not set. """ - _reject_removed_client_kwargs(kwargs) - super().__init__( name=name, client=client, @@ -1710,6 +1707,7 @@ def get( singular_name (str): Resource kind (in lowercase), in use where we have multiple matches for resource. raw (bool): If True return raw object. exceptions_dict (dict): Exceptions dict for TimeoutSampler + **kwargs (Any): Filtering arguments passed to the resource list call. Returns: generator: Generator of Resources of cls.kind @@ -2083,7 +2081,7 @@ def __init__( Args: resource_class (Type[Resource]): The resource class to instantiate (e.g., Namespace). num_resources (int): The number of resource copies to create. - client (DynamicClient): The dynamic client to use. Defaults to None. + client (DynamicClient): The dynamic client to use; mandatory. **kwargs (Any): Arguments to be passed to the constructor of the resource_class. A 'name' key is required in kwargs to serve as the base name for the resources. """ diff --git a/tests/test_resource.py b/tests/test_resource.py index 56bf4ce7f6..d4153c0ea5 100644 --- a/tests/test_resource.py +++ b/tests/test_resource.py @@ -131,6 +131,9 @@ def test_client_is_required(self, fake_client: FakeDynamicClient) -> None: assert pod.client is fake_client def test_removed_client_kwargs_rejected(self, fake_client: FakeDynamicClient) -> None: + # Callers do `except TypeError`, so the exception must stay a TypeError subclass + assert issubclass(MissingClientError, TypeError) + with pytest.raises(TypeError, match="Unsupported argument"): Namespace.get(client=fake_client, dyn_client=fake_client) From 36c0aa7c177687466774a9338d559086eefedaa3 Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 4 Nov 2026 15:53:06 +0200 Subject: [PATCH 07/16] fix(resource): polish client API docs and error message - preserve a caller-supplied message in MissingClientError.__str__ - document get_all_cluster_resources with Returns instead of Yields, to match the other three client APIs that return an inner generator - reword the NamespacedResource kwargs docstring so only the unknown-kwarg rejection is described as naming the class - annotate OVNOverlayNetworkAttachmentDefinition and tighten the old_nad_format flag to bool - move the MissingClientError/TypeError contract assertion into its own test Co-authored-by: PI (rits/zai-org/glm-5-3) Signed-off-by: rnetser --- ocp_resources/exceptions.py | 4 +++- ocp_resources/network_attachment_definition.py | 14 +++++++------- ocp_resources/resource.py | 9 +++++---- tests/test_resource.py | 5 +++-- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/ocp_resources/exceptions.py b/ocp_resources/exceptions.py index 6caae2eff2..60c584f2c4 100644 --- a/ocp_resources/exceptions.py +++ b/ocp_resources/exceptions.py @@ -17,7 +17,9 @@ class MissingClientError(TypeError): """ def __str__(self) -> str: - return "client is required. Pass a DynamicClient from get_client() or get_client(fake=True)." + return ( + super().__str__() or "client is required. Pass a DynamicClient from get_client() or get_client(fake=True)." + ) class MissingResourceError(Exception): diff --git a/ocp_resources/network_attachment_definition.py b/ocp_resources/network_attachment_definition.py index cf24199a47..68444e9587 100644 --- a/ocp_resources/network_attachment_definition.py +++ b/ocp_resources/network_attachment_definition.py @@ -73,7 +73,7 @@ def __init__( mtu: int | None = None, macspoofchk: bool | None = None, teardown: bool = True, - old_nad_format: bool | None = False, + old_nad_format: bool = False, add_resource_name: bool = True, dry_run: bool | None = None, **kwargs: Any, @@ -197,12 +197,12 @@ def resource_name(self): class OVNOverlayNetworkAttachmentDefinition(NetworkAttachmentDefinition): def __init__( self, - network_name=None, - topology=None, - vlan=None, - mtu=None, - **kwargs, - ): + network_name: str | None = None, + topology: str | None = None, + vlan: int | None = None, + mtu: str | None = None, + **kwargs: Any, + ) -> None: """ Create and manage an OVN k8s overlay NetworkAttachmentDefinition (a switched, layer 2, topology network). diff --git a/ocp_resources/resource.py b/ocp_resources/resource.py index 8c757bfc4a..8a6fcbf285 100644 --- a/ocp_resources/resource.py +++ b/ocp_resources/resource.py @@ -1477,8 +1477,9 @@ def get_all_cluster_resources( TypeError: If removed client kwargs (dyn_client, config_file, config_dict, context) are passed, or if a positional argument is passed. - Yields: - kubernetes.dynamic.resource.ResourceField: Cluster resource. + Returns: + generator: Yields kubernetes.dynamic.resource.ResourceField cluster resources; the + client is validated before iteration starts. Example: for resource in get_all_cluster_resources(client=client, label_selector="my-label=value"): @@ -1664,8 +1665,8 @@ def __init__( delete_timeout: Timeout associated with delete action. ensure_exists: Whether to check if the resource exists before when initializing the resource, raise if not. **kwargs: Forwarded to `Resource.__init__`, which rejects removed client kwargs - (dyn_client, config_file, config_dict, context) and unknown arguments, naming the - instantiated class. + (dyn_client, config_file, config_dict, context) with a migration hint, and rejects + unknown arguments with an error naming the instantiated class. Raises: TypeError: If removed client kwargs (dyn_client, config_file, config_dict, context) diff --git a/tests/test_resource.py b/tests/test_resource.py index d4153c0ea5..0b0709a699 100644 --- a/tests/test_resource.py +++ b/tests/test_resource.py @@ -130,10 +130,11 @@ def test_client_is_required(self, fake_client: FakeDynamicClient) -> None: pod = Pod(client=fake_client, name=BASE_POD_NAME, namespace="default", containers=POD_CONTAINERS) assert pod.client is fake_client - def test_removed_client_kwargs_rejected(self, fake_client: FakeDynamicClient) -> None: - # Callers do `except TypeError`, so the exception must stay a TypeError subclass + def test_missing_client_error_is_type_error_subclass(self) -> None: + """Callers do `except TypeError`, so the exception must stay a TypeError subclass.""" assert issubclass(MissingClientError, TypeError) + def test_removed_client_kwargs_rejected(self, fake_client: FakeDynamicClient) -> None: with pytest.raises(TypeError, match="Unsupported argument"): Namespace.get(client=fake_client, dyn_client=fake_client) From 9f5f93a9c868df15602d05d776ed76a03120fe23 Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 4 Nov 2026 16:12:55 +0200 Subject: [PATCH 08/16] fix(network): type OVN overlay MTU as int OVNOverlayNetworkAttachmentDefinition declared mtu as str while every sibling NetworkAttachmentDefinition class and the ovn-k8s-cni-overlay NetConf expect an integer. Verified that mtu=1400 serializes as a JSON number in spec.config. Co-authored-by: PI (rits/zai-org/glm-5-3) Signed-off-by: rnetser --- ocp_resources/network_attachment_definition.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ocp_resources/network_attachment_definition.py b/ocp_resources/network_attachment_definition.py index 68444e9587..34bf42c83f 100644 --- a/ocp_resources/network_attachment_definition.py +++ b/ocp_resources/network_attachment_definition.py @@ -200,7 +200,7 @@ def __init__( network_name: str | None = None, topology: str | None = None, vlan: int | None = None, - mtu: str | None = None, + mtu: int | None = None, **kwargs: Any, ) -> None: """ @@ -214,7 +214,7 @@ def __init__( resources created in different namespaces to the same network. vlan (int, optional): A vlan tag ID that will be assigned to traffic from this additional network. - mtu (str, optional): The maximum transmission unit (MTU). + mtu (int, optional): The maximum transmission unit (MTU). topology (str): The secondary network topology to be created. """ super().__init__( From 3702f3f4c61018d342398742ae27bea9b1a16a3a Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 4 Nov 2026 16:24:50 +0200 Subject: [PATCH 09/16] fix(examples): pass an explicit client in role and NNCP examples Both examples constructed resources without `client`, which now raises MissingClientError, and contradicted the AGENTS.md rule this PR added. examples/special_cases.py is left alone: it already fails to import ocp_resources.network, which is a pre-existing defect. Co-authored-by: PI (rits/zai-org/glm-5-3) Signed-off-by: rnetser --- examples/node_network_configuration_policy.py | 4 ++++ examples/role.py | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/examples/node_network_configuration_policy.py b/examples/node_network_configuration_policy.py index f775148dc7..cb5db5d0b6 100644 --- a/examples/node_network_configuration_policy.py +++ b/examples/node_network_configuration_policy.py @@ -1,9 +1,13 @@ from ocp_resources.node_network_configuration_policy import ( NodeNetworkConfigurationPolicy, ) +from ocp_resources.resource import get_client + +client = get_client() # Using capture syntax to switch ipv4 config between interfaces my_nncp = NodeNetworkConfigurationPolicy( + client=client, name="capture_nncp", capture={ "first-nic": 'interfaces.name=="ens8"', diff --git a/examples/role.py b/examples/role.py index a554786e1e..87c3d56bf9 100644 --- a/examples/role.py +++ b/examples/role.py @@ -1,5 +1,8 @@ +from ocp_resources.resource import get_client from ocp_resources.role import Role +client = get_client() + # The example bellow is also relevant for the ClusterRole resource. # Add multiple rules to a specific Role or ClusterRole as a list of rules dictionaries: rules = [ @@ -20,6 +23,7 @@ }, ] latency_role = Role( + client=client, name="latency-role", namespace="namespace_name", rules=rules, From 5c5aa2d337ea0e11278da4d94b5c6ea3997ded64 Mon Sep 17 00:00:00 2001 From: rnetser Date: Wed, 4 Nov 2026 16:44:28 +0200 Subject: [PATCH 10/16] fix(examples): give the auto-validation demo its own fake client The demo creates pods, so sharing the module-level FAKE_CLIENT left 'auto-validated-pod' behind and a second run in the same process failed with a conflict instead of showing successful validation. Restores the per-run `get_client(fake=True)` the demo had before the examples were aligned on FAKE_CLIENT. Co-authored-by: PI (rits/zai-org/glm-5-3) Signed-off-by: rnetser --- examples/validation_demo.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/examples/validation_demo.py b/examples/validation_demo.py index e0e0a29c95..fceadad080 100644 --- a/examples/validation_demo.py +++ b/examples/validation_demo.py @@ -64,10 +64,15 @@ def demo_auto_validation(): """Demonstrate automatic validation during create""" print_section(title="Auto-validation During Create") + # This demo creates pods, so it needs its own fake client: reusing the shared + # FAKE_CLIENT would leave 'auto-validated-pod' behind and a second run of the + # demo would fail with a conflict instead of showing successful validation. + client = get_client(fake=True) + # Create pod with auto-validation enabled print("1. Creating pod with auto-validation enabled...") pod = Pod( - client=FAKE_CLIENT, + client=client, name="auto-validated-pod", namespace="default", containers=[{"name": "nginx", "image": "nginx:latest"}], @@ -83,7 +88,7 @@ def demo_auto_validation(): # Try to create invalid pod with auto-validation print("\n2. Creating invalid pod with auto-validation...") invalid_pod = Pod( - client=FAKE_CLIENT, + client=client, name="invalid-auto-pod", namespace="default", containers=[{"name": "nginx"}], # Missing image From c5b61ac7b9b3532656c923a77d61b8515e0cb6ba Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 5 Nov 2026 11:58:10 +0200 Subject: [PATCH 11/16] test(examples): pin the rule that examples pass an explicit client examples/ is excluded from every pre-commit hook, so a resource built without client= there only fails when a reader runs the example - which is how examples/role.py and examples/node_network_configuration_policy.py were broken by the mandatory-client change while all checks stayed green. The test statically walks examples/*.py, resolves every Resource/NamespacedResource subclass in the package, and fails when a constructor or get/list/get_all_cluster_resources/delete_events call omits client=. examples/special_cases.py is skipped because it imports the non-existent ocp_resources.network and is broken independently. Signed-off-by: rnetser --- tests/test_examples_client.py | 97 +++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 tests/test_examples_client.py diff --git a/tests/test_examples_client.py b/tests/test_examples_client.py new file mode 100644 index 0000000000..480c4433e4 --- /dev/null +++ b/tests/test_examples_client.py @@ -0,0 +1,97 @@ +"""Guard the AGENTS.md rule that resources are always constructed with an explicit client. + +`examples/` is excluded from every pre-commit hook (see `.pre-commit-config.yaml`), so a +resource constructed without ``client=`` there fails only when a reader runs the example. +That is exactly how `examples/role.py` and `examples/node_network_configuration_policy.py` +were broken by the mandatory-client change while all checks stayed green. +""" + +from __future__ import annotations + +import ast +import importlib +import inspect +import pkgutil +from pathlib import Path + +import ocp_resources +from ocp_resources.resource import Resource + +EXAMPLES_DIR = Path(__file__).parent.parent / "examples" + +# Classmethod APIs that take a client as well. +CLASSMETHOD_APIS = frozenset({"delete_events", "get", "get_all_cluster_resources", "list"}) + +# `examples/special_cases.py` imports `ocp_resources.network`, a module that does not +# exist, so it is already broken independently of the client contract. +SKIPPED_EXAMPLES = frozenset({"special_cases.py"}) + + +def _resource_class_names() -> set[str]: + """Return the names of every Resource/NamespacedResource subclass in the package.""" + names: set[str] = set() + for module_info in pkgutil.iter_modules(ocp_resources.__path__): + try: + module = importlib.import_module(f"ocp_resources.{module_info.name}") + except Exception: # pragma: no cover - a module that cannot import cannot be scanned + continue + + for name, obj in vars(module).items(): + if inspect.isclass(obj) and obj.__module__ == module.__name__ and issubclass(obj, Resource): + names.add(name) + + return names + + +def _call_target(node: ast.Call, resource_names: set[str]) -> str: + """Return the resource API name for the call, or an empty string when it is unrelated.""" + func = node.func + + if isinstance(func, ast.Name): + return func.id if func.id in resource_names else "" + + # e.g. Pod.get(...) / Resource.get_all_cluster_resources(...) + if isinstance(func, ast.Attribute) and isinstance(func.value, ast.Name): + if func.value.id in resource_names and func.attr in CLASSMETHOD_APIS: + return f"{func.value.id}.{func.attr}" + + return "" + + +def _missing_client_calls(tree: ast.AST, resource_names: set[str]) -> list[str]: + """Return `file:line` markers for resource calls that omit `client=`.""" + missing: list[str] = [] + + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + + target = _call_target(node=node, resource_names=resource_names) + if not target: + continue + + keywords = {keyword.arg for keyword in node.keywords} + if None in keywords: # **kwargs splat, cannot be checked statically + continue + + if "client" not in keywords: + missing.append(f"{target}(...)") + + return missing + + +def test_examples_construct_resources_with_client() -> None: + """Every resource construction in examples/ must pass an explicit client.""" + resource_names = _resource_class_names() + assert resource_names, "failed to discover any resource class" + + offenders: list[str] = [] + + for path in sorted(EXAMPLES_DIR.glob("*.py")): + if path.name in SKIPPED_EXAMPLES: + continue + + for marker in _missing_client_calls(tree=ast.parse(path.read_text()), resource_names=resource_names): + offenders.append(f"{path.name}: {marker}") + + assert not offenders, "resources constructed without client= in examples:\n" + "\n".join(offenders) From 7a21ee6b9f97df862f4b7a9cc1c21e9a4d4bd02b Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 5 Nov 2026 12:30:25 +0200 Subject: [PATCH 12/16] test(examples): harden the client guard and fix special_cases Review of the previous commit found the guard could pass vacuously: - module import failures were swallowed, silently shrinking the set of checked classes; they now fail the test, discovery walks subpackages and a floor assertion catches partial discovery - a missing examples/ directory produced a vacuous pass; the scan now asserts it found files - Event.get/list/delete_events were never checked (Event is not a Resource subclass); they are now included - a client passed positionally to a classmethod was reported as missing - aliased and module-qualified imports, e.g. `from ocp_resources.role import Role as R`, escaped detection; both are now resolved - markers now carry line numbers, and the AST helpers have parametrized unit tests instead of relying on the tree being clean examples/special_cases.py imported `ocp_resources.network`, which stopped existing in #1510, so it was the one file exempted from the check. It now imports from ocp_resources.network_config_openshift_io and passes a client, and the skip list is gone. Co-authored-by: PI (rits/deepseek-ai/DeepSeek-V4.1-Flash-1017) Signed-off-by: rnetser --- examples/special_cases.py | 9 +- tests/test_examples_client.py | 197 +++++++++++++++++++++++++++------- 2 files changed, 167 insertions(+), 39 deletions(-) diff --git a/examples/special_cases.py b/examples/special_cases.py index 6032241b73..c95ec2ba31 100644 --- a/examples/special_cases.py +++ b/examples/special_cases.py @@ -3,10 +3,13 @@ For example: `Network` which exists in both operator.openshift.io and config.openshift.io API groups """ -from ocp_resources.network import Network +from ocp_resources.network_config_openshift_io import Network +from ocp_resources.resource import get_client + +client = get_client() # To get the Network resource which uses the default API in the class ("config.openshift.io") -Network(name="cluster") +Network(client=client, name="cluster") # To get a Network resource with a different API group ("operator.openshift.io") -Network(name="cluster", api_group=Network.ApiGroup.OPERATOR_OPENSHIFT_IO) +Network(client=client, name="cluster", api_group=Network.ApiGroup.OPERATOR_OPENSHIFT_IO) diff --git a/tests/test_examples_client.py b/tests/test_examples_client.py index 480c4433e4..ffd7028984 100644 --- a/tests/test_examples_client.py +++ b/tests/test_examples_client.py @@ -1,9 +1,15 @@ -"""Guard the AGENTS.md rule that resources are always constructed with an explicit client. - -`examples/` is excluded from every pre-commit hook (see `.pre-commit-config.yaml`), so a -resource constructed without ``client=`` there fails only when a reader runs the example. -That is exactly how `examples/role.py` and `examples/node_network_configuration_policy.py` -were broken by the mandatory-client change while all checks stayed green. +"""Guard the AGENTS.md rule that every resource is built with an explicit client. + +`examples/` is excluded from the mypy hook (`.pre-commit-config.yaml`), and mypy cannot +catch this class of break anyway: the wrappers accept `client` through `**kwargs`, so +`Role(name=..., rules=...)` type-checks clean and fails only at runtime. That is how +`examples/role.py` and `examples/node_network_configuration_policy.py` were broken by the +mandatory-client change while every check stayed green. + +This test therefore checks the examples statically: every construction of a +Resource/NamespacedResource subclass - and every `get`/`list`/`get_all_cluster_resources`/ +`delete_events` call on one, plus the `Event` APIs - must receive a client, either as the +`client=` keyword or as the first positional argument of a classmethod. """ from __future__ import annotations @@ -14,59 +20,151 @@ import pkgutil from pathlib import Path +import pytest + import ocp_resources +from ocp_resources.event import Event from ocp_resources.resource import Resource EXAMPLES_DIR = Path(__file__).parent.parent / "examples" -# Classmethod APIs that take a client as well. +# Classmethod APIs that require a client as their first argument. CLASSMETHOD_APIS = frozenset({"delete_events", "get", "get_all_cluster_resources", "list"}) -# `examples/special_cases.py` imports `ocp_resources.network`, a module that does not -# exist, so it is already broken independently of the client contract. -SKIPPED_EXAMPLES = frozenset({"special_cases.py"}) +# Class discovery must find at least this many classes; a drop means discovery broke and +# the guard silently stopped covering the affected resources. +MIN_DISCOVERED_CLASSES = 200 def _resource_class_names() -> set[str]: - """Return the names of every Resource/NamespacedResource subclass in the package.""" - names: set[str] = set() - for module_info in pkgutil.iter_modules(ocp_resources.__path__): + """Collect every resource class name the examples may instantiate. + + Returns: + set[str]: Names of the Resource/NamespacedResource subclasses, plus the names of + the non-Resource API classes that take a client (``Event``). + + Raises: + AssertionError: If a module cannot be imported or discovery is implausibly small, + both of which would silently shrink the set of checked names. + """ + names: set[str] = {Event.__name__} + import_failures: list[str] = [] + + for module_info in pkgutil.walk_packages(ocp_resources.__path__, prefix="ocp_resources."): try: - module = importlib.import_module(f"ocp_resources.{module_info.name}") - except Exception: # pragma: no cover - a module that cannot import cannot be scanned + module = importlib.import_module(module_info.name) + except Exception as err: + import_failures.append(f"{module_info.name}: {err!r}") continue for name, obj in vars(module).items(): if inspect.isclass(obj) and obj.__module__ == module.__name__ and issubclass(obj, Resource): names.add(name) + assert not import_failures, f"cannot discover resource classes, failed to import: {import_failures}" + assert len(names) > MIN_DISCOVERED_CLASSES, ( + f"discovered only {len(names)} resource classes, expected more than {MIN_DISCOVERED_CLASSES}:" + " class discovery is broken" + ) + return names -def _call_target(node: ast.Call, resource_names: set[str]) -> str: - """Return the resource API name for the call, or an empty string when it is unrelated.""" +def _import_aliases(tree: ast.AST, resource_names: set[str]) -> dict[str, str]: + """Map local import aliases back to the resource class they refer to. + + Args: + tree: Parsed module. + resource_names: Known resource class names. + + Returns: + dict[str, str]: Local name to resource class name, e.g. ``{"R": "Role"}`` for + ``from ocp_resources.role import Role as R``. + """ + aliases: dict[str, str] = {} + + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("ocp_resources"): + for alias in node.names: + if alias.name in resource_names and alias.asname: + aliases[alias.asname] = alias.name + + return aliases + + +def _attribute_chain(node: ast.expr) -> list[str]: + """Flatten an attribute chain into its dotted parts. + + Args: + node: Expression to flatten. + + Returns: + list[str]: Parts of the chain, e.g. ``["ocp_resources", "role", "Role", "get"]``. + """ + parts: list[str] = [] + + while isinstance(node, ast.Attribute): + parts.append(node.attr) + node = node.value + + if isinstance(node, ast.Name): + parts.append(node.id) + + return list(reversed(parts)) + + +def _call_target(node: ast.Call, resource_names: set[str], aliases: dict[str, str]) -> tuple[str, bool]: + """Identify whether a call targets a client-requiring resource API. + + Args: + node: Call node to inspect. + resource_names: Known resource class names. + aliases: Local alias to resource class name mapping. + + Returns: + tuple[str, bool]: The API name (empty when unrelated) and whether it is a + classmethod, where the client may also be passed positionally. + """ func = node.func if isinstance(func, ast.Name): - return func.id if func.id in resource_names else "" + name = aliases.get(func.id, func.id) + return (name, False) if name in resource_names else ("", False) + + if isinstance(func, ast.Attribute): + chain = _attribute_chain(node=func) + + if len(chain) >= 2: + owner = aliases.get(chain[-2], chain[-2]) - # e.g. Pod.get(...) / Resource.get_all_cluster_resources(...) - if isinstance(func, ast.Attribute) and isinstance(func.value, ast.Name): - if func.value.id in resource_names and func.attr in CLASSMETHOD_APIS: - return f"{func.value.id}.{func.attr}" + if owner in resource_names and chain[-1] in CLASSMETHOD_APIS: + return f"{owner}.{chain[-1]}", True - return "" + # Qualified access such as `ocp_resources.role.Role(...)`. + if chain[-1] in resource_names: + return chain[-1], False + + return "", False def _missing_client_calls(tree: ast.AST, resource_names: set[str]) -> list[str]: - """Return `file:line` markers for resource calls that omit `client=`.""" + """Find resource calls that do not receive a client. + + Args: + tree: Parsed module. + resource_names: Known resource class names. + + Returns: + list[str]: One ``": (...)"`` marker per offending call. + """ + aliases = _import_aliases(tree=tree, resource_names=resource_names) missing: list[str] = [] for node in ast.walk(tree): if not isinstance(node, ast.Call): continue - target = _call_target(node=node, resource_names=resource_names) + target, is_classmethod = _call_target(node=node, resource_names=resource_names, aliases=aliases) if not target: continue @@ -74,24 +172,51 @@ def _missing_client_calls(tree: ast.AST, resource_names: set[str]) -> list[str]: if None in keywords: # **kwargs splat, cannot be checked statically continue - if "client" not in keywords: - missing.append(f"{target}(...)") + # Constructors take client as a required keyword; classmethods take it first. + if "client" in keywords or (is_classmethod and node.args): + continue + + missing.append(f"{node.lineno}: {target}(...)") return missing def test_examples_construct_resources_with_client() -> None: - """Every resource construction in examples/ must pass an explicit client.""" + """Every resource call in examples/ must receive an explicit client.""" resource_names = _resource_class_names() - assert resource_names, "failed to discover any resource class" + example_paths = sorted(EXAMPLES_DIR.rglob("*.py")) + assert example_paths, f"no example files found under {EXAMPLES_DIR}" offenders: list[str] = [] - for path in sorted(EXAMPLES_DIR.glob("*.py")): - if path.name in SKIPPED_EXAMPLES: - continue - + for path in example_paths: for marker in _missing_client_calls(tree=ast.parse(path.read_text()), resource_names=resource_names): - offenders.append(f"{path.name}: {marker}") - - assert not offenders, "resources constructed without client= in examples:\n" + "\n".join(offenders) + offenders.append(f"{path.relative_to(EXAMPLES_DIR.parent)}:{marker}") + + assert not offenders, "resource calls without a client in examples:\n" + "\n".join(offenders) + + +@pytest.mark.parametrize( + ("source", "expected"), + [ + pytest.param('Secret(name="s")', ["1: Secret(...)"], id="constructor-without-client"), + pytest.param('Secret(client=c, name="s")', [], id="constructor-with-client"), + pytest.param('Secret.get(label_selector="a=b")', ["1: Secret.get(...)"], id="classmethod-without-client"), + pytest.param('Secret.get(client=c, label_selector="a=b")', [], id="classmethod-with-client"), + pytest.param("Secret.get(c)", [], id="classmethod-positional-client"), + pytest.param("Resource.get_all_cluster_resources(c)", [], id="cluster-resources-positional-client"), + pytest.param("Event.get()", ["1: Event.get(...)"], id="event-api-without-client"), + pytest.param("Event.delete_events(client=c)", [], id="event-api-with-client"), + pytest.param("from ocp_resources.role import Role as R\nR(name='n')", ["2: Role(...)"], id="aliased-import"), + pytest.param( + "import ocp_resources.role\nocp_resources.role.Role(name='n')", ["2: Role(...)"], id="qualified-import" + ), + pytest.param("Secret(**kwargs)", [], id="kwargs-splat-is-skipped"), + pytest.param("print(Secret.__name__)", [], id="non-call-is-ignored"), + ], +) +def test_missing_client_detection(source: str, expected: list[str]) -> None: + """The detector must flag every client-less resource call and nothing else.""" + resource_names = {"Secret", "Resource", "Role", "Event"} + + assert _missing_client_calls(tree=ast.parse(source), resource_names=resource_names) == expected From 437893e43338c8f82028963505fcaa5a5ef2f738 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 5 Nov 2026 12:43:16 +0200 Subject: [PATCH 13/16] test(examples): tighten the client guard against false results Third review round found four ways the detector could give the wrong answer: - the module-qualified branch matched on the last attribute name only, so `self.Secret(...)` or `factory.Role(...)` was reported as a client-less resource; the chain root must now be ocp_resources or an alias of it - `Event()` was flagged even though only its classmethods need a client; Event moved to a classmethod-only set - walk_packages without onerror swallows a package import error, so its submodules were never yielded and never recorded; failures are recorded and the imported module count must equal the walked count - the discovery guards were assert statements, stripped under python -O; they now raise RuntimeError, and the floor moved from 200 to 240 with the measured count recorded Also reads example files as utf-8 and adds six parametrized cases covering aliased classmethods, module aliases, Event construction and attribute calls named like a resource. Co-authored-by: PI (rits/deepseek-ai/DeepSeek-V4.1-Flash-1017) Signed-off-by: rnetser --- tests/test_examples_client.py | 130 ++++++++++++++++++++++++---------- 1 file changed, 92 insertions(+), 38 deletions(-) diff --git a/tests/test_examples_client.py b/tests/test_examples_client.py index ffd7028984..789406d6fa 100644 --- a/tests/test_examples_client.py +++ b/tests/test_examples_client.py @@ -28,68 +28,98 @@ EXAMPLES_DIR = Path(__file__).parent.parent / "examples" -# Classmethod APIs that require a client as their first argument. +# Classmethod APIs that take a client as their first positional argument. CLASSMETHOD_APIS = frozenset({"delete_events", "get", "get_all_cluster_resources", "list"}) -# Class discovery must find at least this many classes; a drop means discovery broke and -# the guard silently stopped covering the affected resources. -MIN_DISCOVERED_CLASSES = 200 +# Client-requiring API classes that are not Resource subclasses, so they only need a +# client on the classmethods above - constructing them takes no client at all. +CLASSMETHOD_ONLY_CLASSES = frozenset({Event.__name__}) + +# Class discovery must find at least this many classes. Measured 248 Resource subclasses +# on 2026-11-04; a drop below the floor means discovery broke and the guard stopped +# covering the affected resources. +MIN_DISCOVERED_CLASSES = 240 def _resource_class_names() -> set[str]: - """Collect every resource class name the examples may instantiate. + """Collect every Resource/NamespacedResource subclass name. Returns: - set[str]: Names of the Resource/NamespacedResource subclasses, plus the names of - the non-Resource API classes that take a client (``Event``). + set[str]: Names of the resource classes the examples may instantiate. Raises: - AssertionError: If a module cannot be imported or discovery is implausibly small, - both of which would silently shrink the set of checked names. + RuntimeError: If a module or package cannot be imported, if fewer modules were + imported than walked, or if discovery is implausibly small. Each of those + would silently shrink the set of checked names. """ - names: set[str] = {Event.__name__} + names: set[str] = set() import_failures: list[str] = [] + walked = 0 + imported = 0 + + for module_info in pkgutil.walk_packages( + ocp_resources.__path__, + prefix="ocp_resources.", + # Without onerror, a package that fails to import is skipped silently and its + # submodules are never yielded, so the failure would never be recorded. + onerror=lambda name: import_failures.append(f"{name}: package import failed"), + ): + walked += 1 - for module_info in pkgutil.walk_packages(ocp_resources.__path__, prefix="ocp_resources."): try: module = importlib.import_module(module_info.name) except Exception as err: import_failures.append(f"{module_info.name}: {err!r}") continue + imported += 1 + for name, obj in vars(module).items(): if inspect.isclass(obj) and obj.__module__ == module.__name__ and issubclass(obj, Resource): names.add(name) - assert not import_failures, f"cannot discover resource classes, failed to import: {import_failures}" - assert len(names) > MIN_DISCOVERED_CLASSES, ( - f"discovered only {len(names)} resource classes, expected more than {MIN_DISCOVERED_CLASSES}:" - " class discovery is broken" - ) + if import_failures: + raise RuntimeError(f"cannot discover resource classes, failed to import: {import_failures}") + + if imported != walked: + raise RuntimeError(f"imported {imported} of {walked} walked modules: discovery is incomplete") + + if len(names) <= MIN_DISCOVERED_CLASSES: + raise RuntimeError( + f"discovered only {len(names)} resource classes, expected more than {MIN_DISCOVERED_CLASSES}:" + " class discovery is broken" + ) return names -def _import_aliases(tree: ast.AST, resource_names: set[str]) -> dict[str, str]: - """Map local import aliases back to the resource class they refer to. +def _import_aliases(tree: ast.AST, resource_names: set[str]) -> tuple[dict[str, str], set[str]]: + """Resolve the local names a module binds for resource classes and the package. Args: tree: Parsed module. resource_names: Known resource class names. Returns: - dict[str, str]: Local name to resource class name, e.g. ``{"R": "Role"}`` for - ``from ocp_resources.role import Role as R``. + tuple[dict[str, str], set[str]]: Local alias to resource class name, e.g. + ``{"R": "Role"}``, and the local names bound to ``ocp_resources`` or one of + its modules, e.g. ``{"ocp_resources", "r"}``. """ - aliases: dict[str, str] = {} + class_aliases: dict[str, str] = {} + module_names: set[str] = {"ocp_resources"} for node in ast.walk(tree): if isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("ocp_resources"): for alias in node.names: if alias.name in resource_names and alias.asname: - aliases[alias.asname] = alias.name + class_aliases[alias.asname] = alias.name + + elif isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "ocp_resources" or alias.name.startswith("ocp_resources."): + module_names.add(alias.asname or alias.name.split(".")[0]) - return aliases + return class_aliases, module_names def _attribute_chain(node: ast.expr) -> list[str]: @@ -113,13 +143,19 @@ def _attribute_chain(node: ast.expr) -> list[str]: return list(reversed(parts)) -def _call_target(node: ast.Call, resource_names: set[str], aliases: dict[str, str]) -> tuple[str, bool]: +def _call_target( + node: ast.Call, + resource_names: set[str], + class_aliases: dict[str, str], + module_names: set[str], +) -> tuple[str, bool]: """Identify whether a call targets a client-requiring resource API. Args: node: Call node to inspect. resource_names: Known resource class names. - aliases: Local alias to resource class name mapping. + class_aliases: Local alias to resource class name mapping. + module_names: Local names bound to the ocp_resources package or its modules. Returns: tuple[str, bool]: The API name (empty when unrelated) and whether it is a @@ -128,21 +164,24 @@ def _call_target(node: ast.Call, resource_names: set[str], aliases: dict[str, st func = node.func if isinstance(func, ast.Name): - name = aliases.get(func.id, func.id) + name = class_aliases.get(func.id, func.id) return (name, False) if name in resource_names else ("", False) if isinstance(func, ast.Attribute): chain = _attribute_chain(node=func) - if len(chain) >= 2: - owner = aliases.get(chain[-2], chain[-2]) + if len(chain) < 2: + return "", False + + owner = class_aliases.get(chain[-2], chain[-2]) - if owner in resource_names and chain[-1] in CLASSMETHOD_APIS: - return f"{owner}.{chain[-1]}", True + if chain[-1] in CLASSMETHOD_APIS and (owner in resource_names or owner in CLASSMETHOD_ONLY_CLASSES): + return f"{owner}.{chain[-1]}", True - # Qualified access such as `ocp_resources.role.Role(...)`. - if chain[-1] in resource_names: - return chain[-1], False + # Only module-qualified access counts as a constructor; `self.Pod(...)` or + # `factory.Secret(...)` just happen to share a name with a resource class. + if chain[-1] in resource_names and chain[0] in module_names: + return chain[-1], False return "", False @@ -157,14 +196,16 @@ def _missing_client_calls(tree: ast.AST, resource_names: set[str]) -> list[str]: Returns: list[str]: One ``": (...)"`` marker per offending call. """ - aliases = _import_aliases(tree=tree, resource_names=resource_names) + class_aliases, module_names = _import_aliases(tree=tree, resource_names=resource_names) missing: list[str] = [] for node in ast.walk(tree): if not isinstance(node, ast.Call): continue - target, is_classmethod = _call_target(node=node, resource_names=resource_names, aliases=aliases) + target, is_classmethod = _call_target( + node=node, resource_names=resource_names, class_aliases=class_aliases, module_names=module_names + ) if not target: continue @@ -172,7 +213,8 @@ def _missing_client_calls(tree: ast.AST, resource_names: set[str]) -> list[str]: if None in keywords: # **kwargs splat, cannot be checked statically continue - # Constructors take client as a required keyword; classmethods take it first. + # Constructors take client as a required keyword; classmethods take it first, + # because `get`/`list`/`delete_events` declare client as their first parameter. if "client" in keywords or (is_classmethod and node.args): continue @@ -190,7 +232,9 @@ def test_examples_construct_resources_with_client() -> None: offenders: list[str] = [] for path in example_paths: - for marker in _missing_client_calls(tree=ast.parse(path.read_text()), resource_names=resource_names): + tree = ast.parse(path.read_text(encoding="utf-8")) + + for marker in _missing_client_calls(tree=tree, resource_names=resource_names): offenders.append(f"{path.relative_to(EXAMPLES_DIR.parent)}:{marker}") assert not offenders, "resource calls without a client in examples:\n" + "\n".join(offenders) @@ -206,17 +250,27 @@ def test_examples_construct_resources_with_client() -> None: pytest.param("Secret.get(c)", [], id="classmethod-positional-client"), pytest.param("Resource.get_all_cluster_resources(c)", [], id="cluster-resources-positional-client"), pytest.param("Event.get()", ["1: Event.get(...)"], id="event-api-without-client"), + pytest.param("Event.list()", ["1: Event.list(...)"], id="event-list-without-client"), pytest.param("Event.delete_events(client=c)", [], id="event-api-with-client"), + pytest.param("Event()", [], id="event-construction-needs-no-client"), pytest.param("from ocp_resources.role import Role as R\nR(name='n')", ["2: Role(...)"], id="aliased-import"), + pytest.param( + 'from ocp_resources.pod import Pod as P\nP.get(label_selector="a")', + ["2: Pod.get(...)"], + id="aliased-classmethod", + ), pytest.param( "import ocp_resources.role\nocp_resources.role.Role(name='n')", ["2: Role(...)"], id="qualified-import" ), + pytest.param('import ocp_resources.role as r\nr.Role(name="n")', ["2: Role(...)"], id="module-alias-import"), + pytest.param('self.Secret(name="n")', [], id="attribute-named-like-a-resource-is-not-a-constructor"), + pytest.param('factory.Role(name="n")', [], id="foreign-object-attribute-is-not-a-constructor"), pytest.param("Secret(**kwargs)", [], id="kwargs-splat-is-skipped"), pytest.param("print(Secret.__name__)", [], id="non-call-is-ignored"), ], ) def test_missing_client_detection(source: str, expected: list[str]) -> None: """The detector must flag every client-less resource call and nothing else.""" - resource_names = {"Secret", "Resource", "Role", "Event"} + resource_names = {"Secret", "Resource", "Role", "Pod"} assert _missing_client_calls(tree=ast.parse(source), resource_names=resource_names) == expected From b6d569663eb3df330d1a49cbe8a89330d2691616 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 5 Nov 2026 16:16:20 +0200 Subject: [PATCH 14/16] fix(tests): cover all example client call forms - document the parametrized detector inputs and expected markers - detect modules imported from the ocp_resources package root - detect aliased Event APIs - accept non-literal positional client expressions while rejecting literals - skip starred positional arguments that static analysis cannot verify Co-authored-by: PI (rits/deepseek-ai/DeepSeek-V4.1-Flash-1017) Signed-off-by: rnetser --- tests/test_examples_client.py | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/tests/test_examples_client.py b/tests/test_examples_client.py index 789406d6fa..626d7a5d5a 100644 --- a/tests/test_examples_client.py +++ b/tests/test_examples_client.py @@ -108,11 +108,15 @@ def _import_aliases(tree: ast.AST, resource_names: set[str]) -> tuple[dict[str, class_aliases: dict[str, str] = {} module_names: set[str] = {"ocp_resources"} + known_api_names = resource_names | CLASSMETHOD_ONLY_CLASSES + for node in ast.walk(tree): if isinstance(node, ast.ImportFrom) and node.module and node.module.startswith("ocp_resources"): for alias in node.names: - if alias.name in resource_names and alias.asname: + if alias.name in known_api_names and alias.asname: class_aliases[alias.asname] = alias.name + elif node.module == "ocp_resources": + module_names.add(alias.asname or alias.name) elif isinstance(node, ast.Import): for alias in node.names: @@ -210,12 +214,15 @@ def _missing_client_calls(tree: ast.AST, resource_names: set[str]) -> list[str]: continue keywords = {keyword.arg for keyword in node.keywords} - if None in keywords: # **kwargs splat, cannot be checked statically + if None in keywords or any(isinstance(arg, ast.Starred) for arg in node.args): continue # Constructors take client as a required keyword; classmethods take it first, # because `get`/`list`/`delete_events` declare client as their first parameter. - if "client" in keywords or (is_classmethod and node.args): + # Classmethods declare client as their first parameter, so any non-literal + # positional expression may be a client; literals cannot be DynamicClients. + positional_client = is_classmethod and node.args and not isinstance(node.args[0], (ast.Constant, ast.Starred)) + if "client" in keywords or positional_client: continue missing.append(f"{node.lineno}: {target}(...)") @@ -248,6 +255,10 @@ def test_examples_construct_resources_with_client() -> None: pytest.param('Secret.get(label_selector="a=b")', ["1: Secret.get(...)"], id="classmethod-without-client"), pytest.param('Secret.get(client=c, label_selector="a=b")', [], id="classmethod-with-client"), pytest.param("Secret.get(c)", [], id="classmethod-positional-client"), + pytest.param("Secret.get(client_obj)", [], id="classmethod-client-object"), + pytest.param("Secret.get(get_client())", [], id="classmethod-client-factory"), + pytest.param('Secret.get("name")', ["1: Secret.get(...)"], id="classmethod-non-client-positional"), + pytest.param("Secret.get(*args)", [], id="classmethod-starred-args-is-skipped"), pytest.param("Resource.get_all_cluster_resources(c)", [], id="cluster-resources-positional-client"), pytest.param("Event.get()", ["1: Event.get(...)"], id="event-api-without-client"), pytest.param("Event.list()", ["1: Event.list(...)"], id="event-list-without-client"), @@ -263,6 +274,11 @@ def test_examples_construct_resources_with_client() -> None: "import ocp_resources.role\nocp_resources.role.Role(name='n')", ["2: Role(...)"], id="qualified-import" ), pytest.param('import ocp_resources.role as r\nr.Role(name="n")', ["2: Role(...)"], id="module-alias-import"), + pytest.param("from ocp_resources import role\nrole.Role(name='n')", ["2: Role(...)"], id="from-package-module"), + pytest.param( + "from ocp_resources import pod as p\np.Pod(name='n')", ["2: Pod(...)"], id="from-package-module-alias" + ), + pytest.param("from ocp_resources.event import Event as E\nE.get()", ["2: Event.get(...)"], id="event-alias"), pytest.param('self.Secret(name="n")', [], id="attribute-named-like-a-resource-is-not-a-constructor"), pytest.param('factory.Role(name="n")', [], id="foreign-object-attribute-is-not-a-constructor"), pytest.param("Secret(**kwargs)", [], id="kwargs-splat-is-skipped"), @@ -270,7 +286,12 @@ def test_examples_construct_resources_with_client() -> None: ], ) def test_missing_client_detection(source: str, expected: list[str]) -> None: - """The detector must flag every client-less resource call and nothing else.""" + """The detector must flag every client-less resource call and nothing else. + + Args: + source: Python source snippet to parse. + expected: Expected ``: (...)`` markers, empty when no client is missing. + """ resource_names = {"Secret", "Resource", "Role", "Pod"} assert _missing_client_calls(tree=ast.parse(source), resource_names=resource_names) == expected From 702a83bd2f4aca92d28aed66802d219bdd7c5b65 Mon Sep 17 00:00:00 2001 From: rnetser Date: Sun, 8 Nov 2026 11:20:02 +0200 Subject: [PATCH 15/16] docs(resource): document the mandatory client contract The breaking change left docs/*.md and 30 docs snippets describing the old API: client as optional (auto-created from kubeconfig with a FutureWarning), dyn_client/config_file/config_dict/context still listed as resource arguments, and MissingClientError undocumented. - resource-api.md: client is required/keyword-only, the removed kwargs are rejected, get()/get_all_cluster_resources() signatures match the code, and a "Migrating to the Mandatory client" section covers the before/after table plus the silent positional-misbinding risk for legacy subclasses where client sat mid-signature (Secret, BridgeNetworkAttachmentDefinition) - error-handling.md: MissingClientError section, import list and hierarchy - connecting-to-clusters.md: no implicit client, and the FutureWarning troubleshooting entry becomes the new TypeError entry - creating-and-managing-resources.md, querying-resources.md, working-with-events.md: stale client/dyn_client claims replaced - 30 snippets across 6 pages now pass client=client, matching the convention already used by common-patterns.md tests/test_examples_client.py now scans fenced python blocks in docs/*.md with the same AST detector (absolute line numbers, dedented list-nested fences, signature fragments skipped with a parsed-block floor), so docs drift fails the suite instead of shipping. Rename test_07_migration_with_positional_client to ..._positional_args_with_keyword_client: it asserts keyword client, and the docstring now says client travels through **kwargs, so nothing enforces it at the type level. 830 passed, 2 xfailed, coverage 76.26%. Co-authored-by: PI (rits/deepseek-ai/DeepSeek-V4.1-Flash-1017) Signed-off-by: rnetser --- docs/connecting-to-clusters.md | 16 ++- docs/creating-and-managing-resources.md | 2 +- docs/editing-resources-temporarily.md | 10 +- docs/error-handling.md | 49 ++++++++++ docs/generating-resource-classes.md | 1 + docs/querying-resources.md | 6 +- docs/quickstart.md | 1 + docs/resource-api.md | 72 ++++++++++---- docs/validating-resources.md | 2 + docs/working-with-events.md | 2 +- docs/working-with-virtual-machines.md | 21 +++- tests/test_examples_client.py | 125 ++++++++++++++++++++++-- tests/test_resources/test_migration.py | 5 +- 13 files changed, 268 insertions(+), 44 deletions(-) diff --git a/docs/connecting-to-clusters.md b/docs/connecting-to-clusters.md index 3a44f58c78..8d491e7e8b 100644 --- a/docs/connecting-to-clusters.md +++ b/docs/connecting-to-clusters.md @@ -121,7 +121,7 @@ with Namespace(client=client, name="temp-namespace") as ns: ns.wait_for_status(status=Namespace.Status.ACTIVE, timeout=120) ``` -> **Warning:** The `client` parameter will become mandatory in the next major release. Always pass it explicitly. If omitted, the library creates a client automatically using the default kubeconfig, but this triggers a `FutureWarning`. +> **Note:** `client` is a **required keyword-only** argument on every `Resource` / `NamespacedResource` constructor and a required first argument on `get()`, `get_all_cluster_resources()` and the `Event` APIs. Nothing is created implicitly: omitting it raises `TypeError` and `client=None` raises `MissingClientError`. The removed `dyn_client`, `config_file`, `config_dict` and `context` keywords now raise `TypeError` - they belong to `get_client()` only. See [Migrating to the Mandatory `client`](resource-api.html#migrating-to-the-mandatory-client). ## Environment Variables @@ -289,7 +289,19 @@ When multiple authentication parameters are provided, `get_client()` uses this p ## Troubleshooting -**"FutureWarning: 'client' arg will be mandatory"** +**"TypeError: Unsupported argument(s): config_file"** +You passed a client-config keyword to a resource instead of to `get_client()`. `config_file`, `config_dict`, `context` and `dyn_client` are no longer resource arguments: + +```python +# Wrong - the kubeconfig kwargs belong to get_client() and raise TypeError: +# Namespace(config_file="~/.kube/config", name="my-ns") + +# Right +client = get_client(config_file="~/.kube/config") +ns = Namespace(client=client, name="my-ns") +``` + +**"TypeError: ... missing 1 required keyword-only argument: 'client'"** You are creating a resource without passing a `client`. Always call `get_client()` first and pass the result: ```python diff --git a/docs/creating-and-managing-resources.md b/docs/creating-and-managing-resources.md index 46a801b109..1d9cc17c62 100644 --- a/docs/creating-and-managing-resources.md +++ b/docs/creating-and-managing-resources.md @@ -315,7 +315,7 @@ All resource classes accept these common parameters: | Parameter | Type | Default | Description | |-----------|------|---------|-------------| -| `client` | `DynamicClient` | `None` | Cluster client connection (will be required in next major release) | +| `client` | `DynamicClient` | **required** | Cluster client connection; keyword-only. Obtain it from `get_client()` | | `name` | `str` | `None` | Resource name | | `namespace` | `str` | `None` | Namespace (namespaced resources only) | | `teardown` | `bool` | `True` | Whether `clean_up()` deletes the resource | diff --git a/docs/editing-resources-temporarily.md b/docs/editing-resources-temporarily.md index 604e0488ca..8e0eb4dcc1 100644 --- a/docs/editing-resources-temporarily.md +++ b/docs/editing-resources-temporarily.md @@ -16,7 +16,7 @@ Temporarily modify Kubernetes and OpenShift resources during tests and have thei from ocp_resources.resource import ResourceEditor from ocp_resources.node import Node -node = Node(name="worker-0") +node = Node(client=client, name="worker-0") with ResourceEditor(patches={node: {"metadata": {"labels": {"test-label": "true"}}}}): # The node now has the label "test-label": "true" @@ -36,7 +36,7 @@ That's it—when the `with` block ends (even if an exception occurs), the origin from ocp_resources.resource import ResourceEditor from ocp_resources.config_map import ConfigMap -cm = ConfigMap(name="my-config", namespace="my-namespace") +cm = ConfigMap(client=client, name="my-config", namespace="my-namespace") ``` ### 2. Define your patch @@ -69,8 +69,8 @@ from ocp_resources.resource import ResourceEditor from ocp_resources.namespace import Namespace from ocp_resources.config_map import ConfigMap -ns = Namespace(name="test-ns") -cm = ConfigMap(name="app-config", namespace="test-ns") +ns = Namespace(client=client, name="test-ns") +cm = ConfigMap(client=client, name="app-config", namespace="test-ns") patches = { ns: {"metadata": {"labels": {"env": "test"}}}, @@ -91,7 +91,7 @@ If you need finer control over when patches are applied and reverted—for examp from ocp_resources.resource import ResourceEditor from ocp_resources.config_map import ConfigMap -cm = ConfigMap(name="app-config", namespace="default") +cm = ConfigMap(client=client, name="app-config", namespace="default") editor = ResourceEditor(patches={cm: {"data": {"mode": "maintenance"}}}) diff --git a/docs/error-handling.md b/docs/error-handling.md index 81bb9a7b8a..b2bd42cffb 100644 --- a/docs/error-handling.md +++ b/docs/error-handling.md @@ -5,6 +5,7 @@ All custom exceptions are defined in `ocp_resources.exceptions` and can be impor ```python from ocp_resources.exceptions import ( ExecOnPodError, + MissingClientError, MissingRequiredArgumentError, ResourceTeardownError, ValidationError, @@ -147,6 +148,53 @@ except MissingRequiredArgumentError as e: --- +## MissingClientError + +Raised when a `Resource` / `NamespacedResource` constructor, `get()`, `get_all_cluster_resources()`, or an `Event` API (`Event.get()`, `Event.list()`, `Event.delete_events()`) receives `client=None`. The library no longer creates a client implicitly, so `client` must come from `get_client()`. + +`MissingClientError` subclasses the built-in `TypeError`, so existing `except TypeError` handlers keep working. + +**Import:** + +```python +from ocp_resources.exceptions import MissingClientError +``` + +**Constructor Parameters:** None - the message is generated by `__str__()`, and a custom message is used as-is if one is passed. + +**String representation:** + +``` +client is required. Pass a DynamicClient from get_client() or get_client(fake=True). +``` + +**Raised by:** + +| Entry point | Condition | +|---|---| +| `Resource(...)` / `NamespacedResource(...)` | `client=None` (omitting `client` entirely raises the plain `TypeError` for the missing keyword-only argument) | +| `Resource.get()` / `NamespacedResource.get()` | `client=None` | +| `Resource.get_all_cluster_resources()` | `client=None` - validated eagerly, before the returned generator is iterated | +| `Event.get()` / `Event.list()` / `Event.delete_events()` | `client=None` | + +**Example:** + +```python +from ocp_resources.exceptions import MissingClientError +from ocp_resources.pod import Pod +from ocp_resources.resource import get_client + +client = get_client(fake=True) +try: + pod = Pod(client=client if client else None, name="my-pod", namespace="default") +except MissingClientError as e: + print(e) # client is required. Pass a DynamicClient from get_client() or get_client(fake=True). +``` + +> **Note:** Passing the removed `dyn_client`, `config_file`, `config_dict` or `context` keywords raises a plain `TypeError` with an `Unsupported argument(s): ...` message instead - see [Migrating to the Mandatory `client`](resource-api.html#migrating-to-the-mandatory-client). + +--- + ## ResourceTeardownError Raised when a resource's `clean_up()` method returns `False` during context manager exit. This indicates the resource could not be deleted. @@ -648,6 +696,7 @@ for resource in resources: | Exception | Parent | Module | |-----------|--------|--------| | `ExecOnPodError` | `Exception` | `ocp_resources.exceptions` | +| `MissingClientError` | `TypeError` | `ocp_resources.exceptions` | | `MissingRequiredArgumentError` | `Exception` | `ocp_resources.exceptions` | | `ResourceTeardownError` | `Exception` | `ocp_resources.exceptions` | | `ValidationError` | `Exception` | `ocp_resources.exceptions` | diff --git a/docs/generating-resource-classes.md b/docs/generating-resource-classes.md index 4856b550af..bd1068012a 100644 --- a/docs/generating-resource-classes.md +++ b/docs/generating-resource-classes.md @@ -22,6 +22,7 @@ This creates a Python file in `ocp_resources/` with a fully typed class you can from ocp_resources.deployment import Deployment dep = Deployment( + client=client, name="my-app", namespace="default", replicas=3, diff --git a/docs/querying-resources.md b/docs/querying-resources.md index c5f141b155..2d0ab033ca 100644 --- a/docs/querying-resources.md +++ b/docs/querying-resources.md @@ -109,18 +109,18 @@ for resource in Resource.get_all_cluster_resources(client=client, label_selector @classmethod def get( cls, - client: DynamicClient | None = None, + client: DynamicClient, + *, singular_name: str = "", exceptions_dict: dict[type[Exception], list[str]] = DEFAULT_CLUSTER_RETRY_EXCEPTIONS, raw: bool = False, - *args, **kwargs, ) -> Generator ``` | Parameter | Type | Description | |---|---|---| -| `client` | `DynamicClient` | Kubernetes dynamic client (required) | +| `client` | `DynamicClient` | Kubernetes dynamic client (required; positional or keyword) | | `singular_name` | `str` | Resource kind in lowercase; used when multiple resources match the same kind | | `raw` | `bool` | If `True`, return raw `ResourceInstance`/`ResourceField` objects instead of wrapper instances | | `exceptions_dict` | `dict` | Exceptions to retry on during API calls | diff --git a/docs/quickstart.md b/docs/quickstart.md index 0d5ff527a6..6137aa74c6 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -270,6 +270,7 @@ You can also validate manually without creating: ```python pod = Pod( + client=client, name="check-me", namespace="default", containers=[{"name": "nginx", "image": "nginx:latest"}], diff --git a/docs/resource-api.md b/docs/resource-api.md index 9ed0b1da9d..350d7a1c0c 100644 --- a/docs/resource-api.md +++ b/docs/resource-api.md @@ -69,6 +69,38 @@ See [Connecting to Clusters](connecting-to-clusters.html) for connection pattern --- +## Migrating to the Mandatory `client` + +`client` is now a **required keyword-only** argument on `Resource` / `NamespacedResource`, and a required first argument on the `.get()`, `.get_all_cluster_resources()` and `Event` APIs. The library no longer creates a client for you, so nothing happens implicitly from `KUBECONFIG` or `~/.kube/config`. + +| Before | After | +|---|---| +| `Pod(name="p", namespace="ns")` | `Pod(client=client, name="p", namespace="ns")` | +| `Pod(dyn_client=client, ...)` | `Pod(client=client, ...)` - `dyn_client` raises `TypeError` | +| `Pod(config_file="~/.kube/config", ...)` | `Pod(client=get_client(config_file="~/.kube/config"), ...)` | +| `Pod(config_dict={...}, ...)` | `Pod(client=get_client(config_dict={...}), ...)` | +| `Pod(context="prod", ...)` | `Pod(client=get_client(context="prod"), ...)` | +| `Pod.get(dyn_client=client, ...)` | `Pod.get(client=client, ...)` | +| `Event.get(dyn_client=client, ...)` | `Event.get(client=client, ...)` | + +`config_file`, `config_dict` and `context` still exist - they now belong to `get_client()` only. + +```python +from ocp_resources.pod import Pod +from ocp_resources.resource import get_client + +client = get_client() # once, at session/module setup +pod = Pod(client=client, name="my-pod", namespace="default") +``` + +### Failure modes to know about + +- Passing a removed keyword (`dyn_client`, `config_file`, `config_dict`, `context`) raises `TypeError: Unsupported argument(s): ...` with the migration hint. +- Omitting `client` raises `TypeError` (`... missing 1 required keyword-only argument: 'client'`); passing `client=None` raises `MissingClientError`, whose message names `get_client()`. +- Removing `client` from the middle of a legacy constructor signature means old **positional** calls no longer fail loudly - the argument silently binds to the next parameter. For example `Secret("n", "ns", client)` used to set `client` and now sets `accesskeyid`; `BridgeNetworkAttachmentDefinition(...)` used to take `client` as the 7th positional and now takes `mtu` there. Pass `client` by keyword and construct these resources with keywords only. + +--- + ## `Resource` ```python @@ -92,14 +124,15 @@ See [Understanding the Resource Class Hierarchy](resource-class-hierarchy.html) ```python Resource( - client=client, - name="my-resource", + name="my-resource", # positional + *, + client=client, # keyword-only, required ) ``` | Parameter | Type | Default | Description | |---|---|---|---| -| `client` | `DynamicClient \| None` | `None` | Kubernetes dynamic client. Will be mandatory in the next major release. | +| `client` | `DynamicClient` | **required** | Kubernetes dynamic client; keyword-only. Obtain it from `get_client()` (or `get_client(fake=True)` in tests). A missing or `None` client raises (`MissingClientError` subclasses `TypeError`). | | `name` | `str \| None` | `None` | Resource name. Required unless `yaml_file` or `kind_dict` is provided. | | `teardown` | `bool` | `True` | Whether this resource should be deleted when used as a context manager. | | `yaml_file` | `str \| None` | `None` | Path to a YAML file defining the resource. Mutually exclusive with `kind_dict`. | @@ -107,9 +140,6 @@ Resource( | `dry_run` | `bool` | `False` | If `True`, create operations use dry-run mode. | | `node_selector` | `dict[str, Any] \| None` | `None` | Node selector for scheduling. | | `node_selector_labels` | `dict[str, str] \| None` | `None` | Node selector labels for scheduling. | -| `config_file` | `str \| None` | `None` | Path to kubeconfig. Deprecated; pass `client` instead. | -| `config_dict` | `dict[str, Any] \| None` | `None` | Kubeconfig dict. | -| `context` | `str \| None` | `None` | Kubeconfig context name. Deprecated; pass `client` instead. | | `label` | `dict[str, str] \| None` | `None` | Labels to set on the resource. | | `annotations` | `dict[str, str] \| None` | `None` | Annotations to set on the resource. | | `api_group` | `str` | `""` | Override the class-level `api_group`. | @@ -119,11 +149,15 @@ Resource( | `wait_for_resource` | `bool` | `False` | When used as a context manager, wait for the resource to exist after creation. | | `schema_validation_enabled` | `bool` | `False` | Enable automatic OpenAPI schema validation on `create()` and `update_replace()`. | +`config_file`, `config_dict` and `context` are **not** constructor arguments any more - they belong to `get_client()`. Passing them (or `dyn_client`) through a resource constructor raises `TypeError` with a migration hint. See [Migrating to the Mandatory `client`](#migrating-to-the-mandatory-client). + **Raises:** | Exception | Condition | |---|---| | `ValueError` | Both `yaml_file` and `kind_dict` are provided. | +| `TypeError` | `dyn_client`, `config_file`, `config_dict`, `context`, or any unknown keyword argument is passed. | +| `MissingClientError` | `client` is `None` (a `TypeError` subclass, so existing `except TypeError` handlers keep working). | | `NotImplementedError` | Neither `api_group` nor `api_version` is defined on the class. | | `MissingRequiredArgumentError` | None of `name`, `yaml_file`, or `kind_dict` is provided. | | `ResourceNotFoundError` | `ensure_exists=True` and the resource does not exist. | @@ -424,14 +458,11 @@ Get the resource API object with optional filtering kwargs. @classmethod def get( cls, - client: DynamicClient | None = None, - dyn_client: DynamicClient | None = None, - config_file: str = "", + client: DynamicClient, + *, singular_name: str = "", exceptions_dict: dict[type[Exception], list[str]] = ..., raw: bool = False, - context: str | None = None, - *args: Any, **kwargs: Any, ) -> Generator[Any, None, None] ``` @@ -440,17 +471,18 @@ List resources of this kind from the cluster. | Parameter | Type | Default | Description | |---|---|---|---| -| `client` | `DynamicClient \| None` | `None` | Kubernetes client. | -| `dyn_client` | `DynamicClient \| None` | `None` | Deprecated alias for `client`. | -| `config_file` | `str` | `""` | Path to kubeconfig. Deprecated; pass `client`. | +| `client` | `DynamicClient` | **required** | Kubernetes client; positional or keyword. Obtain it from `get_client()`. | | `singular_name` | `str` | `""` | Singular resource name for disambiguation. | | `exceptions_dict` | `dict` | `DEFAULT_CLUSTER_RETRY_EXCEPTIONS` | Exceptions to retry. | | `raw` | `bool` | `False` | If `True`, yield raw `ResourceInstance` objects instead of wrapper instances. | -| `context` | `str \| None` | `None` | Kubeconfig context. Deprecated; pass `client`. | | `**kwargs` | | | Passed through to the API (e.g., `label_selector`, `field_selector`). | **Returns:** `Generator` of resource instances. +**Raises:** `MissingClientError` if `client` is `None`; `TypeError` if `dyn_client`, `config_file`, `config_dict` or `context` is passed, or if an extra positional argument is given (`singular_name` and the rest are keyword-only). + +`dyn_client`, `config_file` and `context` are no longer accepted - see [Migrating to the Mandatory `client`](#migrating-to-the-mandatory-client). + > **Note:** For `Resource` (cluster-scoped), yielded objects are constructed with `name` only. For `NamespacedResource`, yielded objects include both `name` and `namespace`. ```python @@ -477,11 +509,7 @@ See [Querying and Watching Resources](querying-resources.html) for advanced list ```python @staticmethod def get_all_cluster_resources( - client: DynamicClient | None = None, - config_file: str = "", - context: str | None = None, - config_dict: dict[str, Any] | None = None, - *args: Any, + client: DynamicClient, **kwargs: Any, ) -> Generator[ResourceField, None, None] ``` @@ -490,11 +518,13 @@ Yield all resources across **all** API groups in the cluster. | Parameter | Type | Default | Description | |---|---|---|---| -| `client` | `DynamicClient \| None` | `None` | Kubernetes client. | +| `client` | `DynamicClient` | **required** | Kubernetes client; positional or keyword. Obtain it from `get_client()`. | | `**kwargs` | | | Passed to the API (e.g., `label_selector`). | **Yields:** `ResourceField` +**Raises:** `MissingClientError` if `client` is `None`; `TypeError` if `dyn_client`, `config_file`, `config_dict` or `context` is passed, or if an extra positional argument is given. `client` is validated before iteration starts, so a bad call fails immediately instead of on the first `next()`. + ```python for resource in Resource.get_all_cluster_resources(client=client, label_selector="app=myapp"): print(f"{resource.kind}: {resource.metadata.name}") diff --git a/docs/validating-resources.md b/docs/validating-resources.md index e713ee3a25..74d5eb585b 100644 --- a/docs/validating-resources.md +++ b/docs/validating-resources.md @@ -13,6 +13,7 @@ Catch configuration mistakes before they reach the Kubernetes API by validating from ocp_resources.pod import Pod pod = Pod( + client=client, name="nginx-pod", namespace="default", containers=[{"name": "nginx", "image": "nginx:latest"}], @@ -33,6 +34,7 @@ from ocp_resources.exceptions import ValidationError from ocp_resources.service import Service service = Service( + client=client, name="nginx-service", namespace="default", selector={"app": "nginx"}, diff --git a/docs/working-with-events.md b/docs/working-with-events.md index 136ad12d3f..b49ba2201d 100644 --- a/docs/working-with-events.md +++ b/docs/working-with-events.md @@ -314,7 +314,7 @@ for event in pod.events( print(event.object.message) ``` -> **Warning:** The `dyn_client` parameter on `Event.get()` and `Event.delete_events()` is deprecated and will be removed in the next major release. Use `client` instead. +> **Note:** `client` is a required argument on all `Event` APIs (`Event.get()`, `Event.list()`, `Event.delete_events()`). The old `dyn_client` parameter has been removed and now raises `TypeError` - pass `client=get_client()` instead. A `client=None` call raises `MissingClientError`, and `Event.get()` validates the client before returning its generator, so a bad call fails immediately. ## Related Pages diff --git a/docs/working-with-virtual-machines.md b/docs/working-with-virtual-machines.md index eda46302ae..076bcf7b68 100644 --- a/docs/working-with-virtual-machines.md +++ b/docs/working-with-virtual-machines.md @@ -14,6 +14,7 @@ Manage the full lifecycle of KubeVirt virtual machines on OpenShift — create V from ocp_resources.virtual_machine import VirtualMachine vm = VirtualMachine( + client=client, name="my-vm", namespace="my-namespace", body={ @@ -41,6 +42,7 @@ with vm: from ocp_resources.virtual_machine import VirtualMachine vm = VirtualMachine( + client=client, name="my-vm", namespace="my-namespace", body={ @@ -67,6 +69,7 @@ You can also create a VM from a YAML file: ```python vm = VirtualMachine( + client=client, name="my-vm", namespace="my-namespace", yaml_file="vm-definition.yaml", @@ -182,6 +185,7 @@ vmi = vm.vmi # Returns a VirtualMachineInstance object from ocp_resources.virtual_machine_instance import VirtualMachineInstance vmi = VirtualMachineInstance( + client=client, name="my-vmi", namespace="my-namespace", domain={ @@ -336,6 +340,7 @@ Trigger a live migration by creating a `VirtualMachineInstanceMigration`: from ocp_resources.virtual_machine_instance_migration import VirtualMachineInstanceMigration migration = VirtualMachineInstanceMigration( + client=client, name="migrate-my-vm", namespace="my-namespace", vmi_name="my-vm", @@ -353,6 +358,7 @@ Control resource allocation for migrations: from ocp_resources.virtual_machine_migration_resource_quota import VirtualMachineMigrationResourceQuota quota = VirtualMachineMigrationResourceQuota( + client=client, name="migration-quota", namespace="my-namespace", requests_cpu="2", @@ -371,6 +377,7 @@ quota.deploy() from ocp_resources.virtual_machine_snapshot import VirtualMachineSnapshot snapshot = VirtualMachineSnapshot( + client=client, name="my-vm-snapshot", namespace="my-namespace", vm_name="my-vm", @@ -390,6 +397,7 @@ snapshot.wait_snapshot_done(timeout=300) from ocp_resources.virtual_machine_restore import VirtualMachineRestore restore = VirtualMachineRestore( + client=client, name="my-vm-restore", namespace="my-namespace", vm_name="my-vm", @@ -415,6 +423,7 @@ The `wait_restore_done()` method checks both the restore's `complete` status and from ocp_resources.virtual_machine_clone import VirtualMachineClone clone = VirtualMachineClone( + client=client, name="clone-my-vm", namespace="my-namespace", source_name="my-vm", @@ -444,6 +453,7 @@ clone.deploy() from ocp_resources.virtual_machine_export import VirtualMachineExport export = VirtualMachineExport( + client=client, name="export-my-vm", namespace="my-namespace", source={"apiGroup": "kubevirt.io", "kind": "VirtualMachine", "name": "my-vm"}, @@ -467,6 +477,7 @@ KubeVirt instance types define reusable VM sizing profiles. Preferences define n from ocp_resources.virtual_machine_instancetype import VirtualMachineInstancetype instancetype = VirtualMachineInstancetype( + client=client, name="small", namespace="my-namespace", cpu={"guest": 2}, @@ -483,6 +494,7 @@ Both `cpu` and `memory` are **required**. Omitting either raises `MissingRequire from ocp_resources.virtual_machine_cluster_instancetype import VirtualMachineClusterInstancetype cluster_instancetype = VirtualMachineClusterInstancetype( + client=client, name="large", cpu={"guest": 8}, memory={"guest": "16Gi"}, @@ -497,6 +509,7 @@ cluster_instancetype.deploy() from ocp_resources.virtual_machine_preference import VirtualMachinePreference preference = VirtualMachinePreference( + client=client, name="linux-pref", namespace="my-namespace", cpu={"preferredCPUTopology": "preferSockets"}, @@ -511,6 +524,7 @@ preference.deploy() from ocp_resources.virtual_machine_cluster_preference import VirtualMachineClusterPreference cluster_preference = VirtualMachineClusterPreference( + client=client, name="windows-pref", clock={"preferredTimer": {"hpet": {"present": False}}}, features={"preferredHyperv": {"spinlocks": {"spinlocks": 8191}}}, @@ -526,6 +540,7 @@ Scale out VMIs horizontally: from ocp_resources.virtual_machine_instance_replica_set import VirtualMachineInstanceReplicaSet replica_set = VirtualMachineInstanceReplicaSet( + client=client, name="my-vmi-rs", namespace="my-namespace", replicas=3, @@ -555,6 +570,7 @@ from ocp_resources.virtual_machine_storage_migration import VirtualMachineStorag # Create a migration plan plan = VirtualMachineStorageMigrationPlan( + client=client, name="migrate-storage", namespace="my-namespace", virtual_machines=[{"name": "my-vm", "volumes": [{"name": "rootdisk", "target": {"storageClassName": "fast-ssd"}}]}], @@ -564,6 +580,7 @@ plan.deploy() # Execute the migration migration = VirtualMachineStorageMigration( + client=client, name="migrate-storage-exec", namespace="my-namespace", virtual_machine_storage_migration_plan_ref={"name": "migrate-storage", "namespace": "my-namespace"}, @@ -577,6 +594,7 @@ migration.deploy() from ocp_resources.virtual_machine_template import VirtualMachineTemplate template = VirtualMachineTemplate( + client=client, name="fedora-template", namespace="my-namespace", virtual_machine={ @@ -601,11 +619,12 @@ template.deploy() ```python # List all VMs in a namespace -for vm in VirtualMachine.get(namespace="my-namespace"): +for vm in VirtualMachine.get(client=client, namespace="my-namespace"): print(f"{vm.name}: {vm.instance.get('status', {}).get('printableStatus')}") # List with label selectors for vm in VirtualMachine.get( + client=client, namespace="my-namespace", label_selector="app=webserver", ): diff --git a/tests/test_examples_client.py b/tests/test_examples_client.py index 626d7a5d5a..85488747ce 100644 --- a/tests/test_examples_client.py +++ b/tests/test_examples_client.py @@ -4,12 +4,14 @@ catch this class of break anyway: the wrappers accept `client` through `**kwargs`, so `Role(name=..., rules=...)` type-checks clean and fails only at runtime. That is how `examples/role.py` and `examples/node_network_configuration_policy.py` were broken by the -mandatory-client change while every check stayed green. - -This test therefore checks the examples statically: every construction of a -Resource/NamespacedResource subclass - and every `get`/`list`/`get_all_cluster_resources`/ -`delete_events` call on one, plus the `Event` APIs - must receive a client, either as the -`client=` keyword or as the first positional argument of a classmethod. +mandatory-client change while every check stayed green. User-facing docs drift the same +way - nothing executes them - so `docs/*.md` snippets are checked too. + +This test therefore checks the examples and the documentation statically: every +construction of a Resource/NamespacedResource subclass - and every `get`/`list`/ +`get_all_cluster_resources`/`delete_events` call on one, plus the `Event` APIs - must +receive a client, either as the `client=` keyword or as the first positional argument of +a classmethod. """ from __future__ import annotations @@ -18,6 +20,8 @@ import importlib import inspect import pkgutil +import re +import textwrap from pathlib import Path import pytest @@ -27,6 +31,16 @@ from ocp_resources.resource import Resource EXAMPLES_DIR = Path(__file__).parent.parent / "examples" +DOCS_DIR = Path(__file__).parent.parent / "docs" + +# Fenced blocks that contain Python. Everything else in the docs is bash, yaml, or json. +PYTHON_FENCE = re.compile(r"^\s*```(?:python|py)\s*$") +FENCE_END = re.compile(r"^\s*```\s*$") + +# Docs keep API-signature blocks (`def get(cls, client: DynamicClient, *, ...) -> Generator`) +# next to runnable snippets. Signatures have no body, so they never parse - they are +# skipped, and this floor fails the test if that skip ever swallows the real snippets. +MIN_PARSED_DOCS_BLOCKS = 380 # Classmethod APIs that take a client as their first positional argument. CLASSMETHOD_APIS = frozenset({"delete_events", "get", "get_all_cluster_resources", "list"}) @@ -190,12 +204,14 @@ def _call_target( return "", False -def _missing_client_calls(tree: ast.AST, resource_names: set[str]) -> list[str]: +def _missing_client_calls(tree: ast.AST, resource_names: set[str], line_offset: int = 0) -> list[str]: """Find resource calls that do not receive a client. Args: tree: Parsed module. resource_names: Known resource class names. + line_offset: Value added to each reported line number, so callers that parse a + fragment (a docs snippet, for example) can report the line in the host file. Returns: list[str]: One ``": (...)"`` marker per offending call. @@ -225,11 +241,44 @@ def _missing_client_calls(tree: ast.AST, resource_names: set[str]) -> list[str]: if "client" in keywords or positional_client: continue - missing.append(f"{node.lineno}: {target}(...)") + missing.append(f"{node.lineno + line_offset}: {target}(...)") return missing +def _markdown_python_blocks(path: Path) -> list[tuple[int, str]]: + """Extract the fenced Python blocks of a Markdown file. + + Args: + path: Markdown file to read. + + Returns: + list[tuple[int, str]]: One ``(first_code_line, code)`` pair per block, where + ``first_code_line`` is the 1-based line of ``code``'s first line in ``path`` + so reported line numbers point at the Markdown file. Fences are dedented so + snippets nested in list items still parse. + """ + blocks: list[tuple[int, str]] = [] + lines = path.read_text(encoding="utf-8").splitlines() + index = 0 + + while index < len(lines): + if not PYTHON_FENCE.match(lines[index]): + index += 1 + continue + + start = index + 1 + end = start + + while end < len(lines) and not FENCE_END.match(lines[end]): + end += 1 + + blocks.append((start + 1, textwrap.dedent("\n".join(lines[start:end])))) + index = end + 1 + + return blocks + + def test_examples_construct_resources_with_client() -> None: """Every resource call in examples/ must receive an explicit client.""" resource_names = _resource_class_names() @@ -247,6 +296,66 @@ def test_examples_construct_resources_with_client() -> None: assert not offenders, "resource calls without a client in examples:\n" + "\n".join(offenders) +def test_docs_snippets_construct_resources_with_client() -> None: + """Every resource call in a docs snippet must receive an explicit client.""" + resource_names = _resource_class_names() + doc_paths = sorted(DOCS_DIR.glob("*.md")) + assert doc_paths, f"no docs found under {DOCS_DIR}" + + offenders: list[str] = [] + skipped = 0 + parsed = 0 + + for path in doc_paths: + for first_line, code in _markdown_python_blocks(path=path): + try: + tree = ast.parse(code) + except SyntaxError: + # Signature-only illustrations have no body and never parse. + skipped += 1 + continue + + parsed += 1 + + for marker in _missing_client_calls(tree=tree, resource_names=resource_names, line_offset=first_line - 1): + offenders.append(f"{path.relative_to(DOCS_DIR.parent)}:{marker}") + + assert parsed > MIN_PARSED_DOCS_BLOCKS, ( + f"parsed only {parsed} docs snippets ({skipped} skipped as signatures), expected more than" + f" {MIN_PARSED_DOCS_BLOCKS}: snippet extraction is broken and the guard checked nothing" + ) + assert not offenders, "resource calls without a client in docs:\n" + "\n".join(offenders) + + +@pytest.mark.parametrize( + ("markdown", "expected"), + [ + pytest.param("```python\nPod(client=c)\n```\n", [(2, "Pod(client=c)")], id="python-fence"), + pytest.param("```py\nPod(client=c)\n```\n", [(2, "Pod(client=c)")], id="py-fence"), + pytest.param("```bash\nkubectl get pods\n```\n", [], id="non-python-fence-is-skipped"), + pytest.param("no fences here\n", [], id="no-fence"), + pytest.param( + "```python\na = 1\n```\n\ntext\n\n```python\nb = 2\n```\n", + [(2, "a = 1"), (8, "b = 2")], + id="two-blocks-report-absolute-lines", + ), + pytest.param("- item:\n ```python\n a = 1\n ```\n", [(3, "a = 1")], id="list-nested-fence-is-dedented"), + ], +) +def test_markdown_python_block_extraction(tmp_path: Path, markdown: str, expected: list[tuple[int, str]]) -> None: + """The Markdown extractor must find python fences and keep absolute line numbers. + + Args: + tmp_path: Pytest temporary directory. + markdown: Markdown source to parse. + expected: Expected ``(first_code_line, code)`` pairs. + """ + notebook = tmp_path / "page.md" + notebook.write_text(markdown, encoding="utf-8") + + assert _markdown_python_blocks(path=notebook) == expected + + @pytest.mark.parametrize( ("source", "expected"), [ diff --git a/tests/test_resources/test_migration.py b/tests/test_resources/test_migration.py index b1a13590fd..1cf96923b4 100644 --- a/tests/test_resources/test_migration.py +++ b/tests/test_resources/test_migration.py @@ -63,11 +63,12 @@ def test_06_migration_with_resume_conversion(self, fake_client: FakeDynamicClien assert resource_dict["spec"]["resumeConversion"] is True deployed_resource.clean_up(wait=False) - def test_07_migration_with_positional_client(self, fake_client: FakeDynamicClient) -> None: + def test_07_migration_positional_args_with_keyword_client(self, fake_client: FakeDynamicClient) -> None: """Keep the documented positional constructor arguments. `name`, `namespace`, `plan_name`, `plan_namespace` and `cut_over` stay positional; - `client` is a required keyword-only argument. + `client` is passed by keyword (it reaches `NamespacedResource` through `**kwargs`, + so nothing enforces that at the type level - only the run-time check does). Args: fake_client (FakeDynamicClient): Fake Kubernetes client supplied by the fixture. From e7095c0681df05c8dd541827797bfaaf0d8ae9a2 Mon Sep 17 00:00:00 2001 From: rnetser Date: Sun, 8 Nov 2026 11:33:31 +0200 Subject: [PATCH 16/16] fix(docs): make the two Qodo-flagged snippets runnable Qodo round on 702a83bd found two copy-paste defects in the new docs. - resource-api.md: the constructor example put a bare `*,` inside a call, which is a SyntaxError for anyone who copies it. It is now a valid `Resource(name="my-resource", client=client)` call, with the real keyword-only signature shown as prose instead of a fake call. - error-handling.md: the MissingClientError demo built the client with `client if client else None` from a live fake client, so the except branch was unreachable and the documented message never printed. It now passes `client=None`, plus a note that omitting `client` entirely raises Python's own TypeError instead. Both blocks now parse and are covered by the docs snippet guard. 830 passed, 2 xfailed, coverage 76.26%. Co-authored-by: PI (rits/deepseek-ai/DeepSeek-V4.1-Flash-1017) Signed-off-by: rnetser --- docs/error-handling.md | 6 +++--- docs/resource-api.md | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/error-handling.md b/docs/error-handling.md index b2bd42cffb..d4a6e09931 100644 --- a/docs/error-handling.md +++ b/docs/error-handling.md @@ -182,15 +182,15 @@ client is required. Pass a DynamicClient from get_client() or get_client(fake=Tr ```python from ocp_resources.exceptions import MissingClientError from ocp_resources.pod import Pod -from ocp_resources.resource import get_client -client = get_client(fake=True) try: - pod = Pod(client=client if client else None, name="my-pod", namespace="default") + pod = Pod(client=None, name="my-pod", namespace="default") except MissingClientError as e: print(e) # client is required. Pass a DynamicClient from get_client() or get_client(fake=True). ``` +> **Note:** Omitting `client` altogether never reaches `MissingClientError` - Python rejects the missing keyword-only argument first, with `TypeError: ... missing 1 required keyword-only argument: 'client'`. + > **Note:** Passing the removed `dyn_client`, `config_file`, `config_dict` or `context` keywords raises a plain `TypeError` with an `Unsupported argument(s): ...` message instead - see [Migrating to the Mandatory `client`](resource-api.html#migrating-to-the-mandatory-client). --- diff --git a/docs/resource-api.md b/docs/resource-api.md index 350d7a1c0c..ef2983cc5d 100644 --- a/docs/resource-api.md +++ b/docs/resource-api.md @@ -123,13 +123,13 @@ See [Understanding the Resource Class Hierarchy](resource-class-hierarchy.html) ### Constructor ```python -Resource( - name="my-resource", # positional - *, - client=client, # keyword-only, required -) +Resource(name="my-resource", client=client) ``` +The real signature places `client` behind a bare `*`, so it must be passed as a keyword: +`def __init__(self, name=None, *, client, teardown=True, ...)`. `name` stays positional-or-keyword; +anything after `*` (including `client`) is keyword-only. + | Parameter | Type | Default | Description | |---|---|---|---| | `client` | `DynamicClient` | **required** | Kubernetes dynamic client; keyword-only. Obtain it from `get_client()` (or `get_client(fake=True)` in tests). A missing or `None` client raises (`MissingClientError` subclasses `TypeError`). |