Skip to content

Conversation

@ogenstad
Copy link
Contributor

@ogenstad ogenstad commented Sep 16, 2025

Replaces #537 with resolved conflicts.

Summary by CodeRabbit

  • New Features

    • Added ability to create diffs between two timestamps.
    • Enhanced diff summary to support optional name and time-range filters.
  • Bug Fixes

    • Fixed escaping of special characters in GraphQL inputs to prevent mutation errors.
    • Improved branch selection in CLI and validation workflows using CLI args, environment variables, or default branch.
    • Corrected diff status reporting for nodes (e.g., now shows UPDATED where applicable).
  • Documentation

    • Added a video player to the Python SDK introduction and linked a new blog post.

minitriga and others added 9 commits September 8, 2025 17:12
…o times. (#531)

* feat: add create_diff method and update get_diff_summary to accept name parameter

* feat: enhance get_diff_summary to support time range filtering

* fix: update action field in diff summary to reflect correct status

* feat: add create_diff method and update get_diff_summary for time range support

* feat: add create_diff method and enhance get_diff_summary to support time range filtering

* fix: remove unnecessary blank lines in InfrahubClientSync class

* feat: update create_diff method to validate time range and change return type
…ables (#536)

* Fix branch handling in `_run_transform` and `execute_graphql_query` functions

* Add changelog entry for fixing branch handling in `_run_transform` and `execute_graphql_query` functions

* Remove unused import of `get_branch` from `utils.py`

* Add jinja2 transform and GraphQL query for tags, and implement branch selection test

* Update changelog to specify environment variable usage for branch management in `_run_transform` and `execute_graphql_query` functions.

* Remove unused `get_branch` import and set default branch in `validate_graphql` function
Remove unnecessary assignments before `return` statement
fix improperly escaped special characters in `HFID`
@coderabbitai
Copy link

coderabbitai bot commented Sep 16, 2025

Walkthrough

This PR introduces diff management APIs in the SDK: adds create_diff and extends get_diff_summary with optional name/from_time/to_time filters, updating related GraphQL queries. It fixes GraphQL string escaping by switching to JSON-based escaping. CLI branch handling now resolves a default branch when unspecified, with tests and fixtures added for Jinja2 transforms and branch selection. Documentation gains a VideoPlayer component, a videos section, and adds react-player dependency. Multiple files receive minor refactors (direct returns, query struct changes), repository active_branch simplification, and linter configuration adjusts RET504. Changelog entries and unit test expectations for diff actions are updated.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.91% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately describes the PR’s primary purpose—merging the 'stable' branch into 'develop' and indicating conflicts were resolved—so it is directly related to the changeset and clear for history scanning.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch pog-stable-to-develop-20250916

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the type/documentation Improvements or additions to documentation label Sep 16, 2025
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 16, 2025

Deploying infrahub-sdk-python with  Cloudflare Pages  Cloudflare Pages

Latest commit: e7ed981
Status: ✅  Deploy successful!
Preview URL: https://c1a89f43.infrahub-sdk-python.pages.dev
Branch Preview URL: https://pog-stable-to-develop-202509.infrahub-sdk-python.pages.dev

View logs

@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 37.70492% with 38 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
infrahub_sdk/client.py 22.22% 27 Missing and 8 partials ⚠️
infrahub_sdk/ctl/validate.py 33.33% 2 Missing ⚠️
infrahub_sdk/playback.py 0.00% 1 Missing ⚠️
@@             Coverage Diff             @@
##           develop     #550      +/-   ##
===========================================
- Coverage    75.97%   75.63%   -0.34%     
===========================================
  Files          100      100              
  Lines         9345     8916     -429     
  Branches      1835     1754      -81     
===========================================
- Hits          7100     6744     -356     
+ Misses        1754     1689      -65     
+ Partials       491      483       -8     
Flag Coverage Δ
integration-tests 34.63% <11.47%> (-1.55%) ⬇️
python-3.10 48.08% <29.50%> (-1.52%) ⬇️
python-3.11 48.10% <29.50%> (-1.48%) ⬇️
python-3.12 48.05% <29.50%> (-1.52%) ⬇️
python-3.13 48.08% <29.50%> (-1.50%) ⬇️
python-3.9 46.70% <26.22%> (-1.31%) ⬇️
python-filler-3.12 25.07% <9.83%> (+0.76%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
infrahub_sdk/branch.py 53.90% <100.00%> (-0.65%) ⬇️
infrahub_sdk/ctl/cli_commands.py 70.61% <100.00%> (+1.40%) ⬆️
infrahub_sdk/ctl/utils.py 68.53% <100.00%> (+0.44%) ⬆️
infrahub_sdk/diff.py 95.23% <100.00%> (-0.12%) ⬇️
infrahub_sdk/graphql.py 99.11% <100.00%> (+<0.01%) ⬆️
infrahub_sdk/node/node.py 76.13% <100.00%> (-0.06%) ⬇️
infrahub_sdk/repository.py 100.00% <100.00%> (ø)
infrahub_sdk/timestamp.py 83.78% <100.00%> (+0.74%) ⬆️
infrahub_sdk/playback.py 60.00% <0.00%> (+2.30%) ⬆️
infrahub_sdk/ctl/validate.py 52.77% <33.33%> (-0.75%) ⬇️
... and 1 more

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ogenstad ogenstad marked this pull request as ready for review September 16, 2025 08:39
@ogenstad ogenstad requested a review from a team September 16, 2025 08:39
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)
infrahub_sdk/branch.py (5)

60-71: Bug: missing “?” before query string

generate_diff_data_url concatenates params without “?”, producing invalid URLs.

Apply this diff:

-        return url + urlencode(url_params)
+        return f"{url}?{urlencode(url_params)}"

143-152: Return type mismatch in rebase (async)

Annotation says BranchData but returns a bool. Prefer returning the object since the mutation requests it.

Apply this diff:

-    async def rebase(self, branch_name: str) -> BranchData:
+    async def rebase(self, branch_name: str) -> BranchData:
@@
-        return response["BranchRebase"]["ok"]
+        return BranchData(**response["BranchRebase"]["object"])

153-173: Return type mismatch in validate (async)

Method returns a bool but is annotated as BranchData. Align the signature.

Apply this diff:

-    async def validate(self, branch_name: str) -> BranchData:
+    async def validate(self, branch_name: str) -> bool:
@@
-        return response["BranchValidate"]["ok"]
+        return response["BranchValidate"]["ok"]

339-347: Return type mismatch in rebase (sync)

Same issue as async; return the BranchData object.

Apply this diff:

-    def rebase(self, branch_name: str) -> BranchData:
+    def rebase(self, branch_name: str) -> BranchData:
@@
-        return response["BranchRebase"]["ok"]
+        return BranchData(**response["BranchRebase"]["object"])

349-369: Return type mismatch in validate (sync)

Returns a bool but annotated as BranchData. Adjust signature.

Apply this diff:

-    def validate(self, branch_name: str) -> BranchData:
+    def validate(self, branch_name: str) -> bool:
@@
-        return response["BranchValidate"]["ok"]
+        return response["BranchValidate"]["ok"]
infrahub_sdk/client.py (1)

257-274: Use address_type (not prefix_type) in IPAddressPoolGetResource input.

infrahub_sdk/client.py:257-274build_ip_address_allocation_query assigns the address_type argument to input_data["prefix_type"]. prefix_type is used by IPPrefixPool allocations; for IPAddressPoolGetResource the key should be "address_type".

-        if address_type:
-            input_data["prefix_type"] = address_type
+        if address_type:
+            input_data["address_type"] = address_type
🧹 Nitpick comments (9)
changelog/529.added.md (1)

1-2: Clarify fragment style and parameters for readability

Consider splitting into two bullet points and naming the new optional parameters explicitly (name, from_time, to_time) for clarity.

changelog/+escape-hfid.fixed.md (1)

1-1: Remove leading dash to match Towncrier fragment style

Towncrier fragments shouldn’t start with a list marker; it will add formatting during release notes generation.

Apply this diff:

- - Fixed issue with improperly escaped special characters in `hfid` fields and other string values in GraphQL mutations by implementing proper JSON-style string escaping
+ Fixed issue with improperly escaped special characters in `hfid` fields and other string values in GraphQL mutations by implementing proper JSON-style string escaping
changelog/535.fixed.md (1)

1-1: Correct the wording: resolution isn’t only via environment variables

Code resolves a default branch via client config (and CLI/env/git precedence), not strictly “use environment variables”.

Apply this diff:

-Fix branch handling in `_run_transform` and `execute_graphql_query` functions in Infrahubctl to use environment variables for branch management. 
+Fix branch handling in `_run_transform` and `execute_graphql_query` to reliably resolve a default branch (CLI arg > env var > Git active branch via client config).
infrahub_sdk/repository.py (1)

31-33: Handle detached HEAD: avoid .decode() on None.

porcelain.active_branch(...) can return None (e.g., detached HEAD). Decoding unconditionally will raise. Guard and return None per the annotated type.

Apply this diff:

     @property
     def active_branch(self) -> str | None:
-        return porcelain.active_branch(self.root_directory).decode("utf-8")
+        branch = porcelain.active_branch(self.root_directory)
+        return branch.decode("utf-8") if branch else None
docs/src/components/VideoPlayer/index.tsx (1)

1-31: SSR-safety and a11y: render on client only and add an accessible label.

Docusaurus may SSR; wrapping with BrowserOnly avoids potential SSR issues. Also expose an optional title and set aria-label.

Apply this diff:

-import React from 'react';
-import ReactPlayer from 'react-player';
+import React from 'react';
+import BrowserOnly from '@docusaurus/BrowserOnly';
+import ReactPlayer from 'react-player';
 
-interface VideoPlayerProps {
+interface VideoPlayerProps {
   url: string;
   light?: boolean;
+  title?: string;
 }
 
-export default function VideoPlayer({ url, light = false }: VideoPlayerProps) {
+export default function VideoPlayer({ url, light = false, title = 'Infrahub video' }: VideoPlayerProps) {
   return (
-    <div style={{
+    <div aria-label={title} role="region" style={{
       border: '3px solid #eee',
       borderRadius: '12px',
       padding: '8px',
       backgroundColor: '#fafafa',
       margin: '2rem 0',
-      maxWidth: '800px'
+      maxWidth: '800px'
     }}>
-      <div style={{ position: 'relative', paddingTop: '56.25%' }}>
-        <ReactPlayer
-          url={url}
-          light={light}
-          style={{ position: 'absolute', top: 0, left: 0 }}
-          width="100%"
-          height="100%"
-          controls
-        />
-      </div>
+      <div style={{ position: 'relative', paddingTop: '56.25%' }}>
+        <BrowserOnly fallback={<div style={{height: 0}} />}>
+          {() => (
+            <ReactPlayer
+              url={url}
+              light={light}
+              style={{ position: 'absolute', top: 0, left: 0 }}
+              width="100%"
+              height="100%"
+              controls
+            />
+          )}
+        </BrowserOnly>
+      </div>
     </div>
   );
 }
docs/docs/python-sdk/introduction.mdx (1)

4-12: Use Docusaurus alias and avoid deprecated <center>.

Prefer @site alias for local imports and replace <center> with a flex wrapper.

Apply this diff:

-import VideoPlayer from '../../src/components/VideoPlayer';
+import VideoPlayer from '@site/src/components/VideoPlayer';
@@
-## Videos
-
-<center>
-  <VideoPlayer url='https://www.youtube.com/live/RbRz8_t0FBs?feature=shared' light />
-</center>
+## Videos
+
+<div style={{ display: 'flex', justifyContent: 'center' }}>
+  <VideoPlayer url="https://www.youtube.com/live/RbRz8_t0FBs?feature=shared" light />
+</div>
tests/fixtures/repos/ctl_integration/templates/tags.j2 (1)

1-1: Guard against empty edges in fixtures.

Make the template resilient if no edges are returned; avoids index errors in tests.

Apply this diff:

-{{ data['BuiltinTag']['edges'][0]['node']['name']['value'] }}
+{%- set first = (data.BuiltinTag.edges | first) -%}
+{{ first.node.name.value if first else '' }}
infrahub_sdk/node/node.py (1)

579-583: Use FETCH not GENERATE in artifact_fetch support check.

Wrong constant leads to misleading error when fetch isn't supported.

Apply:

-        self._validate_artifact_support(ARTIFACT_GENERATE_FEATURE_NOT_SUPPORTED_MESSAGE)
+        self._validate_artifact_support(ARTIFACT_FETCH_FEATURE_NOT_SUPPORTED_MESSAGE)
infrahub_sdk/ctl/validate.py (1)

77-85: Print branch after fallback resolution.

Currently logs None even when default branch is used.

Apply:

-    console.print(f"[purple]Query '{query}' will be validated on branch '{branch}'.")
-
     variables_dict = parse_cli_vars(variables)

     client = initialize_client_sync()

     if not branch:
         branch = client.config.default_infrahub_branch

+    console.print(f"[purple]Query '{query}' will be validated on branch '{branch}'.")
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac60214 and e7ed981.

⛔ Files ignored due to path filters (1)
  • docs/package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (23)
  • changelog/+escape-hfid.fixed.md (1 hunks)
  • changelog/529.added.md (1 hunks)
  • changelog/535.fixed.md (1 hunks)
  • docs/docs/python-sdk/introduction.mdx (1 hunks)
  • docs/package.json (1 hunks)
  • docs/src/components/VideoPlayer/index.tsx (1 hunks)
  • infrahub_sdk/branch.py (2 hunks)
  • infrahub_sdk/client.py (25 hunks)
  • infrahub_sdk/ctl/cli_commands.py (1 hunks)
  • infrahub_sdk/ctl/utils.py (1 hunks)
  • infrahub_sdk/ctl/validate.py (2 hunks)
  • infrahub_sdk/diff.py (2 hunks)
  • infrahub_sdk/graphql.py (2 hunks)
  • infrahub_sdk/node/node.py (2 hunks)
  • infrahub_sdk/playback.py (1 hunks)
  • infrahub_sdk/repository.py (1 hunks)
  • infrahub_sdk/timestamp.py (1 hunks)
  • pyproject.toml (1 hunks)
  • tests/fixtures/repos/ctl_integration/.infrahub.yml (1 hunks)
  • tests/fixtures/repos/ctl_integration/templates/tags.j2 (1 hunks)
  • tests/fixtures/repos/ctl_integration/templates/tags_query.gql (1 hunks)
  • tests/unit/ctl/test_render_app.py (1 hunks)
  • tests/unit/sdk/test_diff_summary.py (3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method

Files:

  • infrahub_sdk/playback.py
  • infrahub_sdk/ctl/utils.py
  • infrahub_sdk/repository.py
  • infrahub_sdk/timestamp.py
  • infrahub_sdk/ctl/validate.py
  • infrahub_sdk/graphql.py
  • infrahub_sdk/ctl/cli_commands.py
  • tests/unit/sdk/test_diff_summary.py
  • infrahub_sdk/branch.py
  • tests/unit/ctl/test_render_app.py
  • infrahub_sdk/diff.py
  • infrahub_sdk/node/node.py
  • infrahub_sdk/client.py
infrahub_sdk/ctl/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

infrahub_sdk/ctl/**/*.py: Build CLI commands with Typer
Organize and keep all CLI commands within infrahub_sdk/ctl/

Files:

  • infrahub_sdk/ctl/utils.py
  • infrahub_sdk/ctl/validate.py
  • infrahub_sdk/ctl/cli_commands.py
docs/**/*.{md,mdx}

📄 CodeRabbit inference engine (CLAUDE.md)

docs/**/*.{md,mdx}: Follow the Diataxis framework and classify docs as Tutorials, How-to guides, Explanation, or Reference
Structure How-to guides with required frontmatter and sections: introduction, prerequisites, step-by-step steps, validation, related resources
Structure Topics (Explanation) docs with introduction, concepts & definitions, background & context, architecture & design, connections, further reading
Use a professional, concise, informative tone with consistent structure across documents
Use proper language tags on all code blocks
Include both async and sync examples where applicable using the Tabs component
Add validation steps to guides to confirm success and expected outputs
Use callouts for warnings, tips, and important notes
Define new terms on first use and use domain-relevant terminology consistent with Infrahub’s model/UI
Conform to markdown style rules in .markdownlint.yaml and Vale styles in .vale/styles/

Files:

  • docs/docs/python-sdk/introduction.mdx
tests/unit/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Place and write unit tests under tests/unit/ (isolated component tests)

Files:

  • tests/unit/sdk/test_diff_summary.py
  • tests/unit/ctl/test_render_app.py
tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing

Files:

  • tests/unit/sdk/test_diff_summary.py
  • tests/unit/ctl/test_render_app.py
infrahub_sdk/client.py

📄 CodeRabbit inference engine (CLAUDE.md)

infrahub_sdk/client.py: Use HTTPX for transport with proxy support (single proxy or HTTP/HTTPS mounts)
Support authentication via API tokens or JWT with automatic refresh

Files:

  • infrahub_sdk/client.py
🧬 Code graph analysis (5)
infrahub_sdk/ctl/utils.py (1)
infrahub_sdk/config.py (1)
  • default_infrahub_branch (125-130)
infrahub_sdk/ctl/validate.py (2)
infrahub_sdk/utils.py (1)
  • write_to_file (310-323)
infrahub_sdk/config.py (1)
  • default_infrahub_branch (125-130)
infrahub_sdk/ctl/cli_commands.py (1)
infrahub_sdk/utils.py (1)
  • write_to_file (310-323)
tests/unit/ctl/test_render_app.py (2)
tests/helpers/fixtures.py (1)
  • read_fixture (10-16)
tests/helpers/utils.py (1)
  • temp_repo_and_cd (31-42)
infrahub_sdk/client.py (4)
infrahub_sdk/branch.py (3)
  • get (193-203)
  • get (233-243)
  • BranchData (17-25)
infrahub_sdk/types.py (1)
  • HTTPMethod (13-15)
infrahub_sdk/graphql.py (3)
  • Mutation (173-204)
  • render (161-170)
  • render (181-204)
infrahub_sdk/diff.py (1)
  • get_diff_summary_query (38-74)
🔇 Additional comments (21)
infrahub_sdk/graphql.py (1)

3-3: Good fix: use JSON escaping for GraphQL strings

json.dumps() properly escapes quotes, backslashes, and control chars; this removes injection/parse bugs in inline args. Please add unit tests covering quotes, backslashes, newlines, and non‑ASCII.

Also applies to: 22-24

pyproject.toml (1)

273-274: Enable RET504 for prod code; ignore in tests is sensible

Change looks good and aligns with cleaner returns in the SDK, while keeping tests flexible.

infrahub_sdk/playback.py (1)

59-59: LGTM: direct return is cleaner

No behavior change; simpler and clearer.

infrahub_sdk/branch.py (2)

191-192: LGTM: direct dict comprehension return

Minor readability improvement without changing behavior.


231-232: LGTM: direct dict comprehension return (sync)

Matches async variant; consistent.

docs/package.json (1)

25-26: react-player v3.3.2 bundles TypeScript defs; verify SSR usage in Docusaurus

v3.3.2 ships with bundled .d.ts — @types/react-player not required. If ReactPlayer is used in the docs, ensure client-only usage (dynamic import or guard with typeof window !== 'undefined') and run a docusaurus build to catch SSR errors. Location: docs/package.json lines 25–26.

infrahub_sdk/timestamp.py (1)

52-55: Inline return is fine; no behavior change.

Directly returning ZonedDateTime.parse_common_iso(value) is clear and equivalent to the previous code.

tests/fixtures/repos/ctl_integration/templates/tags_query.gql (1)

1-11: LGTM.

Query shape matches the template usage and variable is correctly typed non-null.

infrahub_sdk/ctl/cli_commands.py (1)

49-49: Import addition is fine.

write_to_file is used below; import placement is consistent.

tests/unit/sdk/test_diff_summary.py (1)

112-112: LGTM: expectations updated to use status-derived action.

Switch to "UPDATED" matches diff.py mapping from status → action.

Also applies to: 127-127, 143-143

infrahub_sdk/node/node.py (2)

582-582: LGTM: direct return simplifies control flow.


1207-1211: LGTM: sync artifact_fetch mirrors async path and returns directly.

infrahub_sdk/diff.py (2)

40-43: LGTM: query now accepts name/from_time/to_time filters.


120-127: LGTM: action sourced from status; direct NodeDiff return is clearer.

tests/fixtures/repos/ctl_integration/.infrahub.yml (2)

29-33: LGTM: jinja2 transform wired to tags_query and template.


37-38: LGTM: adds tags_query fixture for render tests.

tests/unit/ctl/test_render_app.py (3)

78-87: LGTM: parameterized branch-source selection test.


93-103: LGTM: URL includes expected branch; HTTPXMock ensures correct path is hit.


104-116: LGTM: env/CLI permutations exercise default-branch logic.

infrahub_sdk/client.py (2)

1269-1286: LGTM: get_diff_summary passes filters as variables and uses branch path.


1584-1589: LGTM: allocation calls consistently pass branch/tracker/timeout.

Comment on lines +1230 to +1257
async def create_diff(
self,
branch: str,
name: str,
from_time: datetime,
to_time: datetime,
wait_until_completion: bool = True,
) -> bool | str:
if from_time > to_time:
raise ValueError("from_time must be <= to_time")
input_data = {
"wait_until_completion": wait_until_completion,
"data": {
"name": name,
"branch": branch,
"from_time": from_time.isoformat(),
"to_time": to_time.isoformat(),
},
}

mutation_query = MUTATION_QUERY_TASK if not wait_until_completion else {"ok": None}
query = Mutation(mutation="DiffUpdate", input_data=input_data, query=mutation_query)
response = await self.execute_graphql(query=query.render(), tracker="mutation-diff-update")

if not wait_until_completion and "task" in response["DiffUpdate"]:
return response["DiffUpdate"]["task"]["id"]

return response["DiffUpdate"]["ok"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Create diff executes against default branch instead of provided branch.

Missing branch_name causes HTTP request to target default branch; results may be wrong.

Apply:

-        response = await self.execute_graphql(query=query.render(), tracker="mutation-diff-update")
+        response = await self.execute_graphql(
+            query=query.render(),
+            branch_name=branch,
+            tracker="mutation-diff-update",
+        )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async def create_diff(
self,
branch: str,
name: str,
from_time: datetime,
to_time: datetime,
wait_until_completion: bool = True,
) -> bool | str:
if from_time > to_time:
raise ValueError("from_time must be <= to_time")
input_data = {
"wait_until_completion": wait_until_completion,
"data": {
"name": name,
"branch": branch,
"from_time": from_time.isoformat(),
"to_time": to_time.isoformat(),
},
}
mutation_query = MUTATION_QUERY_TASK if not wait_until_completion else {"ok": None}
query = Mutation(mutation="DiffUpdate", input_data=input_data, query=mutation_query)
response = await self.execute_graphql(query=query.render(), tracker="mutation-diff-update")
if not wait_until_completion and "task" in response["DiffUpdate"]:
return response["DiffUpdate"]["task"]["id"]
return response["DiffUpdate"]["ok"]
async def create_diff(
self,
branch: str,
name: str,
from_time: datetime,
to_time: datetime,
wait_until_completion: bool = True,
) -> bool | str:
if from_time > to_time:
raise ValueError("from_time must be <= to_time")
input_data = {
"wait_until_completion": wait_until_completion,
"data": {
"name": name,
"branch": branch,
"from_time": from_time.isoformat(),
"to_time": to_time.isoformat(),
},
}
mutation_query = MUTATION_QUERY_TASK if not wait_until_completion else {"ok": None}
query = Mutation(mutation="DiffUpdate", input_data=input_data, query=mutation_query)
response = await self.execute_graphql(
query=query.render(),
branch_name=branch,
tracker="mutation-diff-update",
)
if not wait_until_completion and "task" in response["DiffUpdate"]:
return response["DiffUpdate"]["task"]["id"]
return response["DiffUpdate"]["ok"]
🤖 Prompt for AI Agents
infrahub_sdk/client.py around lines 1230 to 1257: the GraphQL input is missing
the expected branch_name field so the request falls back to the default branch;
adjust the payload so the API receives the branch name in the correct key — add
a top-level "branch_name": branch (or rename the existing "data.branch" to the
API-expected "branch_name") in input_data before rendering the mutation so the
request targets the provided branch.

mutation_query = MUTATION_QUERY_TASK if not wait_until_completion else {"ok": None}
query = Mutation(mutation="DiffUpdate", input_data=input_data, query=mutation_query)
response = self.execute_graphql(query=query.render(), tracker="mutation-diff-update")

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Same issue in sync create_diff.

Apply:

-        response = self.execute_graphql(query=query.render(), tracker="mutation-diff-update")
+        response = self.execute_graphql(
+            query=query.render(),
+            branch_name=branch,
+            tracker="mutation-diff-update",
+        )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
response = self.execute_graphql(
query=query.render(),
branch_name=branch,
tracker="mutation-diff-update",
)
🤖 Prompt for AI Agents
In infrahub_sdk/client.py around line 2460, the synchronous create_diff
implementation has the same bug as the async create_diff; apply the identical
fix from the async version: make input validation consistent, wrap the core
logic in a try/except block, log the caught exception with details, and ensure
the function returns the same result/structure (or re-raises) as the async
variant so behavior is consistent across both implementations.

Comment on lines +121 to +124

if not branch:
branch = client.config.default_infrahub_branch

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Bug: calling a method-like config attribute without parentheses.

client.config.default_infrahub_branch appears to be a method (see config snippet). Assigning it directly yields a bound method, not a str, which will break execute_graphql. Call it.

Apply this diff:

-    if not branch:
-        branch = client.config.default_infrahub_branch
+    if not branch:
+        branch = client.config.default_infrahub_branch()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if not branch:
branch = client.config.default_infrahub_branch
if not branch:
branch = client.config.default_infrahub_branch()
🤖 Prompt for AI Agents
In infrahub_sdk/ctl/utils.py around lines 121 to 124, the code assigns
client.config.default_infrahub_branch directly which is a method-like attribute;
change the assignment to call the method (e.g.,
client.config.default_infrahub_branch()) so branch becomes a string, not a bound
method, ensuring execute_graphql receives the correct type.

@ogenstad ogenstad merged commit 654fc80 into develop Sep 17, 2025
20 checks passed
@ogenstad ogenstad deleted the pog-stable-to-develop-20250916 branch September 17, 2025 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants