fix: raise on 404 in ArgoClient.get_workflow_templates (#3239) - #3240
fix: raise on 404 in ArgoClient.get_workflow_templates (#3239)#3240Haris-bin-shakeel wants to merge 1 commit into
Conversation
Greptile SummaryThe PR makes workflow-template enumeration report Kubernetes 404 responses instead of silently presenting them as an empty namespace.
Confidence Score: 5/5The PR appears safe to merge; the implementation cleanly distinguishes an enumeration failure from a healthy empty result. No actionable regressions or outstanding findings remain, and the tests directly exercise both sides of the intended behavior distinction.
|
| Filename | Overview |
|---|---|
| metaflow/plugins/argo/argo_client.py | Correctly raises the existing client exception when workflow-template enumeration receives a 404 while preserving pagination and expired-token handling. |
| test/unit/test_argo_client.py | Adds focused tests for the changed 404 behavior and the successful empty-list case. |
Reviews (3): Last reviewed commit: "fix: raise on 404 in ArgoClient.get_work..." | Re-trigger Greptile
f9585cd to
4521262
Compare
Shriprasad-P
left a comment
There was a problem hiding this comment.
I independently reproduced the generator behavior this fixes.
On the base revision, a 404 reaches return None inside get_workflow_templates(), which terminates the generator normally. As a result, callers consuming it with list() receive [], making a failed collection request indistinguishable from a successful 200 response with an empty items list.
This change correctly preserves that distinction by raising ArgoClientException for the 404 path while leaving valid empty collections unchanged.
I also checked the existing pagination / 410 Expired handling and the singular getter behavior; both remain unaffected. The new regression test fails against the base behavior and passes with this patch.
LGTM.
A 404 from list_namespaced_custom_object is an enumeration failure, not an empty list. Return None in a generator silently yields [], which is indistinguishable from a successful empty response. Adds unit tests for the 404 and empty-200 cases.
4521262 to
f9e6894
Compare
|
Hi @mcg1969, Just rebased this PR onto the latest Thanks to @Shriprasad-P for independently reproducing and approving the fix! |
Shriprasad-P
left a comment
There was a problem hiding this comment.
Review
Makes sense to raise on 404 from get_workflow_templates instead of returning None and letting callers mis-handle a missing CRD/API as “empty list”.
The unit tests distinguish 404 → ArgoClientException vs empty 200 → [], which is exactly the behavioral split this change needs.
Commenting as a non-maintainer; change LGTM from a correctness standpoint.
PR Type
Summary
get_workflow_templatessilently yields[]on a 404, making an enumeration failure indistinguishable from a healthy empty namespace.Issue
Fixes #3239
Root Cause
return Noneinside a generator stops iteration without raising. A 404 onlist_namespaced_custom_objectmeans the namespace or CRD is missing — not "zero templates" (that's a 200 withitems: []).Why This Fix Is Correct
error_messageis already constructed before the 404 branch. Every other non-410 status in that block raisesArgoClientException(error_message)— this makes 404 consistent with that fallthrough. Singular getters keepreturn Noneon 404; there a missing named resource is the correct semantic.Tests
Two tests added:
test_get_workflow_templates_404_raises— 404 raisesArgoClientExceptiontest_get_workflow_templates_empty_200_does_not_raise— empty 200 yields[]Non-Goals
Singular getters (
get_workflow_template, etc.) intentionally not changed.AI Tool Usage
Used Cursor for initial analysis assistance. Reviewed, understood, and tested all code personally.