Fix "Import Error" sync status with no explanation in the task logs #7736
Conversation
github-actions
bot
added
the
group/backend
WalkthroughA new public exception class Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
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 |
petercrocker
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
backend/tests/unit/git/test_repository_config.py (2)
94-106: Consider verifying the documentation URL is present.The
get_repository_configmethod includes a documentation link in the error message. Consider adding an assertion to verify this URL is present in the error.assert repo_without_config.name in str(exc_info.value) assert "missing a configuration file" in str(exc_info.value) assert ".infrahub.yml" in str(exc_info.value) or ".infrahub.yaml" in str(exc_info.value) + assert "docs.infrahub.app" in str(exc_info.value)
134-159: Fixture placement could be improved for readability.The
repo_with_valid_configfixture is defined after tests that use other fixtures. While this works, placing all fixtures together at the top of the class would improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/infrahub/exceptions.py(1 hunks)backend/infrahub/git/integrator.py(5 hunks)backend/tests/unit/git/test_git_read_only_repository.py(1 hunks)backend/tests/unit/git/test_git_repository.py(3 hunks)backend/tests/unit/git/test_repository_config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Poetry for dependency management in backend Python projects
Files:
backend/tests/unit/git/test_git_repository.pybackend/tests/unit/git/test_repository_config.pybackend/tests/unit/git/test_git_read_only_repository.pybackend/infrahub/exceptions.pybackend/infrahub/git/integrator.py
backend/tests/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
backend/tests/**/*.py: Run backend tests with pytest or via invoke tasks
Place backend tests in backend/tests/ directory
Files:
backend/tests/unit/git/test_git_repository.pybackend/tests/unit/git/test_repository_config.pybackend/tests/unit/git/test_git_read_only_repository.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Always use triple quotes (""") for Python docstrings
Follow Google-style docstring format for Python docstrings
Include brief one-line description in Python docstrings when applicable
Include detailed description in Python docstrings when applicable
Include Args/Parameters section without typing in Python docstrings when applicable
Include Returns section in Python docstrings when applicable
Include Raises section in Python docstrings when applicable
Include Examples section in Python docstrings when applicable
**/*.py: Use type hints for all function parameters and return values in Python
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
Use ruff and mypy for type checking and validations in PythonUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all function parameters and return values in Python code
Use async/await whenever possible; useasync deffor asynchronous functions andawaitfor asynchronous calls
Use Pydantic models for dataclasses instead of plain Python dataclasses
Always use triple quotes (""") for docstrings and follow Google-style docstring format with sections: brief description, detailed description (if needed), Args/Parameters, Returns, Raises, and Examples when applicable
Use ruff and mypy for type checking and validations; runpoetry run invoke formatto format all Python files andpoetry run invoke lintto validate formatting
Files:
backend/tests/unit/git/test_git_repository.pybackend/tests/unit/git/test_repository_config.pybackend/tests/unit/git/test_git_read_only_repository.pybackend/infrahub/exceptions.pybackend/infrahub/git/integrator.py
🧬 Code graph analysis (3)
backend/tests/unit/git/test_git_repository.py (1)
backend/infrahub/git/repository.py (1)
sync(65-123)
backend/tests/unit/git/test_git_read_only_repository.py (1)
backend/infrahub/git/repository.py (1)
sync_from_remote(247-255)
backend/infrahub/git/integrator.py (1)
backend/infrahub/exceptions.py (3)
RepositoryConfigurationError(119-133)RepositoryInvalidFileSystemError(105-116)TransformError(177-185)
🪛 GitHub Actions: CI
backend/tests/unit/git/test_git_repository.py
[error] 1-1: Ruff formatting check failed: 2 files would be reformatted. Run 'ruff format --write --exclude python_sdk .' to fix code style issues.
backend/tests/unit/git/test_git_read_only_repository.py
[error] 1-1: Ruff formatting check failed: 2 files would be reformatted. Run 'ruff format --write --exclude python_sdk .' to fix code style issues.
⏰ 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
🔇 Additional comments (6)
backend/infrahub/exceptions.py (1)
119-133: LGTM!The new
RepositoryConfigurationErrorexception class follows the established patterns in this file, with proper inheritance fromRepositoryError, a clear docstring, and a user-friendly default message that includes actionable guidance.backend/infrahub/git/integrator.py (3)
439-452: Well-documented method signature change.The updated docstring properly documents the new behavior with Args, Returns, and Raises sections following Google-style format. The return type change from
InfrahubRepositoryConfig | NonetoInfrahubRepositoryConfigis a breaking change but is appropriate given that the method now raises exceptions for all error cases.
467-476: Clear, actionable error message for missing config file.The error message includes the repository name, describes the problem, specifies acceptable file names, and provides a documentation link. This directly addresses the PR objective of providing user-friendly error messages.
482-487: Proper exception chaining for YAML parse errors.The
from excclause preserves the original exception context, which helps with debugging.backend/tests/unit/git/test_repository_config.py (2)
1-17: LGTM - Well-structured test module.Good module organization with proper imports, docstring, and TYPE_CHECKING block for type hints.
173-199: Good unit tests for the exception class.These tests verify the exception's behavior independently of the repository logic, covering default message, custom message, and inheritance. Good test isolation.
| # Mock import_objects_from_files since we're testing git sync, not import functionality | ||
| with patch( | ||
| "infrahub.git.repository.InfrahubRepository.import_objects_from_files", new_callable=AsyncMock | ||
| ): | ||
| await repo.sync() |
There was a problem hiding this comment.
Same formatting fix needed here.
Apply consistent formatting to pass ruff check.
Apply this diff:
# Mock import_objects_from_files since we're testing git sync, not import functionality
- with patch(
- "infrahub.git.repository.InfrahubRepository.import_objects_from_files", new_callable=AsyncMock
- ):
- await repo.sync()
+ with patch("infrahub.git.repository.InfrahubRepository.import_objects_from_files", new_callable=AsyncMock):
+ await repo.sync()📝 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.
| # Mock import_objects_from_files since we're testing git sync, not import functionality | |
| with patch( | |
| "infrahub.git.repository.InfrahubRepository.import_objects_from_files", new_callable=AsyncMock | |
| ): | |
| await repo.sync() | |
| # Mock import_objects_from_files since we're testing git sync, not import functionality | |
| with patch("infrahub.git.repository.InfrahubRepository.import_objects_from_files", new_callable=AsyncMock): | |
| await repo.sync() |
🤖 Prompt for AI Agents
In backend/tests/unit/git/test_git_repository.py around lines 530 to 534, the
multi-line with patch(...) call needs consistent formatting to satisfy ruff:
rewrite the with statement so the call to patch is parenthesized across lines
with each argument on its own line, include a trailing comma after the last
argument, place the closing parenthesis on its own line, and keep the with-block
body (await repo.sync()) indented as before.
CodSpeed Performance ReportMerging #7736 will not alter performanceComparing Summary
|
Since get_repository_config() no longer returns None, the type ignore comments are no longer needed.
Since get_repository_config() no longer returns None, the type ignore comments are no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/infrahub/git/integrator.py (1)
1-485: Fix ruff formatting failure before mergingCI reports that
ruff formatwould reformat one file. Please run the project’s standard formatting command (per guidelines/logs, e.g.poetry run invoke formatorruff format --exclude python_sdk .) and commit the result so the lint step passes.
🧹 Nitpick comments (1)
backend/infrahub/git/integrator.py (1)
439-452: get_repository_config behavior and messaging look good; consider adding branch/commit context to logsThe updated
get_repository_configdoes a nice job of:
- Enforcing presence of
.infrahub.yml/.infrahub.yamland failing fast withRepositoryConfigurationError.- Differentiating missing file, YAML parse errors, and invalid schema/format via tailored messages (including the docs URL).
- Returning a non‑optional
InfrahubRepositoryConfig, which simplifies downstream code.One optional improvement: for the
log.errorcalls on missing/invalid config and YAML errors, consider includingbranch_nameand/orcommitto make debugging multi-branch scenarios easier.For example:
- log.error( - f"Repository '{self.name}' is missing a configuration file. " - "Expected '.infrahub.yml' or '.infrahub.yaml' in the repository root." - ) + log.error( + "Repository '%s' is missing a configuration file at commit '%s'. " + "Expected '.infrahub.yml' or '.infrahub.yaml' in the repository root.", + self.name, + commit, + )Same idea could apply to the YAML parse and invalid-format branches.
Also applies to: 467-476, 480-487, 497-502
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/infrahub/git/integrator.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Poetry for dependency management in backend Python projects
Files:
backend/infrahub/git/integrator.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Always use triple quotes (""") for Python docstrings
Follow Google-style docstring format for Python docstrings
Include brief one-line description in Python docstrings when applicable
Include detailed description in Python docstrings when applicable
Include Args/Parameters section without typing in Python docstrings when applicable
Include Returns section in Python docstrings when applicable
Include Raises section in Python docstrings when applicable
Include Examples section in Python docstrings when applicable
**/*.py: Use type hints for all function parameters and return values in Python
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
Use ruff and mypy for type checking and validations in PythonUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all function parameters and return values in Python code
Use async/await whenever possible; useasync deffor asynchronous functions andawaitfor asynchronous calls
Use Pydantic models for dataclasses instead of plain Python dataclasses
Always use triple quotes (""") for docstrings and follow Google-style docstring format with sections: brief description, detailed description (if needed), Args/Parameters, Returns, Raises, and Examples when applicable
Use ruff and mypy for type checking and validations; runpoetry run invoke formatto format all Python files andpoetry run invoke lintto validate formatting
Files:
backend/infrahub/git/integrator.py
🧬 Code graph analysis (1)
backend/infrahub/git/integrator.py (1)
backend/infrahub/exceptions.py (3)
RepositoryConfigurationError(119-133)RepositoryInvalidFileSystemError(105-116)TransformError(177-185)
🪛 GitHub Actions: CI
backend/infrahub/git/integrator.py
[error] 199-199: ruff format check failed. 1 file would be reformatted. Run 'ruff format --exclude python_sdk .' to fix formatting.
⏰ 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
🔇 Additional comments (2)
backend/infrahub/git/integrator.py (2)
50-55: New RepositoryConfigurationError import is consistent and correctImporting
RepositoryConfigurationErroralongside the other repository-related errors is appropriate and matches how it’s used later inget_repository_config.
176-220: Sync flow now correctly treats any config/import failure as an import errorThe refactored
import_objects_from_filesflow looks solid: you eagerly load the repository config and then pass the non-optionalconfig_filethrough all import steps, and any exception (includingRepositoryConfigurationError) now marks the sync asERROR_IMPORTbefore re-raising. This matches the goal of surfacing meaningful failures instead of silentNone/no-op behavior.Please double-check via an end‑to‑end run that:
- A repo missing
.infrahub.yml/.infrahub.yamlends up withERROR_IMPORTsync status, and- The raised
RepositoryConfigurationErrormessage is actually presented to users in the UI/task logs.
…nfrahub into pmc-20251127-infrahubyml-error
The type ignore comments are needed because mypy doesn't understand Prefect's @task decorator return type correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/infrahub/git/integrator.py (1)
188-208: Config is now required for imports; consider cleaning up type-ignoresThe new flow that:
- sets
sync_status = RepositorySyncStatus.IN_SYNCby default,- always loads the repository configuration once via
get_repository_config, and- runs all
import_*steps with the sameconfig_filewhile mapping any exception toERROR_IMPORTis a clear improvement and matches the “fail loudly on bad/missing config” requirement.
However, the repeated
# type: ignore[misc]/# type: ignore[misc, call-overload]on these calls look redundant given the signatures ofget_repository_configand theimport_*methods now agree onInfrahubRepositoryConfig. Unless mypy is still complaining in your environment, consider dropping these ignores so the type checker can surface real issues.
🧹 Nitpick comments (1)
backend/infrahub/git/integrator.py (1)
437-500: Repository config loading and error messages look solid; minor consistency tweakThe new
get_repository_configtask:
- Picks
.infrahub.ymlover.infrahub.yamland errors cleanly when neither exists.- Distinguishes YAML parse failures from schema/format validation failures.
- Raises
RepositoryConfigurationErrorwith repository name and a descriptive message, and logs aterrorlevel.- Uses a clear Google-style docstring with typed parameters and return type.
Functionally this achieves the PR’s goal of surfacing actionable configuration problems.
One small polish suggestion: for consistency with the missing-config case (which links to the docs), you might want to append the same documentation URL to the messages for the YAML-parse and invalid-format branches as well, so every
RepositoryConfigurationErrorpoints users to the same remediation docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/infrahub/git/integrator.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Poetry for dependency management in backend Python projects
Files:
backend/infrahub/git/integrator.py
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Always use triple quotes (""") for Python docstrings
Follow Google-style docstring format for Python docstrings
Include brief one-line description in Python docstrings when applicable
Include detailed description in Python docstrings when applicable
Include Args/Parameters section without typing in Python docstrings when applicable
Include Returns section in Python docstrings when applicable
Include Raises section in Python docstrings when applicable
Include Examples section in Python docstrings when applicable
**/*.py: Use type hints for all function parameters and return values in Python
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
Use ruff and mypy for type checking and validations in PythonUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all function parameters and return values in Python code
Use async/await whenever possible; useasync deffor asynchronous functions andawaitfor asynchronous calls
Use Pydantic models for dataclasses instead of plain Python dataclasses
Always use triple quotes (""") for docstrings and follow Google-style docstring format with sections: brief description, detailed description (if needed), Args/Parameters, Returns, Raises, and Examples when applicable
Use ruff and mypy for type checking and validations; runpoetry run invoke formatto format all Python files andpoetry run invoke lintto validate formatting
Files:
backend/infrahub/git/integrator.py
⏰ 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). (2)
- GitHub Check: validate-generated-documentation
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
backend/infrahub/git/integrator.py (1)
50-55: New exception imports are consistent with usageImporting
RepositoryConfigurationError(alongside the existing repository-related errors) aligns with its use inget_repository_configand keeps exception handling centralized ininfrahub.exceptions. No issues here.
The conflict-01 test fixture was missing a .infrahub.yml configuration file. With the new RepositoryConfigurationError exception that is now raised when a repository lacks this config file, the integration tests using this fixture were failing. This adds a minimal .infrahub.yml to fix the integration tests: - test_run_pipeline_validate_requested_jobs - test_run_generators_validate_requested_jobs
This PR addresses issue #6124 by providing clear, actionable error messages when a Git repository is missing its .infrahub.yml configuration file or when the configuration file is invalid.
Previously, when adding a Git repository without a .infrahub.yml file, users would see an "Import Error" sync status with no explanation in the task logs. The only indication was a debug-level log message that wasn't visible to users, making it difficult to diagnose and fix the issue.
Added a new RepositoryConfigurationError exception class that provides clear, user-friendly error messages
Modified get_repository_config() to raise exceptions instead of silently returning None
Error messages now include the repository name, specific issue description, and a link to documentation
Fixes #6124
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.