diff --git a/AGENTS.md b/AGENTS.md index f5c3330e87..8c0380a1c1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -58,14 +58,17 @@ 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`. 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(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/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..d4a6e09931 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 + +try: + 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). + +--- + ## 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..ef2983cc5d 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 @@ -91,15 +123,16 @@ See [Understanding the Resource Class Hierarchy](resource-class-hierarchy.html) ### Constructor ```python -Resource( - client=client, - name="my-resource", -) +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 \| 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/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, 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/examples/validation_demo.py b/examples/validation_demo.py index a03407f06a..fceadad080 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 @@ -60,7 +64,9 @@ def demo_auto_validation(): """Demonstrate automatic validation during create""" print_section(title="Auto-validation During Create") - # Get a fake client for demo + # 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 @@ -141,6 +147,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 +163,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 +184,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 +221,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..e7b38c1c0d 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=FAKE_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=FAKE_CLIENT, name="my-pod", namespace="default", containers=[{ @@ -119,6 +124,7 @@ def case_3_invalid_field_values(): print("Problem code:") print(""" pod = Pod( + client=FAKE_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=FAKE_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=FAKE_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=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..b7b432d4ec 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, @@ -28,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 @@ -39,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/endpoints.py b/ocp_resources/endpoints.py index 982a72727d..a7bc51b362 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, @@ -27,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 @@ -37,7 +35,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 c74f1afde9..ee17704b2e 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 @@ -6,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__) @@ -16,29 +17,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, @@ -50,8 +32,7 @@ def get( get - retrieves K8s events. Args: - client (DynamicClient): K8s client - dyn_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 @@ -60,8 +41,11 @@ 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. example: reading all CSV Warning events in namespace "my-namespace", with reason of "AnEventReason" for event in Event.get( @@ -72,7 +56,8 @@ def get( ): print(event.object) """ - _client = cls._resolve_client(client, dyn_client) + if client is None: + raise MissingClientError() LOGGER.info("Reading events") LOGGER.debug( @@ -81,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: @@ -119,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. @@ -128,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:: @@ -137,6 +129,9 @@ def list( field_selector="type==Warning", ) """ + if client is None: + raise MissingClientError() + if since_seconds < 0: raise ValueError("since_seconds must be >= 0") @@ -172,8 +167,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, @@ -186,8 +180,7 @@ def delete_events( events in the test, in order to prevent false positive test. Args: - client (DynamicClient): K8s client - dyn_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 @@ -196,11 +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") """ - _client = cls._resolve_client(client, dyn_client) + if client is None: + raise MissingClientError() LOGGER.info("Deleting events") LOGGER.debug( @@ -209,7 +206,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/exceptions.py b/ocp_resources/exceptions.py index 8eb5f365de..60c584f2c4 100644 --- a/ocp_resources/exceptions.py +++ b/ocp_resources/exceptions.py @@ -10,6 +10,18 @@ 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 ( + super().__str__() or "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..9c623028ea 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, @@ -25,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) @@ -33,7 +31,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..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. @@ -35,7 +34,6 @@ def __init__( self, name=None, namespace=None, - client=None, teardown=True, yaml_file=None, delete_timeout=TIMEOUT_4MINUTES, @@ -50,7 +48,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..fb0b8a76e8 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, @@ -24,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 @@ -33,7 +31,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..7fa036e1a3 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, @@ -29,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 @@ -42,7 +40,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..9a14b530b7 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, @@ -24,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. @@ -33,7 +31,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..34bf42c83f 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,32 +17,27 @@ class NetworkAttachmentDefinition(NamespacedResource): def __init__( self, - name=None, - namespace=None, - client=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 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, if config is None basic config will be created. """ super().__init__( - *args, name=name, namespace=namespace, - client=client, **kwargs, ) self.cni_type = cni_type @@ -68,28 +64,28 @@ def to_dict(self) -> None: class BridgeNetworkAttachmentDefinition(NetworkAttachmentDefinition): def __init__( self, - name, - namespace, - bridge_name, - 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, - ): + 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 = False, + add_resource_name: bool = True, + dry_run: bool | None = None, + **kwargs: Any, + ) -> None: 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 @@ -119,33 +115,33 @@ 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, - client=None, - mtu=None, - tuning_type=None, - teardown=True, - macspoofchk=None, - add_resource_name=True, - dry_run=None, - ): + 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, 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 @@ -166,27 +162,27 @@ def resource_name(self): class OvsBridgeNetworkAttachmentDefinition(BridgeNetworkAttachmentDefinition): def __init__( self, - name, - namespace, - bridge_name, - vlan=None, - client=None, - mtu=None, - teardown=True, - dry_run=None, - cni_version=DEFAULT_CNI_VERSION, - ): + 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, 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: @@ -201,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: int | None = None, + **kwargs: Any, + ) -> None: """ Create and manage an OVN k8s overlay NetworkAttachmentDefinition (a switched, layer 2, topology network). @@ -218,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__( 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 5e1731b4e3..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, @@ -236,7 +234,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 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 86e0d3a79d..8a6fcbf285 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 @@ -43,6 +42,7 @@ from ocp_resources.exceptions import ( ClientWithBasicAuthError, ConditionError, + MissingClientError, MissingRequiredArgumentError, MissingResourceResError, ResourceTeardownError, @@ -193,6 +193,26 @@ 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. + + 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)) + 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,17 +639,15 @@ class ApiVersion: def __init__( self, - client: DynamicClient | None = None, # TODO: make mandatory in the next major release name: str | None = None, + *, + client: DynamicClient, teardown: bool = True, yaml_file: str | None = None, delete_timeout: int = TIMEOUT_4MINUTES, 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 = "", @@ -638,37 +656,53 @@ 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 + Create an API resource. 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 - 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 - 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. + 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. + 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. + **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 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 MissingClientError() + self.name = name self.teardown = teardown self.yaml_file = yaml_file @@ -677,19 +711,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,56 +1209,49 @@ 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]: """ Get resources 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. + 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 + **kwargs (Any): Filtering arguments passed to the resource list call. 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) + 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 MissingClientError() + _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,47 +1462,43 @@ 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, - *args: Any, + client: DynamicClient, **kwargs: Any, ) -> Generator[ResourceField, None, None]: """ 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() + client (DynamicClient): Mandatory k8s client; obtain it from get_client(). **kwargs (dict): kwargs to pass to client.get() - Yields: - kubernetes.dynamic.resource.ResourceField: Cluster resource. + 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. + + Returns: + generator: Yields kubernetes.dynamic.resource.ResourceField cluster resources; the + client is validated before iteration starts. 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, - ) - client = get_client(config_file=config_file, config_dict=config_dict, context=context) + if client is None: + raise MissingClientError() + _reject_removed_client_kwargs(kwargs) - for _resource in client.resources.search(): - try: - _resources = client.get(_resource, *args, **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: """ @@ -1631,13 +1644,37 @@ def __init__( self, name: str | None = None, namespace: str | None = None, + *, + client: DynamicClient, teardown: bool = True, yaml_file: str | None = None, delete_timeout: int = TIMEOUT_4MINUTES, - client: DynamicClient | None = None, 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__`, which rejects removed client kwargs + (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) + 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. + """ super().__init__( name=name, client=client, @@ -1656,51 +1693,44 @@ 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]: """ Get resources 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. + 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 + **kwargs (Any): Filtering arguments passed to the resource list call. 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) + 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 MissingClientError() + _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 +1739,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, ) @@ -2052,7 +2082,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/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_examples_client.py b/tests/test_examples_client.py new file mode 100644 index 0000000000..85488747ce --- /dev/null +++ b/tests/test_examples_client.py @@ -0,0 +1,406 @@ +"""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. 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 + +import ast +import importlib +import inspect +import pkgutil +import re +import textwrap +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" +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"}) + +# 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/NamespacedResource subclass name. + + Returns: + set[str]: Names of the resource classes the examples may instantiate. + + Raises: + 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] = 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 + + 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) + + 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]) -> 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: + 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"}``. + """ + 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 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: + if alias.name == "ocp_resources" or alias.name.startswith("ocp_resources."): + module_names.add(alias.asname or alias.name.split(".")[0]) + + return class_aliases, module_names + + +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], + 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. + 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 + classmethod, where the client may also be passed positionally. + """ + func = node.func + + if isinstance(func, ast.Name): + 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: + return "", False + + owner = class_aliases.get(chain[-2], chain[-2]) + + if chain[-1] in CLASSMETHOD_APIS and (owner in resource_names or owner in CLASSMETHOD_ONLY_CLASSES): + return f"{owner}.{chain[-1]}", True + + # 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 + + +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. + """ + 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, class_aliases=class_aliases, module_names=module_names + ) + if not target: + continue + + keywords = {keyword.arg for keyword in node.keywords} + 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. + # 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 + 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() + example_paths = sorted(EXAMPLES_DIR.rglob("*.py")) + assert example_paths, f"no example files found under {EXAMPLES_DIR}" + + offenders: list[str] = [] + + for path in example_paths: + 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) + + +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"), + [ + 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("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"), + 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("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"), + 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. + + 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 diff --git a/tests/test_resource.py b/tests/test_resource.py index bd409eca9e..0b0709a699 100644 --- a/tests/test_resource.py +++ b/tests/test_resource.py @@ -5,7 +5,9 @@ import pytest import yaml -from ocp_resources.exceptions import ResourceTeardownError +from fake_kubernetes_client.dynamic_client import FakeDynamicClient +from ocp_resources.event import Event +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 @@ -96,6 +98,98 @@ def test_get_all_cluster_resources(self, fake_client): if _resources: break + def test_client_is_required(self, fake_client: FakeDynamicClient) -> None: + # 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(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"): + Namespace.get() + + with pytest.raises(MissingClientError, match="client is required"): + Namespace.get(client=None) # type: ignore[arg-type] + + with pytest.raises(TypeError, match="client"): + Resource.get_all_cluster_resources() + + # 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"): + Pod.get() + + 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) + assert pod.client is fake_client + + 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) + + with pytest.raises(TypeError, match="Unsupported argument"): + # value is an arbitrary placeholder; the kwarg name triggers rejection + Namespace.get(client=fake_client, config_file="kubeconfig") + + with pytest.raises(TypeError, match="Unsupported argument"): + Resource.get_all_cluster_resources(client=fake_client, context="default") + + with pytest.raises(TypeError, match="Unsupported argument"): + 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"): + Namespace.get(fake_client, fake_client) + + # get_all_cluster_resources takes client as its only positional argument + with pytest.raises(TypeError, match="positional"): + Resource.get_all_cluster_resources(fake_client, "extra") + + # 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, + 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"): + Event.get() + + # 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(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(MissingClientError, match="client is required"): + 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 diff --git a/tests/test_resources/test_migration.py b/tests/test_resources/test_migration.py index b13fdfa23b..1cf96923b4 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 @@ -64,8 +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: - """Preserve the existing positional constructor arguments. + 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 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. @@ -76,10 +79,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()