Remove raise_for_error from several methods#874
Conversation
This parameter has been deprecated for a while and should not be used anymore.
github-actions
Bot
added
the
type/documentation
WalkthroughThe pull request removes the deprecated 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #874 +/- ##
====================================================
+ Coverage 80.62% 80.77% +0.14%
====================================================
Files 119 119
Lines 10335 10311 -24
Branches 1550 1529 -21
====================================================
- Hits 8333 8329 -4
+ Misses 1475 1473 -2
+ Partials 527 509 -18
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/docs/python-sdk/sdk_ref/infrahub_sdk/client.mdx (2)
225-245:⚠️ Potential issue | 🟡 MinorDocument the default HTTP exceptions for
execute_graphql().
raise_for_erroris gone, so non-2xx responses are now part of the default contract, but theRaisessection still only listsGraphQLError. Please update the source docstrings to mention the HTTP/auth exceptions callers now need to handle, then regenerate this reference.Based on learnings, do not edit
docs/python-sdk/sdk_ref/**/*.mdxfiles directly; regenerate withuv run invoke docs-generate.Also applies to: 506-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/python-sdk/sdk_ref/infrahub_sdk/client.mdx` around lines 225 - 245, The docstring for execute_graphql() must be updated to document that non-2xx HTTP responses and authentication errors can be raised (in addition to GraphQLError); update the source function's docstring (execute_graphql in the client class) to list HTTPError/RequestException (or your project's HTTP/auth exception types) under Raises and describe when they occur, then regenerate the SDK reference HTML/MDX by running the docs generation task (uv run invoke docs-generate) rather than editing docs/python-sdk/sdk_ref/*.mdx directly; apply the same docstring change to the other affected execute_graphql occurrences mentioned.
290-323:⚠️ Potential issue | 🟡 MinorThe allocation docblocks still describe non-optional returns.
These signatures now return
... | None, but the prose below still promises anInfrahubNodeonly, and the sync prefix block still refers tosizeinstead ofprefix_length. Please fix the source docstrings and regenerate so the narrative matches the API surface.Based on learnings, do not edit
docs/python-sdk/sdk_ref/**/*.mdxfiles directly; regenerate withuv run invoke docs-generate.Also applies to: 328-361, 676-709, 714-747
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/python-sdk/sdk_ref/infrahub_sdk/client.mdx` around lines 290 - 323, The docstrings for allocate_next_ip_address (and the other affected allocation methods) still describe a non-optional InfrahubNode return and reference a "size" parameter in the sync prefix block even though the API signatures now return Optional (e.g., SchemaType | None) and use prefix_length; update the original Python docstrings for allocate_next_ip_address (and the other listed allocator methods) to state the return may be None (InfrahubNode | None) and replace any "size" mentions with "prefix_length" in the sync prefix examples/descriptions, then regenerate the SDK docs (do NOT edit docs/python-sdk/sdk_ref/**/*.mdx directly) by running `uv run invoke docs-generate`.infrahub_sdk/client.py (1)
945-965:⚠️ Potential issue | 🔴 CriticalRe-raise unexpected HTTP status codes instead of silently replaying the request.
When an
httpx.HTTPStatusErroris raised with a status code outside {401, 403, 404}, the exception is caught but not re-raised. Withretry_on_failure=True, the loop silently retries untilmax_retry_durationexpires—dangerous for mutations since retries aren't guaranteed idempotent. With retries disabled, the failed response is treated as successful and passed todecode_json().🔧 Minimal fix
except httpx.HTTPStatusError as exc: if exc.response.status_code in {401, 403}: response = decode_json(response=exc.response) errors = response.get("errors", []) messages = [error.get("message") for error in errors] raise AuthenticationError(" | ".join(messages)) from exc if exc.response.status_code == 404: raise URLNotFoundError(url=url) from exc + raiseAlso applies to: line 1852–1872 (sync version)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrahub_sdk/client.py` around lines 945 - 965, The HTTPStatusError except block around the async request (the block handling resp = await self._post(...) and httpx.HTTPStatusError as exc) only handles 401/403 and 404 but swallows all other status codes, causing silent retries or treating failures as success; update that except block to re-raise the original exc for any status codes not explicitly handled (i.e., after handling 401/403 -> raise AuthenticationError and 404 -> raise URLNotFoundError, add a final raise exc), and mirror the same change in the sync equivalent (the similar except block around the non-async _post call) so unexpected HTTP errors are not silently replayed or passed to decode_json().
🧹 Nitpick comments (1)
changelog/+infp380.removed.md (1)
1-1: Call out theTypeErrormigration break explicitly.The entry says the argument is gone, but not what existing callers will actually see. Adding a short note that passing
raise_for_errornow raisesTypeErrorwill make the upgrade impact much easier to spot.✍️ Suggested wording
- Removed the deprecated `raise_for_error` argument from `execute_graphql`, `query_gql_query`, `get_diff_summary`, `allocate_next_ip_address`, and `allocate_next_ip_prefix` client methods. HTTP errors are now always raised via `resp.raise_for_status()`. + Removed the deprecated `raise_for_error` argument from `execute_graphql`, `query_gql_query`, `get_diff_summary`, `allocate_next_ip_address`, and `allocate_next_ip_prefix` client methods. HTTP errors are now always raised via `resp.raise_for_status()`. Passing `raise_for_error` to these methods now raises `TypeError`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@changelog/`+infp380.removed.md at line 1, Update the changelog entry to explicitly note that callers who pass the removed raise_for_error argument will now receive a TypeError; edit the sentence mentioning the removed argument for execute_graphql, query_gql_query, get_diff_summary, allocate_next_ip_address, and allocate_next_ip_prefix to add a short parenthetical or following sentence like “Passing raise_for_error will now raise TypeError” so the migration break is clear to readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/docs/python-sdk/sdk_ref/infrahub_sdk/client.mdx`:
- Around line 225-245: The docstring for execute_graphql() must be updated to
document that non-2xx HTTP responses and authentication errors can be raised (in
addition to GraphQLError); update the source function's docstring
(execute_graphql in the client class) to list HTTPError/RequestException (or
your project's HTTP/auth exception types) under Raises and describe when they
occur, then regenerate the SDK reference HTML/MDX by running the docs generation
task (uv run invoke docs-generate) rather than editing
docs/python-sdk/sdk_ref/*.mdx directly; apply the same docstring change to the
other affected execute_graphql occurrences mentioned.
- Around line 290-323: The docstrings for allocate_next_ip_address (and the
other affected allocation methods) still describe a non-optional InfrahubNode
return and reference a "size" parameter in the sync prefix block even though the
API signatures now return Optional (e.g., SchemaType | None) and use
prefix_length; update the original Python docstrings for
allocate_next_ip_address (and the other listed allocator methods) to state the
return may be None (InfrahubNode | None) and replace any "size" mentions with
"prefix_length" in the sync prefix examples/descriptions, then regenerate the
SDK docs (do NOT edit docs/python-sdk/sdk_ref/**/*.mdx directly) by running `uv
run invoke docs-generate`.
In `@infrahub_sdk/client.py`:
- Around line 945-965: The HTTPStatusError except block around the async request
(the block handling resp = await self._post(...) and httpx.HTTPStatusError as
exc) only handles 401/403 and 404 but swallows all other status codes, causing
silent retries or treating failures as success; update that except block to
re-raise the original exc for any status codes not explicitly handled (i.e.,
after handling 401/403 -> raise AuthenticationError and 404 -> raise
URLNotFoundError, add a final raise exc), and mirror the same change in the sync
equivalent (the similar except block around the non-async _post call) so
unexpected HTTP errors are not silently replayed or passed to decode_json().
---
Nitpick comments:
In `@changelog/`+infp380.removed.md:
- Line 1: Update the changelog entry to explicitly note that callers who pass
the removed raise_for_error argument will now receive a TypeError; edit the
sentence mentioning the removed argument for execute_graphql, query_gql_query,
get_diff_summary, allocate_next_ip_address, and allocate_next_ip_prefix to add a
short parenthetical or following sentence like “Passing raise_for_error will now
raise TypeError” so the migration break is clear to readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0a82ae3-4f8c-4f22-9cc1-f9d43c0f54a1
📒 Files selected for processing (5)
changelog/+infp380.removed.mddocs/docs/python-sdk/sdk_ref/infrahub_sdk/client.mdxinfrahub_sdk/client.pyinfrahub_sdk/ctl/utils.pyinfrahub_sdk/ctl/validate.py
💤 Files with no reviewable changes (2)
- infrahub_sdk/ctl/validate.py
- infrahub_sdk/ctl/utils.py
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
LGTM, perhaps you can just create a test PR with your branch against the develop branch of Infrahub so that we can see if this argument is defined anywhere without our code.
It is used in some Infrahub backend tests that I will change in a dedicated PR, by using the commit produced by the merge of this PR for the SDK submodule. |
This parameter has been deprecated for a while and should not be used anymore.
This parameter has been deprecated for a while and should not be used anymore.
2 participants
Why
The
raise_for_errorparameter was deprecated in v1.15.0 (#508) with aDeprecationWarning. It is now time to fully remove it to clean up the public API.What changed
raise_for_errorparameter from both async (InfrahubClient) and sync (InfrahubClientSync) versions of:execute_graphqlquery_gql_queryget_diff_summaryallocate_next_ip_addressallocate_next_ip_prefixresp.raise_for_status()is now always called inexecute_graphqlandquery_gql_query(previously conditional onraise_for_error).raise_for_error_deprecation_warninghelper.raise_for_error=True.How to review
How to test
Impact & rollout
raise_for_error=...to these methods will get aTypeError. This was deprecated since v1.15.0 with aDeprecationWarning.Checklist
Summary by CodeRabbit
Breaking Changes
raise_for_errorparameter from client methodsNonein some cases