Make argocd manifest fetching more generic - #251
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates ArgoCD application manifest extraction to skip umbrella applications, match owned top-level ChangesArgoCD manifest extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ArgoApplication
participant _extract_app_manifests
participant LiveManifests
ArgoApplication->>_extract_app_manifests: provide resources, health, metadata, and source revision
_extract_app_manifests->>LiveManifests: fetch live manifests under semaphore
LiveManifests-->>_extract_app_manifests: return tracking-ID-owned apps workloads
_extract_app_manifests-->>ArgoApplication: return service metadata
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/edge_containers_cli/cmds/argo_commands.py (1)
312-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate live-manifest fetch/parse logic with
_get_service_manifest.This fetch-and-parse block (
argocd app manifests ... --source live+YAML(typ="safe").load_all(...)+isinstance(manifest, dict)guard) duplicates the same pattern in_get_service_manifest(Lines 210-220). Extracting a shared helper (e.g._get_live_manifests(namespace, name) -> Iterator[dict]) would reduce duplication and keep both call sites in sync if the command or parsing changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/edge_containers_cli/cmds/argo_commands.py` around lines 312 - 322, Extract the duplicated live-manifest command execution and YAML parsing into a shared helper, such as _get_live_manifests(namespace, name), returning only dictionary manifests. Replace the inline block in the shown resource-processing flow and the equivalent logic in _get_service_manifest with this helper, preserving semaphore usage and existing empty/non-dictionary filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/edge_containers_cli/cmds/argo_commands.py`:
- Around line 299-310: Update the fallback label lookup in the
application-processing flow to read the metadata label named description instead
of device. Keep the existing "service" default for missing labels and leave the
timestamp handling unchanged.
- Around line 352-374: Update the workload readiness logic in the manifest
selection block to use DaemonSet-specific status fields when kind is
"DaemonSet", while preserving readyReplicas handling for other workload kinds.
Guard the metadata.creationTimestamp lookup and parsing so missing or invalid
timestamps do not propagate an exception that aborts service listing.
---
Nitpick comments:
In `@src/edge_containers_cli/cmds/argo_commands.py`:
- Around line 312-322: Extract the duplicated live-manifest command execution
and YAML parsing into a shared helper, such as _get_live_manifests(namespace,
name), returning only dictionary manifests. Replace the inline block in the
shown resource-processing flow and the equivalent logic in _get_service_manifest
with this helper, preserving semaphore usage and existing empty/non-dictionary
filtering behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7af1aad-ac70-41b6-bc9f-958161e199c1
📒 Files selected for processing (1)
src/edge_containers_cli/cmds/argo_commands.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/edge_containers_cli/cmds/argo_commands.py (1)
291-297: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not classify mixed-resource applications as umbrellas.
The presence of any child
Applicationcauses an early return, even when the same app owns real workloads. Only skip after confirming that no ownedapps-group workload exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/edge_containers_cli/cmds/argo_commands.py` around lines 291 - 297, Update the umbrella-detection condition in the resource-processing flow so an application is skipped only when it has child Application resources and no owned apps-group workload. Do not return based solely on any resource with kind “Application”; inspect the existing resource metadata to confirm the absence of a real apps workload before returning, while preserving normal processing for mixed-resource applications.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/edge_containers_cli/cmds/argo_commands.py`:
- Around line 371-378: Update the workload-manifest search around the
description label lookup so it only breaks after finding a valid
`metadata.labels.description`; when the first top-level workload lacks that
label, continue scanning subsequent `apps` manifests and retain the existing
fallback behavior if none provides one.
---
Outside diff comments:
In `@src/edge_containers_cli/cmds/argo_commands.py`:
- Around line 291-297: Update the umbrella-detection condition in the
resource-processing flow so an application is skipped only when it has child
Application resources and no owned apps-group workload. Do not return based
solely on any resource with kind “Application”; inspect the existing resource
metadata to confirm the absence of a real apps workload before returning, while
preserving normal processing for mixed-resource applications.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afccab66-9898-4e9d-a2a3-f70f19e2f60e
📒 Files selected for processing (1)
src/edge_containers_cli/cmds/argo_commands.py
…nifest to update test suite
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/edge_containers_cli/cmds/argo_commands.py (1)
320-323: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard dictionary lookups on API resources to prevent
TaskGroupabortion.Unguarded accesses to expected keys can raise a
KeyErrorif the Kubernetes or ArgoCD resource is missing them (e.g., if an app usessourcesinstead ofsource). Because this runs inside aTaskGroup, a singleKeyErrorwill cancel all other concurrently running extraction tasks and abort the entirepscommand.
src/edge_containers_cli/cmds/argo_commands.py#L320-L323: use.get()with a fallback (e.g.,datetime.min) to safely read theapp'screationTimestamp.src/edge_containers_cli/cmds/argo_commands.py#L388-L391: use.get()or atry/exceptblock to safely read themanifest'screationTimestamp.src/edge_containers_cli/cmds/argo_commands.py#L397-L397: safely access the revision, e.g.,app.get("spec", {}).get("source", {}).get("targetRevision", "unknown").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/edge_containers_cli/cmds/argo_commands.py` around lines 320 - 323, Guard all specified API-resource dictionary accesses in argo_commands.py: at lines 320-323, safely retrieve the app creationTimestamp with .get() and a datetime.min fallback; at lines 388-391, safely retrieve the manifest creationTimestamp using .get() or equivalent exception handling; and at line 397, safely resolve the revision through nested .get() calls with "unknown" as the fallback, preventing extraction-task failures from aborting the TaskGroup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/edge_containers_cli/cmds/argo_commands.py`:
- Around line 381-387: Update the label discovery logic in the shown
workload-processing loop so label_set becomes True only after successfully
retrieving the description label inside the try block. Leave it false when a
KeyError occurs, allowing subsequent top-level workloads to be checked.
---
Outside diff comments:
In `@src/edge_containers_cli/cmds/argo_commands.py`:
- Around line 320-323: Guard all specified API-resource dictionary accesses in
argo_commands.py: at lines 320-323, safely retrieve the app creationTimestamp
with .get() and a datetime.min fallback; at lines 388-391, safely retrieve the
manifest creationTimestamp using .get() or equivalent exception handling; and at
line 397, safely resolve the revision through nested .get() calls with "unknown"
as the fallback, preventing extraction-task failures from aborting the
TaskGroup.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05ff7760-8615-4ddf-8f6d-bc655dd33a6e
📒 Files selected for processing (2)
src/edge_containers_cli/cmds/argo_commands.pytests/data/argocd.yaml
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
OCopping
force-pushed
the
generic-manifest-fetch
branch
2 times, most recently
from
July 20, 2026 09:47
5422f47 to
881a892
Compare
Also add a taskgroup except* handler
OCopping
force-pushed
the
generic-manifest-fetch
branch
from
July 20, 2026 09:49
881a892 to
85e92ac
Compare
Contributor
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
1 similar comment
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
the |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
gilesknap
reviewed
Jul 24, 2026
gilesknap
left a comment
gilesknap
left a comment
Member
There was a problem hiding this comment.
oooh this looks good. testing now.
gilesknap
approved these changes
Jul 24, 2026
gilesknap
left a comment
gilesknap
left a comment
Member
There was a problem hiding this comment.
nice work. This appears to do the right thing from my tests on a couple of beamlines.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
argocd.argoproj.io/tracking-id, including stronger fallbacks when annotation or related metadata is missing.argocd app listtest expectations to include an explicithealth.status: Healthyfield for the relevant entry.