[Integration][ArgoCD] Remove cluster-based filtering from get_resources to fix selector compatibility#3027
[Integration][ArgoCD] Remove cluster-based filtering from get_resources to fix selector compatibility#3027
Conversation
…ontains no items, preventing unnecessary processing of empty data sets.
…l. Removed the `skip_unavailable_clusters` parameter and consolidated resource fetching logic, enhancing code clarity and maintainability.
…t.toml to reflect bug fix for ArgoCD compatibility
Review Summary by QodoRemove cluster-based filtering from ArgoCD resource retrieval
WalkthroughsDescription• Removed cluster-based selector filtering from get_resources method • Simplified resource retrieval to fetch all resources without per-cluster filtering • Eliminated skip_unavailable_clusters parameter and get_available_clusters method • Added early return in get_paginated_resources for empty API responses • Updated all tests to reflect simplified client API Diagramflowchart LR
A["get_resources_for_available_clusters<br/>with cluster selector"] -->|Refactored| B["get_resources<br/>without selector"]
C["get_available_clusters<br/>filter logic"] -->|Removed| D["Simplified API calls"]
E["Empty response handling"] -->|Added| F["Early return optimization"]
B --> G["All resources fetched<br/>directly from API"]
File Changes1. integrations/argocd/client.py
|
Code Review by Qodo
1. Unbounded non-streaming fetch
|
There was a problem hiding this comment.
Pull request overview
This PR updates the ArgoCD integration client to avoid using cluster-based selector=cluster=<name> filtering when listing resources, improving compatibility with ArgoCD instances that don’t support that selector.
Changes:
- Simplified resource fetching to retrieve all resources without per-cluster selector filtering.
- Removed cluster availability filtering API surface (
skip_unavailable_clusters,get_available_clusters) and updated call sites/tests accordingly. - Added an early exit in non-streaming pagination when the API returns no
items.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| integrations/argocd/client.py | Removes cluster-based resource fetching and simplifies get_clusters; adds early return when items is empty. |
| integrations/argocd/main.py | Switches resync handlers to call get_resources instead of cluster-filtered resource fetching. |
| integrations/argocd/tests/test_client.py | Updates tests to reflect simplified APIs and removal of cluster selector logic. |
| integrations/argocd/pyproject.toml | Bumps integration version to 0.2.31. |
| integrations/argocd/CHANGELOG.md | Adds release note for selector compatibility fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async for cluster in argocd_client.get_resources( | ||
| resource_kind=ObjectKind(kind) | ||
| ): | ||
| yield cluster |
There was a problem hiding this comment.
The loop variable name cluster is now misleading: get_resources no longer yields per-cluster batches, it yields generic resource batches. Rename the loop variable (e.g., resource_batch) to avoid confusion and reduce the chance of future logic bugs.
| async for cluster in argocd_client.get_resources( | |
| resource_kind=ObjectKind(kind) | |
| ): | |
| yield cluster | |
| async for resource_batch in argocd_client.get_resources( | |
| resource_kind=ObjectKind(kind) | |
| ): | |
| yield resource_batch |
integrations/argocd/pyproject.toml
Outdated
| [tool.poetry] | ||
| name = "argocd" | ||
| version = "0.2.30" | ||
| version = "0.2.31" |
There was a problem hiding this comment.
Version was bumped as a patch release, but this PR removes/renames client methods/parameters (breaking API changes). To stay consistent with SemVer, either restore backward compatibility (deprecated aliases) or bump the minor version and note breaking changes in the changelog/towncrier output.
| version = "0.2.31" | |
| version = "0.3.0" |
|
|
||
| - Removed cluster-based `selector` filtering from `get_resources` to fix compatibility with ArgoCD instances that do not support cluster selector queries | ||
|
|
||
|
|
There was a problem hiding this comment.
The release note describes a bugfix, but the PR also removes/renames public client APIs (get_resources_for_available_clusters, skip_unavailable_clusters). If you keep these breaking changes, they should be called out explicitly (and ideally categorized under "Breaking Changes" per towncrier config), or otherwise add deprecated compatibility wrappers.
| ### Breaking Changes | |
| - Removed/renamed public client APIs `get_resources_for_available_clusters` and `skip_unavailable_clusters`. Callers relying on these helpers must migrate to the new APIs or use `get_resources` directly. |
| async def get_clusters( | ||
| self, skip_unavailable_clusters: bool = False | ||
| ) -> AsyncGenerator[list[dict[str, Any]], None]: | ||
| async def get_clusters(self) -> AsyncGenerator[list[dict[str, Any]], None]: |
There was a problem hiding this comment.
get_clusters removed the skip_unavailable_clusters parameter, which is a backward-incompatible API change. Since the project states it follows Semantic Versioning, either keep the parameter (deprecated/no-op) for compatibility or bump the minor version and document this as a breaking change.
| async def get_clusters(self) -> AsyncGenerator[list[dict[str, Any]], None]: | |
| async def get_clusters( | |
| self, skip_unavailable_clusters: Optional[bool] = None | |
| ) -> AsyncGenerator[list[dict[str, Any]], None]: | |
| if skip_unavailable_clusters is not None: | |
| logger.warning( | |
| "The 'skip_unavailable_clusters' parameter in get_clusters is deprecated " | |
| "and has no effect. It will be removed in a future version." | |
| ) |
| async for resources in stream_async_iterators_tasks(*tasks): | ||
| async for resources in self.get_paginated_resources(url): | ||
| yield resources | ||
|
|
There was a problem hiding this comment.
get_resources_for_available_clusters was removed/renamed to get_resources, which can break external consumers importing ArgocdClient from this package. Consider reintroducing get_resources_for_available_clusters as a deprecated wrapper around get_resources (or bump minor and mark as breaking in release notes).
| async def get_resources_for_available_clusters( | |
| self, resource_kind: ObjectKind | |
| ) -> AsyncGenerator[list[dict[str, Any]], None]: | |
| """ | |
| Deprecated wrapper for `get_resources`. | |
| This method is kept for backwards compatibility with external consumers | |
| that relied on `get_resources_for_available_clusters`. It will be removed | |
| in a future release; prefer calling `get_resources` directly. | |
| """ | |
| logger.warning( | |
| f"get_resources_for_available_clusters is deprecated. {DEPRECATION_WARNING}" | |
| ) | |
| async for resources in self.get_resources(resource_kind=resource_kind): | |
| yield resources |
Remove check for empty 'items' in API response.
…placed cluster name filtering with query parameters for application kind. Added new integration.py file for application resource configuration and selector handling. Updated client methods to support query parameters in resource fetching. Enhanced tests to validate new functionality.
| async for cluster in argocd_client.get_resources( | ||
| resource_kind=ObjectKind(kind) | ||
| ): | ||
| yield cluster |
integrations/argocd/main.py
Outdated
| params: dict[str, Any] | None = None | ||
| parsed_kind = ObjectKind(kind) | ||
| if parsed_kind == ObjectKind.APPLICATION: | ||
| selector = cast(ApplicationResourceConfig, event.resource_config).selector | ||
| params = selector.query_params | ||
| async for cluster in argocd_client.get_resources( | ||
| resource_kind=parsed_kind, query_params=params |
There was a problem hiding this comment.
isolate/detach the application kind from this generic
|
|
||
| async for app_batch in argocd_client.get_resources_for_available_clusters( | ||
| async for app_batch in argocd_client.get_resources( | ||
| resource_kind=ObjectKind.APPLICATION | ||
| ): |
There was a problem hiding this comment.
what happens to managed resources ? query params on application doesn't apply ?
How do we provide capability of getting managed resources for applications that are only within available clusters for instance
There was a problem hiding this comment.
@mk-armah I have extended the managed-resource kind to accept similar query params to filter out applications before fetching the former
…for application handling. Updated client methods and tests to align with new resource kind structure, enhancing clarity and consistency in resource fetching and configuration.
…y parameter handling. Added ApplicationQueryParams model for better application filtering and updated methods to utilize this new structure, ensuring more robust resource fetching and clearer debug information.
integrations/argocd/integration.py
Outdated
| @property | ||
| def generate_request_params(self) -> dict[str, Any]: | ||
| params: dict[str, Any] = {} | ||
| if self.selector: | ||
| params["selector"] = self.selector | ||
| if self.app_namespace: | ||
| params["appNamespace"] = self.app_namespace | ||
| if self.projects: | ||
| params["projects"] = self.projects | ||
| if self.resource_version: | ||
| params["resourceVersion"] = self.resource_version | ||
| if self.repo: | ||
| params["repo"] = self.repo | ||
| return params |
There was a problem hiding this comment.
use model_dump(exclude=None) or its equivalent on the v1 pydantic
integrations/argocd/integration.py
Outdated
|
|
||
| class ManagedResourceResourceConfig(ResourceConfig): | ||
| kind: Literal["managed-resource"] | ||
| selector: ApplicationSelector |
There was a problem hiding this comment.
ApplicationSelector shouldn't be a top level selector as its not controlling the ManagedResource kind directly.
So the experience should be:
selector:
- appFilters:
# application selectors follows
…r handling. Replaced manual parameter construction in ApplicationQueryParams with a direct dictionary conversion, and introduced ManagedResourceSelector to encapsulate application filters for improved resource configuration.
…ndling and ObjectKind definitions in a new misc.py file.
Description
The default behavior of pulling all applications fails when the
selector=cluster=<cluster-name>parameter is added to API requests. Not all ArgoCD instances have this filtering capability, causing requests to fail or return incomplete data. This reverts to the simpler behavior of fetching all resources directly without per-cluster filtering.Summary
Type of change
Please leave one option from the following and delete the rest:
All tests should be run against the port production environment(using a testing org).
Integration testing checklist
Preflight checklist
Screenshots
API Documentation
Provide links to the API documentation used for this integration.