IFC-1977: Add git config for user name and email, set config on repo #7544
Conversation
github-actions
bot
added
the
group/backend
WalkthroughAdded two optional fields to GitSettings: Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
github-actions
bot
added
the
type/documentation
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/infrahub/config.py(2 hunks)backend/infrahub/git/base.py(3 hunks)backend/tests/unit/git/test_git_repository.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/git/test_git_repository.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/unit/git/test_git_repository.pybackend/infrahub/git/base.pybackend/infrahub/config.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/tests/unit/git/test_git_repository.pybackend/infrahub/git/base.pybackend/infrahub/config.py
🧬 Code graph analysis (2)
backend/tests/unit/git/test_git_repository.py (5)
backend/tests/unit/git/conftest.py (2)
git_upstream_repo_01(71-85)client(27-28)backend/tests/unit/conftest.py (1)
git_repos_dir(148-154)backend/infrahub/config.py (1)
git(876-877)backend/tests/helpers/test_client.py (1)
dummy_async_request(10-14)backend/infrahub/git/base.py (5)
directory_root(194-196)directory_branches(204-206)directory_commits(209-211)directory_temp(214-216)get_git_repo_main(266-279)
backend/infrahub/git/base.py (1)
backend/infrahub/config.py (1)
git(876-877)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-docker-integration
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-tests-integration
- GitHub Check: backend-tests-unit
- GitHub Check: backend-tests-functional
🔇 Additional comments (4)
backend/infrahub/config.py (1)
16-16: LGTM! Git user configuration fields added correctly.The addition of
user_nameanduser_emailfields toGitSettingsfollows existing patterns. Using Pydantic'sEmailStrtype ensures automatic email validation, and making both fields optional maintains backward compatibility.Also applies to: 448-449
backend/infrahub/git/base.py (2)
19-19: LGTM! Configuration import added correctly.The import enables access to git configuration settings for the new repository setup logic.
408-408: LGTM! Repository config applied at the right time.Calling
create_repo_config(repo)after successful cloning ensures the git user identity is configured before any commits are made in the repository.backend/tests/unit/git/test_git_repository.py (1)
15-15: LGTM! Config import needed for test.The import enables the test to configure and verify git user settings.
backend/infrahub/git/base.py
Outdated
| @staticmethod | ||
| def create_repo_config(repo: Repo) -> None: | ||
| if config.SETTINGS.git.user_name and config.SETTINGS.git.user_email: | ||
| with repo.config_writer() as git_config: | ||
| git_config.set_value("user", "name", config.SETTINGS.git.user_name) | ||
| git_config.set_value("user", "email", config.SETTINGS.git.user_email) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Add docstring and verify the AND logic requirement.
-
Missing docstring: The coding guidelines require Google-style docstrings for all Python functions. Please add a docstring explaining the method's purpose, parameters, and behavior.
-
Verify AND condition: Line 422 requires both
user_nameANDuser_emailto be set before applying either. This means if a user configures only one field, neither will be applied to the repository. Is this the intended behavior, or should each field be set independently if available?
As per coding guidelines.
Apply this diff to add a docstring:
@staticmethod
def create_repo_config(repo: Repo) -> None:
+ """Configure local git repository with user identity settings.
+
+ Sets user.name and user.email in the repository's local git configuration
+ when both values are provided in the application settings.
+
+ Args:
+ repo: The Git repository object to configure
+ """
if config.SETTINGS.git.user_name and config.SETTINGS.git.user_email:
with repo.config_writer() as git_config:
git_config.set_value("user", "name", config.SETTINGS.git.user_name)
git_config.set_value("user", "email", config.SETTINGS.git.user_email)Consider whether each field should be set independently:
@staticmethod
def create_repo_config(repo: Repo) -> None:
"""Configure local git repository with user identity settings.
Sets user.name and user.email in the repository's local git configuration
based on the application settings. Each field is set independently if provided.
Args:
repo: The Git repository object to configure
"""
with repo.config_writer() as git_config:
if config.SETTINGS.git.user_name:
git_config.set_value("user", "name", config.SETTINGS.git.user_name)
if config.SETTINGS.git.user_email:
git_config.set_value("user", "email", config.SETTINGS.git.user_email)I'll verify the review comment by examining the actual code and its context.
---
🏁 Script executed:
```shell
#!/bin/bash
# View the method and surrounding context
cat backend/infrahub/git/base.py | sed -n '405,430p'
Length of output: 1164
🏁 Script executed:
#!/bin/bash
# Search for tests related to create_repo_config
fd -e py -x grep -l "create_repo_config" {}Length of output: 30
🏁 Script executed:
#!/bin/bash
# Check if there are any other git config setup patterns in the codebase
rg "config_writer|set_value.*user" -n -C 2 --type pyLength of output: 632
🏁 Script executed:
#!/bin/bash
# Look for SETTINGS.git configuration to understand the pattern
rg "SETTINGS\.git\.(user_name|user_email)" -n --type pyLength of output: 734
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the test file to see how create_repo_config is tested
sed -n '1080,1110p' backend/tests/unit/git/test_git_repository.py
Length of output: 1064
🏁 Script executed:
#!/bin/bash
# Look for more test context around this method
sed -n '1070,1115p' backend/tests/unit/git/test_git_repository.pyLength of output: 1369
🏁 Script executed:
#!/bin/bash
# Check if there are any tests with partial configuration (only one field)
rg "user_email|user_name" backend/tests/unit/git/test_git_repository.py -B 2 -A 2Length of output: 406
Add missing docstring per coding guidelines; AND condition appears intentional based on test patterns.
The method lacks a docstring, which violates the Python coding guidelines requiring Google-style docstrings for all functions. The test test_new_repo_has_config confirms both user_name and user_email are always configured together and reset together, indicating the AND condition (line 422) is intentional for atomicity—either both fields are applied or neither is applied. No partial configuration scenarios are tested, suggesting this is the designed behavior.
Add the docstring:
@staticmethod
def create_repo_config(repo: Repo) -> None:
+ """Configure git repository with user identity settings.
+
+ Sets user.name and user.email in the repository's local git configuration
+ when both values are provided in the application settings.
+
+ Args:
+ repo: The Git repository object to configure
+ """
if config.SETTINGS.git.user_name and config.SETTINGS.git.user_email:
with repo.config_writer() as git_config:
git_config.set_value("user", "name", config.SETTINGS.git.user_name)
git_config.set_value("user", "email", config.SETTINGS.git.user_email)📝 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.
| @staticmethod | |
| def create_repo_config(repo: Repo) -> None: | |
| if config.SETTINGS.git.user_name and config.SETTINGS.git.user_email: | |
| with repo.config_writer() as git_config: | |
| git_config.set_value("user", "name", config.SETTINGS.git.user_name) | |
| git_config.set_value("user", "email", config.SETTINGS.git.user_email) | |
| @staticmethod | |
| def create_repo_config(repo: Repo) -> None: | |
| """Configure git repository with user identity settings. | |
| Sets user.name and user.email in the repository's local git configuration | |
| when both values are provided in the application settings. | |
| Args: | |
| repo: The Git repository object to configure | |
| """ | |
| if config.SETTINGS.git.user_name and config.SETTINGS.git.user_email: | |
| with repo.config_writer() as git_config: | |
| git_config.set_value("user", "name", config.SETTINGS.git.user_name) | |
| git_config.set_value("user", "email", config.SETTINGS.git.user_email) |
🤖 Prompt for AI Agents
In backend/infrahub/git/base.py around lines 420 to 426, the static method
create_repo_config lacks a Google-style docstring; add a docstring that briefly
describes the purpose (apply git user.name and user.email to the given Repo only
when both are present), documents parameters (repo: Repo) and return type
(None), and notes the intentional atomic behavior that both user_name and
user_email must be set together (no partial application) to match tests.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/docs/reference/configuration.mdx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
docs/docs/reference/**/*.{md,mdx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write reference documentation in
docs/docs/reference/
Files:
docs/docs/reference/configuration.mdx
docs/**/*.mdx
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Docusaurus markdown/MDX features in documentation
docs/**/*.mdx: How-to guides should begin title with "How to..." and follow the Diataxis How-to structure (intro, prerequisites, step-by-step, validation, advanced, related)
Use imperative, task-focused instructions in guides; avoid digressions
Topics should use titles like "About..." or "Understanding..." and follow the Diataxis Topics structure (concepts, background, architecture, mental models, connections, alternatives, further reading)
Define new terms on first use; prefer domain-relevant terminology; keep naming consistent with Infrahub UI/data model
Files:
docs/docs/reference/configuration.mdx
docs/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Develop documentation in
docs/, preview withinvoke docs.build docs.serve, and validate withinvoke docs.validate
Files:
docs/docs/reference/configuration.mdx
**/*.mdx
📄 CodeRabbit inference engine (.github/instructions/documentation.instructions.md)
**/*.mdx: Structure documentation primarily as How-to Guides and Topics (explanations) following the Diataxis framework
Use a professional, approachable tone; avoid jargon unless defined; use plain language with technical precision
Write concise, direct, active sentences
Be informative over promotional; focus on explaining how and why
Maintain consistent, predictable structure across sections and documents
For Guides: use conditional imperatives (e.g., "If you want X, do Y")
For Guides: focus on practical tasks and problems, not tools
For Guides: address the user directly with imperative verbs (e.g., "Configure...", "Create...")
For Guides: keep focus on the specific goal; avoid digressions into explanations
For Guides: title in YAML frontmatter should clearly state the problem and begin with "How to..."
For Guides: include an Introduction stating the problem/goal, context, and what the user will achieve
For Guides: include a Prerequisites/Assumptions section listing requirements and prior knowledge
For Guides: provide step-by-step instructions with numbered steps; include code snippets/screenshots/tabs/callouts as needed
For Guides: include a Validation/Verification section with checks, example outputs, and potential failure points
For Guides: include a Related Resources section linking to relevant guides/topics/reference
For Topics: title in YAML frontmatter should clearly indicate the topic; consider starting with "About..." or "Understanding..."
For Topics: include an Introduction with overview, why it matters, and questions answered
For Topics: include sections for Concepts & Definitions
For Topics: include Background & Context (history, design rationale, constraints)
For Topics: include Architecture & Design details/diagrams when applicable
For Topics: include Mental Models (analogies/comparisons)
For Topics: connect to other concepts and integration points
For Topics: include Alternative Approaches with pros/cons where relevant
For Topics: include a Further...
Files:
docs/docs/reference/configuration.mdx
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Markdown/MDX files with markdownlint using
.markdownlint.yaml
Files:
docs/docs/reference/configuration.mdx
docs/!(node_modules)/**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
docs/!(node_modules)/**/*.{md,mdx}: Lint documentation prose with Vale
Ensure Vale style checks pass for documentation
Files:
docs/docs/reference/configuration.mdx
🧠 Learnings (3)
📚 Learning: 2025-08-21T11:04:26.357Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: .github/instructions/documentation.instructions.md:0-0
Timestamp: 2025-08-21T11:04:26.357Z
Learning: Applies to **/*.mdx : Ensure content is accurate and reflects the latest Infrahub version
Applied to files:
docs/docs/reference/configuration.mdx
📚 Learning: 2025-08-21T11:04:26.357Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: .github/instructions/documentation.instructions.md:0-0
Timestamp: 2025-08-21T11:04:26.357Z
Learning: Applies to **/*.mdx : Assume readers know Git/CI/CD/YAML-JSON/IaC basics; do not assume prior Infrahub knowledge—introduce core concepts from first principles
Applied to files:
docs/docs/reference/configuration.mdx
📚 Learning: 2025-07-22T08:13:56.198Z
Learnt from: CR
Repo: opsmill/infrahub PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-22T08:13:56.198Z
Learning: Infrahub offers a central hub to manage the data, templates and playbooks that powers your infrastructure by combining the version control and branch management capabilities similar to Git with the flexible data model and UI of a graph database
Applied to files:
docs/docs/reference/configuration.mdx
🪛 LanguageTool
docs/docs/reference/configuration.mdx
[style] ~55-~55: It’s more common nowadays to write this noun as one word.
Context: ...] | None | | INFRAHUB_GIT_USER_NAME | User name of the git user | None | None | | `INFR...
(RECOMMENDED_COMPOUNDS)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-playwright
- GitHub Check: documentation
- GitHub Check: backend-tests-unit
- GitHub Check: backend-tests-integration
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-functional
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/unit/git/conftest.py (1)
1352-1358: Add docstring and type hint to fixture.The fixture definition is correct and follows proper setup-yield-teardown pattern. Consider adding a docstring and return type hint as per coding guidelines.
Apply this diff:
+from typing import Generator + @pytest.fixture -def git_user_config(): +def git_user_config() -> Generator[None, None, None]: + """Set git user config for testing. + + Sets config.SETTINGS.git.user_email to '[email protected]' and + config.SETTINGS.git.user_name to 'Test User', then resets them after the test. + """ config.SETTINGS.git.user_email = "[email protected]" config.SETTINGS.git.user_name = "Test User" yield config.SETTINGS.git.user_email = None config.SETTINGS.git.user_name = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/unit/git/conftest.py(2 hunks)backend/tests/unit/git/test_git_repository.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/unit/git/test_git_repository.py
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/git/conftest.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/unit/git/conftest.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/tests/unit/git/conftest.py
🧬 Code graph analysis (1)
backend/tests/unit/git/conftest.py (1)
backend/infrahub/config.py (1)
git(876-877)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-validate-generated
- GitHub Check: backend-docker-integration
- GitHub Check: documentation
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-integration
- GitHub Check: backend-tests-unit
🔇 Additional comments (1)
backend/tests/unit/git/conftest.py (1)
17-17: LGTM!The import is necessary for the new
git_user_configfixture.
CodSpeed Performance ReportMerging #7544 will not alter performanceComparing Summary
|
backend/infrahub/git/base.py
Outdated
| if config.SETTINGS.git.user_name and config.SETTINGS.git.user_email: | ||
| with repo.config_writer() as git_config: | ||
| git_config.set_value("user", "name", config.SETTINGS.git.user_name) | ||
| git_config.set_value("user", "email", config.SETTINGS.git.user_email) |
There was a problem hiding this comment.
is it possible to have a username without an email or vice versa?
There was a problem hiding this comment.
I think it would be possible if a someone just configures an override for one of them.
I'm not sure where this should be implemented. I was considering to add it as a global commit option when someone starts up the prefect worker. I.e. that they would just run "git config --global" once at that point during startup. Then we don't need to deal with it each time we do something locally in the repo. It would also allow users to change it globally with a config setting.
The annoying thing with this would be that if you ran the infrahub prefect worker locally on your laptop and had this option enabled it would change the global git config...
What do you guys think of this?
There was a problem hiding this comment.
That works, I can move the implementation if we are agreed on that
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/infrahub/workers/infrahub_async.py(3 hunks)backend/tests/integration/conftest.py(1 hunks)backend/tests/integration/workers/test_infrahubasync.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/integration/workers/test_infrahubasync.pybackend/tests/integration/conftest.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/integration/workers/test_infrahubasync.pybackend/tests/integration/conftest.pybackend/infrahub/workers/infrahub_async.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/tests/integration/workers/test_infrahubasync.pybackend/tests/integration/conftest.pybackend/infrahub/workers/infrahub_async.py
🧬 Code graph analysis (3)
backend/tests/integration/workers/test_infrahubasync.py (1)
backend/tests/integration/conftest.py (1)
git_user_config(148-155)
backend/tests/integration/conftest.py (1)
backend/infrahub/config.py (1)
git(876-877)
backend/infrahub/workers/infrahub_async.py (1)
backend/infrahub/config.py (1)
git(876-877)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
| async def set_git_global_config(self) -> None: | ||
| if config.SETTINGS.git.user_name: | ||
| proc_name = await asyncio.create_subprocess_exec( | ||
| "git", | ||
| "config", | ||
| "--global", | ||
| "user.name", | ||
| config.SETTINGS.git.user_name, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) | ||
| await proc_name.wait() | ||
| self._logger.info("Git user name set") | ||
|
|
||
| if config.SETTINGS.git.user_email: | ||
| proc_email = await asyncio.create_subprocess_exec( | ||
| "git", | ||
| "config", | ||
| "--global", | ||
| "user.email", | ||
| config.SETTINGS.git.user_email, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) | ||
| await proc_email.wait() | ||
| self._logger.info("Git user email set") |
There was a problem hiding this comment.
Fail fast when git config command errors
Both subprocess calls ignore the return code and log success unconditionally. If git is missing or the command fails (bad HOME, permissions, etc.), the worker quietly proceeds with a bad identity and the log claims success. Capture stderr, check returncode, and raise so setup aborts when configuration cannot be applied.
@@
- if config.SETTINGS.git.user_name:
- proc_name = await asyncio.create_subprocess_exec(
- "git",
- "config",
- "--global",
- "user.name",
- config.SETTINGS.git.user_name,
- stdout=asyncio.subprocess.PIPE,
- stderr=asyncio.subprocess.PIPE,
- )
- await proc_name.wait()
- self._logger.info("Git user name set")
+ if config.SETTINGS.git.user_name:
+ proc_name = await asyncio.create_subprocess_exec(
+ "git",
+ "config",
+ "--global",
+ "user.name",
+ config.SETTINGS.git.user_name,
+ stdout=asyncio.subprocess.DEVNULL,
+ stderr=asyncio.subprocess.PIPE,
+ )
+ _, stderr_name = await proc_name.communicate()
+ if proc_name.returncode != 0:
+ error_msg = stderr_name.decode("utf-8", errors="ignore").strip() or "unknown error"
+ self._logger.error("Failed to set git user.name: %s", error_msg)
+ raise RuntimeError("Unable to configure git user.name")
+ self._logger.info("Git user name set")
@@
- if config.SETTINGS.git.user_email:
- proc_email = await asyncio.create_subprocess_exec(
- "git",
- "config",
- "--global",
- "user.email",
- config.SETTINGS.git.user_email,
- stdout=asyncio.subprocess.PIPE,
- stderr=asyncio.subprocess.PIPE,
- )
- await proc_email.wait()
- self._logger.info("Git user email set")
+ if config.SETTINGS.git.user_email:
+ proc_email = await asyncio.create_subprocess_exec(
+ "git",
+ "config",
+ "--global",
+ "user.email",
+ config.SETTINGS.git.user_email,
+ stdout=asyncio.subprocess.DEVNULL,
+ stderr=asyncio.subprocess.PIPE,
+ )
+ _, stderr_email = await proc_email.communicate()
+ if proc_email.returncode != 0:
+ error_msg = stderr_email.decode("utf-8", errors="ignore").strip() or "unknown error"
+ self._logger.error("Failed to set git user.email: %s", error_msg)
+ raise RuntimeError("Unable to configure git user.email")
+ self._logger.info("Git user email set")📝 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.
| async def set_git_global_config(self) -> None: | |
| if config.SETTINGS.git.user_name: | |
| proc_name = await asyncio.create_subprocess_exec( | |
| "git", | |
| "config", | |
| "--global", | |
| "user.name", | |
| config.SETTINGS.git.user_name, | |
| stdout=asyncio.subprocess.PIPE, | |
| stderr=asyncio.subprocess.PIPE, | |
| ) | |
| await proc_name.wait() | |
| self._logger.info("Git user name set") | |
| if config.SETTINGS.git.user_email: | |
| proc_email = await asyncio.create_subprocess_exec( | |
| "git", | |
| "config", | |
| "--global", | |
| "user.email", | |
| config.SETTINGS.git.user_email, | |
| stdout=asyncio.subprocess.PIPE, | |
| stderr=asyncio.subprocess.PIPE, | |
| ) | |
| await proc_email.wait() | |
| self._logger.info("Git user email set") | |
| async def set_git_global_config(self) -> None: | |
| if config.SETTINGS.git.user_name: | |
| proc_name = await asyncio.create_subprocess_exec( | |
| "git", | |
| "config", | |
| "--global", | |
| "user.name", | |
| config.SETTINGS.git.user_name, | |
| stdout=asyncio.subprocess.DEVNULL, | |
| stderr=asyncio.subprocess.PIPE, | |
| ) | |
| _, stderr_name = await proc_name.communicate() | |
| if proc_name.returncode != 0: | |
| error_msg = stderr_name.decode("utf-8", errors="ignore").strip() or "unknown error" | |
| self._logger.error("Failed to set git user.name: %s", error_msg) | |
| raise RuntimeError("Unable to configure git user.name") | |
| self._logger.info("Git user name set") | |
| if config.SETTINGS.git.user_email: | |
| proc_email = await asyncio.create_subprocess_exec( | |
| "git", | |
| "config", | |
| "--global", | |
| "user.email", | |
| config.SETTINGS.git.user_email, | |
| stdout=asyncio.subprocess.DEVNULL, | |
| stderr=asyncio.subprocess.PIPE, | |
| ) | |
| _, stderr_email = await proc_email.communicate() | |
| if proc_email.returncode != 0: | |
| error_msg = stderr_email.decode("utf-8", errors="ignore").strip() or "unknown error" | |
| self._logger.error("Failed to set git user.email: %s", error_msg) | |
| raise RuntimeError("Unable to configure git user.email") | |
| self._logger.info("Git user email set") |
🤖 Prompt for AI Agents
In backend/infrahub/workers/infrahub_async.py around lines 206 to 231, the two
git subprocess invocations currently call wait() and unconditionally log
success; change them to run the process and capture stdout/stderr (e.g., via
proc.communicate()), check proc.returncode, and if non-zero log the stderr and
raise an exception to fail fast; include the stderr (and returncode) in the
error message so setup aborts when git is missing or the config command fails.
This PR creates new git configurations that are then set on the repository. The created configurations are - [x] `user_name` - The user name of the git user - [x] `user_email` - The email of the git user
solababs
force-pushed
the
sb-20251103-git-config-user-name-and-email-ifc-1977
branch
2 times, most recently
from
91a70ba to
2740532
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/integration/workers/test_infrahubasync.py (1)
148-155: Consider adding worker cleanup.The test creates a worker instance but doesn't clean it up. While the test logic is correct, consider calling a cleanup method (e.g.,
await worker.shutdown()if available) to prevent potential resource leaks.Example cleanup pattern:
async def test_worker_has_set_git_user_config(self, client, work_pool, git_global_user_config) -> None: worker = InfrahubWorkerAsync(work_pool_name=work_pool.name) - await worker.setup(client=client, metric_port=0) - user_name = await self._run_git_command("config", "--global", "--get", "user.name") - assert user_name == "Test User" + try: + await worker.setup(client=client, metric_port=0) + user_name = await self._run_git_command("config", "--global", "--get", "user.name") + assert user_name == "Test User" - user_email = await self._run_git_command("config", "--global", "--get", "user.email") - assert user_email == "[email protected]" + user_email = await self._run_git_command("config", "--global", "--get", "user.email") + assert user_email == "[email protected]" + finally: + # Add cleanup if worker has a shutdown/cleanup method + pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/integration/workers/test_infrahubasync.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/integration/workers/test_infrahubasync.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/integration/workers/test_infrahubasync.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/tests/integration/workers/test_infrahubasync.py
🧬 Code graph analysis (1)
backend/tests/integration/workers/test_infrahubasync.py (1)
backend/infrahub/config.py (1)
git(896-897)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-playwright
- GitHub Check: validate-generated-documentation
- GitHub Check: documentation
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-integration
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-unit
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
backend/tests/integration/workers/test_infrahubasync.py (2)
1-2: LGTM! Import additions are appropriate.The new imports (
asyncio,AsyncGenerator, andconfig) are necessary for the git configuration testing functionality.Also applies to: 11-11
113-121: LGTM! Helper method is well-implemented.The
_run_git_commandhelper correctly usesasyncio.create_subprocess_exec, properly awaits completion viacommunicate(), and returns stdout on success orNoneon failure.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/integration/workers/test_infrahubasync.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/integration/workers/test_infrahubasync.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/integration/workers/test_infrahubasync.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/tests/integration/workers/test_infrahubasync.py
🧬 Code graph analysis (1)
backend/tests/integration/workers/test_infrahubasync.py (2)
backend/infrahub/config.py (1)
git(896-897)backend/infrahub/workers/infrahub_async.py (2)
InfrahubWorkerAsync(64-235)setup(74-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: documentation
- GitHub Check: backend-validate-generated
- GitHub Check: validate-generated-documentation
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/tests/integration/workers/test_infrahubasync.py (3)
116-124: Consider logging stderr for debugging.The helper correctly executes Git commands but discards stderr. While not critical for passing tests, logging stderr on non-zero exit codes could aid troubleshooting.
Example enhancement:
async def _run_git_command(self, *args: str) -> str | None: proc = await asyncio.create_subprocess_exec( "git", *args, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, ) stdout, stderr = await proc.communicate() - return stdout.decode().strip() if proc.returncode == 0 else None + if proc.returncode == 0: + return stdout.decode().strip() + # Optional: log stderr for debugging + return None
127-168: Prefer pytest's monkeypatch and tmp_path fixtures for cleaner resource management.The current implementation manually manages environment variables and temporary files, and includes assertions testing the fixture's own setup (lines 140, 160, 163). Using pytest's built-in fixtures would simplify cleanup, eliminate manual lifecycle management, and remove the need for self-testing assertions.
Apply this refactor to leverage pytest fixtures:
@pytest.fixture -async def git_global_user_config(self) -> AsyncGenerator[None, None]: +async def git_global_user_config( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> AsyncGenerator[None, None]: async def set_git_config(key: str, value: str | None) -> None: if value is not None: proc = await asyncio.create_subprocess_exec("git", "config", "--global", key, value) else: proc = await asyncio.create_subprocess_exec("git", "config", "--global", "--unset", key) await proc.wait() - with tempfile.NamedTemporaryFile(delete=False) as tmpfile: - tmp_git_config = tmpfile.name - - previous_git_config_global = os.getenv("GIT_CONFIG_GLOBAL") - os.environ["GIT_CONFIG_GLOBAL"] = tmp_git_config - assert os.getenv("GIT_CONFIG_GLOBAL") and os.getenv("GIT_CONFIG_GLOBAL") == tmp_git_config + git_config_file = tmp_path / "gitconfig" + monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(git_config_file)) initial_user_name = config.SETTINGS.git.user_name initial_user_email = config.SETTINGS.git.user_email user_name = "Test User" user_email = "[email protected]" await set_git_config("user.name", user_name) await set_git_config("user.email", user_email) config.SETTINGS.git.user_name = user_name config.SETTINGS.git.user_email = user_email - yield + try: + yield + finally: + config.SETTINGS.git.user_name = initial_user_name + config.SETTINGS.git.user_email = initial_user_email - config.SETTINGS.git.user_name = initial_user_name - config.SETTINGS.git.user_email = initial_user_email - - if previous_git_config_global: - os.environ["GIT_CONFIG_GLOBAL"] = previous_git_config_global - assert os.getenv("GIT_CONFIG_GLOBAL") and os.getenv("GIT_CONFIG_GLOBAL") == previous_git_config_global - else: - os.environ.pop("GIT_CONFIG_GLOBAL", None) - assert os.getenv("GIT_CONFIG_GLOBAL") is None - - try: - Path(tmp_git_config).unlink() - except FileNotFoundError: - passBenefits:
tmp_pathautomatically cleans up the temp directorymonkeypatchautomatically restores environment variables- No need for assertions testing fixture setup
- Explicit try/finally ensures config restoration even if test fails
170-178: Remove assertion testing fixture setup.Line 171 asserts the fixture's internal state rather than the worker's behavior. The test should focus solely on verifying that
InfrahubWorkerAsync.setup()correctly configures Git.async def test_worker_has_set_git_user_config(self, client, work_pool, git_global_user_config) -> None: - assert os.getenv("GIT_CONFIG_GLOBAL") is not None worker = InfrahubWorkerAsync(work_pool_name=work_pool.name) await worker.setup(client=client, metric_port=0) user_name = await self._run_git_command("config", "--global", "--get", "user.name") assert user_name == "Test User" user_email = await self._run_git_command("config", "--global", "--get", "user.email") assert user_email == "[email protected]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/integration/workers/test_infrahubasync.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/integration/workers/test_infrahubasync.py
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/tests/integration/workers/test_infrahubasync.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
backend/tests/integration/workers/test_infrahubasync.py
🧬 Code graph analysis (1)
backend/tests/integration/workers/test_infrahubasync.py (2)
backend/infrahub/config.py (1)
git(896-897)backend/infrahub/workers/infrahub_async.py (2)
InfrahubWorkerAsync(64-235)setup(74-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-docker-integration
- GitHub Check: documentation
- GitHub Check: backend-tests-integration
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-unit
🔇 Additional comments (1)
backend/tests/integration/workers/test_infrahubasync.py (1)
1-5: LGTM!All new imports are relevant and properly support the Git configuration testing functionality.
Also applies to: 14-14
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/infrahub/workers/infrahub_async.py(5 hunks)backend/tests/integration/workers/test_infrahubasync.py(3 hunks)development/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/infrahub/workers/infrahub_async.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/integration/workers/test_infrahubasync.py (2)
backend/infrahub/config.py (1)
git(896-897)backend/infrahub/workers/infrahub_async.py (1)
InfrahubWorkerAsync(69-244)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-benchmark
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-integration
- GitHub Check: documentation
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-unit
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
development/Dockerfile (1)
81-83: LGTM! Git config path properly established.The environment variable and directory creation correctly set up the global git config location for the worker. The directory is created before the git config commands run on lines 85-89, ensuring proper initialization.
backend/tests/integration/workers/test_infrahubasync.py (3)
1-14: LGTM! Necessary imports added.All new imports (
asyncio,os,tempfile,Path, andconfig) are appropriately used in the test implementation.
116-124: LGTM! Clean helper implementation.The
_run_git_commandmethod correctly executes git commands asynchronously and returns the output or None on failure.
167-175: Test logic is sound but depends on fixture correction.The test correctly verifies that git config is set after
worker.setup()runs. Once the fixture issue (lines 126-166) is addressed, this test will properly validate that the worker—not the fixture—writes the git configuration.
| @pytest.fixture | ||
| async def git_global_user_config(self) -> AsyncGenerator[None, None]: | ||
| async def set_git_config(key: str, value: str | None) -> None: | ||
| if value is not None: | ||
| proc = await asyncio.create_subprocess_exec("git", "config", "--global", key, value) | ||
| else: | ||
| proc = await asyncio.create_subprocess_exec("git", "config", "--global", "--unset", key) | ||
| await proc.wait() | ||
|
|
||
| with tempfile.NamedTemporaryFile(delete=False) as tmpfile: | ||
| tmp_git_config = tmpfile.name | ||
|
|
||
| previous_git_config_global = os.getenv("GIT_CONFIG_GLOBAL") | ||
| os.environ["GIT_CONFIG_GLOBAL"] = tmp_git_config | ||
| assert os.getenv("GIT_CONFIG_GLOBAL") and os.getenv("GIT_CONFIG_GLOBAL") == tmp_git_config | ||
|
|
||
| initial_user_name = config.SETTINGS.git.user_name | ||
| initial_user_email = config.SETTINGS.git.user_email | ||
|
|
||
| user_name = "Test User" | ||
| user_email = "[email protected]" | ||
|
|
||
| await set_git_config("user.name", user_name) | ||
| await set_git_config("user.email", user_email) | ||
| config.SETTINGS.git.user_name = user_name | ||
| config.SETTINGS.git.user_email = user_email | ||
|
|
||
| yield | ||
|
|
||
| config.SETTINGS.git.user_name = initial_user_name | ||
| config.SETTINGS.git.user_email = initial_user_email | ||
|
|
||
| if previous_git_config_global: | ||
| os.environ["GIT_CONFIG_GLOBAL"] = previous_git_config_global | ||
| assert os.getenv("GIT_CONFIG_GLOBAL") and os.getenv("GIT_CONFIG_GLOBAL") == previous_git_config_global | ||
| else: | ||
| os.environ.pop("GIT_CONFIG_GLOBAL", None) | ||
| assert os.getenv("GIT_CONFIG_GLOBAL") is None | ||
|
|
||
| Path(tmp_git_config).unlink() | ||
|
|
There was a problem hiding this comment.
Test fixture defeats its own purpose by pre-setting git config.
The fixture sets git config values on lines 148-149 before the test runs, which means the test at lines 171-175 verifies values the fixture already wrote—not values the worker wrote. Even if worker.setup() does nothing, the test passes.
To properly test the worker, the fixture should only prepare the environment (temp config file, GIT_CONFIG_GLOBAL, and config.SETTINGS.git) but not pre-populate the git config. Let the worker's setup() method write the values, then verify.
Additionally:
- Line 165: use
Path(tmp_git_config).unlink(missing_ok=True)to avoid errors if the file was already removed. - Consider using pytest's
monkeypatchfixture for cleaner env var management instead of manualos.environmanipulation.
Apply this diff to fix the test logic:
@pytest.fixture
- async def git_global_user_config(self) -> AsyncGenerator[None, None]:
- async def set_git_config(key: str, value: str | None) -> None:
- if value is not None:
- proc = await asyncio.create_subprocess_exec("git", "config", "--global", key, value)
- else:
- proc = await asyncio.create_subprocess_exec("git", "config", "--global", "--unset", key)
- await proc.wait()
-
+ async def git_global_user_config(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> AsyncGenerator[None, None]:
- with tempfile.NamedTemporaryFile(delete=False) as tmpfile:
- tmp_git_config = tmpfile.name
-
- previous_git_config_global = os.getenv("GIT_CONFIG_GLOBAL")
- os.environ["GIT_CONFIG_GLOBAL"] = tmp_git_config
- assert os.getenv("GIT_CONFIG_GLOBAL") and os.getenv("GIT_CONFIG_GLOBAL") == tmp_git_config
+ tmp_git_config = tmp_path / "gitconfig"
+ monkeypatch.setenv("GIT_CONFIG_GLOBAL", str(tmp_git_config))
initial_user_name = config.SETTINGS.git.user_name
initial_user_email = config.SETTINGS.git.user_email
- user_name = "Test User"
- user_email = "[email protected]"
-
- await set_git_config("user.name", user_name)
- await set_git_config("user.email", user_email)
- config.SETTINGS.git.user_name = user_name
- config.SETTINGS.git.user_email = user_email
+ # Set config values for worker to use, but don't pre-populate git config
+ config.SETTINGS.git.user_name = "Test User"
+ config.SETTINGS.git.user_email = "[email protected]"
yield
config.SETTINGS.git.user_name = initial_user_name
config.SETTINGS.git.user_email = initial_user_email
-
- if previous_git_config_global:
- os.environ["GIT_CONFIG_GLOBAL"] = previous_git_config_global
- assert os.getenv("GIT_CONFIG_GLOBAL") and os.getenv("GIT_CONFIG_GLOBAL") == previous_git_config_global
- else:
- os.environ.pop("GIT_CONFIG_GLOBAL", None)
- assert os.getenv("GIT_CONFIG_GLOBAL") is None
-
- Path(tmp_git_config).unlink()
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/infrahub/workers/infrahub_async.py (1)
220-244: Fail fast when git config commands error.Both subprocess calls ignore the return code and log success unconditionally. If
gitis missing or the command fails, the worker quietly proceeds with incorrect identity configuration. This issue has already been flagged in a previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/infrahub/workers/infrahub_async.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T13:20:02.870Z
Learnt from: fatih-acar
Repo: opsmill/infrahub PR: 7227
File: backend/infrahub/worker.py:14-14
Timestamp: 2025-09-17T13:20:02.870Z
Learning: Task workers in the infrahub codebase do not use multiprocess mode, so Prometheus metrics like INFO_METRIC don't require MultiProcessCollector configuration.
Applied to files:
backend/infrahub/workers/infrahub_async.py
🧬 Code graph analysis (1)
backend/infrahub/workers/infrahub_async.py (1)
backend/infrahub/config.py (1)
git(896-897)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-validate-generated
- GitHub Check: backend-tests-functional
- GitHub Check: validate-generated-documentation
- GitHub Check: documentation
- GitHub Check: json-schema
- GitHub Check: backend-tests-integration
- GitHub Check: backend-tests-unit
- GitHub Check: backend-docker-integration
- GitHub Check: frontend-tests
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
backend/infrahub/workers/infrahub_async.py (3)
1-1: LGTM!The imports are appropriate for the subprocess execution and environment variable parsing functionality.
Also applies to: 17-17
47-47: Verify the production mode default is intentional.The
INFRAHUB_PRODUCTIONflag defaults toTruewhen the environment variable is not set. This means the worker will operate in production mode by default, which could be unexpected. Confirm this is the intended behavior, or consider defaulting toFalsefor safer non-production defaults.
130-130: LGTM!The placement of
set_git_global_config()is appropriate, ensuring Git configuration is established before service initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/infrahub/workers/infrahub_async.py (1)
220-244: Fail fast when git config commands error.Both subprocess calls ignore the return code and log success unconditionally. If
gitis missing or the command fails, the worker quietly proceeds with potentially broken git configuration. The stdout and stderr pipes are never consumed, which could also cause issues.Capture stderr, check
returncode, and raise an exception so setup aborts when configuration cannot be applied.Apply the error handling pattern:
if config.SETTINGS.git.user_name: proc_name = await asyncio.create_subprocess_exec( "git", "config", "--global", "user.name", config.SETTINGS.git.user_name, - stdout=asyncio.subprocess.PIPE, + stdout=asyncio.subprocess.DEVNULL, stderr=asyncio.subprocess.PIPE, ) - await proc_name.wait() - self._logger.info("Git user name set") + _, stderr_name = await proc_name.communicate() + if proc_name.returncode != 0: + error_msg = stderr_name.decode("utf-8", errors="ignore").strip() or "unknown error" + self._logger.error("Failed to set git user.name: %s", error_msg) + raise RuntimeError("Unable to configure git user.name") + self._logger.info("Git user name set") if config.SETTINGS.git.user_email: proc_email = await asyncio.create_subprocess_exec( "git", "config", "--global", "user.email", config.SETTINGS.git.user_email, - stdout=asyncio.subprocess.PIPE, + stdout=asyncio.subprocess.DEVNULL, stderr=asyncio.subprocess.PIPE, ) - await proc_email.wait() - self._logger.info("Git user email set") + _, stderr_email = await proc_email.communicate() + if proc_email.returncode != 0: + error_msg = stderr_email.decode("utf-8", errors="ignore").strip() or "unknown error" + self._logger.error("Failed to set git user.email: %s", error_msg) + raise RuntimeError("Unable to configure git user.email") + self._logger.info("Git user email set")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/infrahub/workers/infrahub_async.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T13:20:02.870Z
Learnt from: fatih-acar
Repo: opsmill/infrahub PR: 7227
File: backend/infrahub/worker.py:14-14
Timestamp: 2025-09-17T13:20:02.870Z
Learning: Task workers in the infrahub codebase do not use multiprocess mode, so Prometheus metrics like INFO_METRIC don't require MultiProcessCollector configuration.
Applied to files:
backend/infrahub/workers/infrahub_async.py
🧬 Code graph analysis (1)
backend/infrahub/workers/infrahub_async.py (1)
backend/infrahub/config.py (1)
git(896-897)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: E2E-testing-version-upgrade / From 1.3.0
- GitHub Check: backend-benchmark
- GitHub Check: frontend-tests
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-tests-unit
- GitHub Check: backend-docker-integration
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-tests-functional
- GitHub Check: documentation
- GitHub Check: backend-tests-integration
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
backend/infrahub/workers/infrahub_async.py (2)
130-130: LGTM!The placement of
set_git_global_config()during setup is appropriate, as it configures git before service initialization.
214-218: LGTM!The directory creation logic correctly creates the target directory and appropriately guards the setup with production and environment variable checks.
| DEFAULT_TASK_LOGGERS = ["infrahub.tasks"] | ||
| DEFAULT_GIT_GLOBAL_CONFIG_DIRECTORY = "/opt/infrahub" | ||
| DEFAULT_GIT_GLOBAL_CONFIG_FILE = f"{DEFAULT_GIT_GLOBAL_CONFIG_DIRECTORY}/git-config" | ||
| INFRAHUB_PRODUCTION = TypeAdapter(bool).validate_python(os.environ.get("INFRAHUB_PRODUCTION", True)) |
There was a problem hiding this comment.
Reconsider the default value for production mode.
The INFRAHUB_PRODUCTION constant defaults to True when the environment variable is not set. This means production mode is active by default, which could lead to unexpected behavior in development and testing environments where production-specific logic (like setting global git config in /opt/infrahub) should not run.
Consider defaulting to False for safety:
-INFRAHUB_PRODUCTION = TypeAdapter(bool).validate_python(os.environ.get("INFRAHUB_PRODUCTION", True))
+INFRAHUB_PRODUCTION = TypeAdapter(bool).validate_python(os.environ.get("INFRAHUB_PRODUCTION", False))📝 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.
| INFRAHUB_PRODUCTION = TypeAdapter(bool).validate_python(os.environ.get("INFRAHUB_PRODUCTION", True)) | |
| INFRAHUB_PRODUCTION = TypeAdapter(bool).validate_python(os.environ.get("INFRAHUB_PRODUCTION", False)) |
🤖 Prompt for AI Agents
In backend/infrahub/workers/infrahub_async.py around line 47,
INFRAHUB_PRODUCTION currently defaults to True when the env var is absent which
enables production-only behavior by default; change the default to False and
ensure the environment value is parsed as a boolean (e.g., pass a False default
to os.environ.get or normalize common truthy strings before validating) so
development/testing won’t accidentally run production logic.
| ), | ||
| ) | ||
| user_name: str | None = Field(default=None, description="User name of the git user") | ||
| user_email: EmailStr | None = Field(default=None, description="Email of the git user") |
There was a problem hiding this comment.
I think for the description we should clarify where it will be used as this is something that shows up in the documentation. I.e. we should state that the username and email provided here will be used when Infrahub commits code to a repository.
| WORKER_DEFAULT_RESULT_STORAGE_BLOCK = f"redisstoragecontainer/{TASK_RESULT_STORAGE_NAME}" | ||
| DEFAULT_TASK_LOGGERS = ["infrahub.tasks"] | ||
| DEFAULT_GIT_GLOBAL_CONFIG_DIRECTORY = "/opt/infrahub" | ||
| DEFAULT_GIT_GLOBAL_CONFIG_FILE = f"{DEFAULT_GIT_GLOBAL_CONFIG_DIRECTORY}/git-config" |
There was a problem hiding this comment.
I'd suggest that we also move this to the config object instead of having it as a hardcoded constant.
|
|
||
| async def set_git_global_config(self) -> None: | ||
| if INFRAHUB_PRODUCTION and not os.getenv("GIT_GLOBAL_CONFIG"): | ||
| os.makedirs(DEFAULT_GIT_GLOBAL_CONFIG_DIRECTORY, exist_ok=True) # noqa: PTH103 |
There was a problem hiding this comment.
Instead of ignoring the error here we should use Pathlib as suggested by the rule: https://docs.astral.sh/ruff/rules/os-makedirs/
| self.service = service | ||
|
|
||
| async def set_git_global_config(self) -> None: | ||
| if INFRAHUB_PRODUCTION and not os.getenv("GIT_GLOBAL_CONFIG"): |
There was a problem hiding this comment.
As we discussed in Slack I think we should remove the INFRAHUB_PRODUCTION variable from here.
| user_email = "[email protected]" | ||
|
|
||
| await set_git_config("user.name", user_name) | ||
| await set_git_config("user.email", user_email) |
There was a problem hiding this comment.
As CodeRabbit indicates I'm wondering if this is correct, i.e. the idea would be that the test test_worker_has_set_git_user_config() would validate that the worker configures these values but here we have already added them in the fixture. Is this a leftover from a previous iteration?
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) | ||
| await proc_email.wait() |
There was a problem hiding this comment.
For these two commands we assume that everything will work fine. We should consider errors as well, what would it look like if any of these calls failed? Would the app crash or would the error be silently ignored?
This PR creates new git configurations that are then set on the repository. The created configurations are
user_name- The user name of the git useruser_email- The email of the git userSummary by CodeRabbit
New Features
Documentation
Tests
Chores