fix tracked commit has on repository when using an annoted git tag as the ref#8463
fix tracked commit has on repository when using an annoted git tag as the ref#8463wvandeun merged 2 commits intorelease-1.8from
Conversation
github-actions
Bot
added
the
group/backend
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
wvandeun
force-pushed
the
fix-commit-hash-ref
branch
from
b79626d to
2da2e10
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/infrahub/git/repository.py`:
- Around line 268-270: The code calls the synchronous GitPython method
git_repo.commit(...) inside an async flow which can block the event loop; change
that call to run in a thread (e.g. use asyncio.to_thread or
loop.run_in_executor) to obtain the commit object and convert it to a string
before continuing, then pass that string to sync_from_remote(commit=...) and
update_commit_value(branch_name=self.infrahub_branch_name, commit=...); update
the variable name (e.g. latest_commit_str) if helpful to distinguish the
threaded result and keep the subsequent await calls (sync_from_remote and
update_commit_value) unchanged.
In `@backend/tests/component/git/test_git_read_only_repository.py`:
- Around line 110-138: The test function
test_update_latest_commit_with_annotated_tag has a plain string docstring;
replace it with a Google‑style docstring (or remove it) — if keeping, add
"Args:" describing no parameters, "Returns:" describing None, and an "Examples:"
section showing expected behavior, ensuring you do not include type annotations
in the docstring; update the docstring located inside the
test_update_latest_commit_with_annotated_tag function accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/git/repository.pybackend/tests/component/git/test_git_read_only_repository.py
| latest_commit = str(git_repo.commit(latest_commit)) | ||
| await self.sync_from_remote(commit=latest_commit) | ||
| await self.update_commit_value(branch_name=self.infrahub_branch_name, commit=latest_commit) |
There was a problem hiding this comment.
Avoid new sync GitPython call in async flow
Line 268 adds a synchronous GitPython call inside an async method, which can block the event loop. Please offload it to a thread (or use an async wrapper) to comply with backend async I/O rules.
♻️ Suggested fix (offload to thread)
+import asyncio
@@
- latest_commit = str(git_repo.commit(latest_commit))
+ latest_commit = str(await asyncio.to_thread(git_repo.commit, latest_commit))As per coding guidelines, "Use async/await for all I/O operations" and "Never block event loop with sync I/O".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/infrahub/git/repository.py` around lines 268 - 270, The code calls
the synchronous GitPython method git_repo.commit(...) inside an async flow which
can block the event loop; change that call to run in a thread (e.g. use
asyncio.to_thread or loop.run_in_executor) to obtain the commit object and
convert it to a string before continuing, then pass that string to
sync_from_remote(commit=...) and
update_commit_value(branch_name=self.infrahub_branch_name, commit=...); update
the variable name (e.g. latest_commit_str) if helpful to distinguish the
threaded result and keep the subsequent await calls (sync_from_remote and
update_commit_value) unchanged.
| @patch("infrahub.git.integrator.InfrahubRepositoryIntegrator.import_objects_from_files", new_callable=AsyncMock) | ||
| async def test_update_latest_commit_with_annotated_tag( | ||
| mock_import_objects: AsyncMock, | ||
| git_upstream_repo_01: dict[str, str | Path], | ||
| git_repos_dir: Path, | ||
| ) -> None: | ||
| """Verify that update_latest_commit stores the commit SHA, not the tag object SHA, for annotated tags.""" | ||
| upstream = Repo(git_upstream_repo_01["path"]) | ||
| tag_name = "v1.0.0" | ||
| expected_commit = str(upstream.commit("main")) | ||
| upstream.create_tag(tag_name, ref="main", message="Release v1.0.0") | ||
|
|
||
| repo = await InfrahubReadOnlyRepository.new( | ||
| id=UUIDT.new(), | ||
| name=git_upstream_repo_01["name"], | ||
| location=str(git_upstream_repo_01["path"]), | ||
| ref=tag_name, | ||
| infrahub_branch_name="main", | ||
| client=InfrahubClient(config=Config(requester=dummy_async_request)), | ||
| service=await InfrahubServices.new(), | ||
| ) | ||
| mock_client = AsyncMock(InfrahubClient) | ||
| repo.client = mock_client | ||
|
|
||
| await repo.update_latest_commit() | ||
|
|
||
| mock_client.repository_update_commit.assert_awaited_with( | ||
| branch_name="main", repository_id=repo.id, commit=expected_commit, is_read_only=True | ||
| ) |
There was a problem hiding this comment.
Use Google‑style docstring sections
The new test’s docstring doesn’t follow the required Google‑style format (missing Args/Returns/Examples). Please expand it or remove the docstring if not needed.
As per coding guidelines, "Follow Google-style docstring format in Python" and "Include Args/Parameters section in Python docstrings without typing information" and "Include Returns section in Python docstrings" and "Include Examples section in Python docstrings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/component/git/test_git_read_only_repository.py` around lines
110 - 138, The test function test_update_latest_commit_with_annotated_tag has a
plain string docstring; replace it with a Google‑style docstring (or remove it)
— if keeping, add "Args:" describing no parameters, "Returns:" describing None,
and an "Examples:" section showing expected behavior, ensuring you do not
include type annotations in the docstring; update the docstring located inside
the test_update_latest_commit_with_annotated_tag function accordingly.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
backend/infrahub/git/repository.py (1)
261-270:⚠️ Potential issue | 🟠 MajorAvoid blocking the event loop with GitPython commit resolution.
git_repo.commit(...)is synchronous and can perform disk I/O inside an async method. Offload it to a worker thread to comply with async I/O rules.♻️ Proposed fix (offload to thread)
+import asyncio- latest_commit = str(git_repo.commit(latest_commit)) + latest_commit_obj = await asyncio.to_thread(git_repo.commit, latest_commit) + latest_commit = str(latest_commit_obj)As per coding guidelines, "Use async/await for all I/O operations" and "Never block event loop with sync I/O".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/git/repository.py` around lines 261 - 270, The call to git_repo.commit(latest_commit) blocks the event loop; replace the direct synchronous call in repository.py with an offloaded call executed on a worker thread (e.g., use asyncio.get_running_loop().run_in_executor or loop.run_in_executor) and await the result, then convert it to str as before and continue to await self.sync_from_remote(...) and self.update_commit_value(...); ensure you still catch/propagate errors in the same places and only change the git_repo.commit(...) invocation to run in the executor.backend/tests/component/git/test_git_read_only_repository.py (1)
110-116:⚠️ Potential issue | 🟡 MinorUpdate the test docstring to Google-style sections (or remove it).
Current docstring is not in the required Google-style format. Please expand it to include Args/Returns/Raises/Examples, or remove it entirely.
📝 Example compliant docstring
- """Verify that update_latest_commit stores the commit SHA, not the tag object SHA, for annotated tags.""" + """ + Verify that update_latest_commit stores the commit SHA, not the tag object SHA, for annotated tags. + + Args: + None. + + Returns: + None. + + Raises: + None. + + Examples: + When ref is an annotated tag, the stored commit equals the tag target SHA. + """As per coding guidelines, "Follow Google-style docstring format in Python" and "Include Args/Parameters section in Python docstrings without typing information" and "Include Returns section in Python docstrings" and "Include Raises section in Python docstrings" and "Include Examples section in Python docstrings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/component/git/test_git_read_only_repository.py` around lines 110 - 116, The test docstring for test_update_latest_commit_with_annotated_tag is not in Google-style; either remove the one-line docstring or replace it with a Google-style docstring containing at least Args (describe fixtures used), Returns (None), Raises (if any), and Examples (if helpful); update the docstring text for the test_update_latest_commit_with_annotated_tag function accordingly so it follows the project's Google-style Python docstring conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/infrahub/git/repository.py`:
- Around line 261-270: The call to git_repo.commit(latest_commit) blocks the
event loop; replace the direct synchronous call in repository.py with an
offloaded call executed on a worker thread (e.g., use
asyncio.get_running_loop().run_in_executor or loop.run_in_executor) and await
the result, then convert it to str as before and continue to await
self.sync_from_remote(...) and self.update_commit_value(...); ensure you still
catch/propagate errors in the same places and only change the
git_repo.commit(...) invocation to run in the executor.
In `@backend/tests/component/git/test_git_read_only_repository.py`:
- Around line 110-116: The test docstring for
test_update_latest_commit_with_annotated_tag is not in Google-style; either
remove the one-line docstring or replace it with a Google-style docstring
containing at least Args (describe fixtures used), Returns (None), Raises (if
any), and Examples (if helpful); update the docstring text for the
test_update_latest_commit_with_annotated_tag function accordingly so it follows
the project's Google-style Python docstring conventions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/git/repository.pybackend/tests/component/git/test_git_read_only_repository.py
2 participants
Why
In #8169 we implemented a capability to use git tags as a ref for integrated repositories. When using a annoted tag, the tracked commit in of the repository in Infrahub was using the hash of the tag object rather than the commit id that the tag references.
Checklist
Summary by CodeRabbit
Bug Fixes
Tests