IFC-1361: Merge core read only repositories on branch merge #7459
Conversation
WalkthroughThe pull request adds explicit handling for read-only repositories during branch merges. BranchMerger gains 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 as they are similar to previous 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). (10)
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
group/backend
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/infrahub/git/tasks.py (1)
479-479: Consider a more explicit condition for read-only repository detection.The condition
elif not model.default_branch:implicitly distinguishes read-only repositories. This could be clearer by checking the repository kind directly or adding a comment explaining the logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/infrahub/core/merge.py(2 hunks)backend/infrahub/git/models.py(1 hunks)backend/infrahub/git/tasks.py(3 hunks)backend/tests/unit/core/diff/test_coordinator_lock.py(2 hunks)changelog/5978.added.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Lint Markdown/MDX files with markdownlint using
.markdownlint.yaml
Files:
changelog/5978.added.md
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/git/models.pybackend/infrahub/git/tasks.pybackend/tests/unit/core/diff/test_coordinator_lock.pybackend/infrahub/core/merge.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/infrahub/git/models.pybackend/infrahub/git/tasks.pybackend/tests/unit/core/diff/test_coordinator_lock.pybackend/infrahub/core/merge.py
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/core/diff/test_coordinator_lock.py
🧬 Code graph analysis (4)
backend/infrahub/git/models.py (1)
backend/infrahub/git/base.py (1)
default_branch(175-176)
backend/infrahub/git/tasks.py (1)
backend/infrahub/message_bus/types.py (1)
kind(72-75)
backend/tests/unit/core/diff/test_coordinator_lock.py (3)
backend/infrahub/core/schema/__init__.py (1)
SchemaRoot(47-99)backend/infrahub/core/schema/manager.py (1)
register_schema(202-209)backend/infrahub/core/branch/models.py (1)
update_schema_hash(125-133)
backend/infrahub/core/merge.py (4)
backend/infrahub/core/protocols.py (2)
CoreReadOnlyRepository(495-497)CoreRepository(500-502)backend/infrahub/core/manager.py (4)
NodeManager(135-1363)query(138-155)query(159-176)query(179-273)backend/infrahub/git/models.py (1)
GitRepositoryMerge(86-95)backend/infrahub/services/adapters/workflow/local.py (1)
submit_workflow(33-41)
🪛 LanguageTool
changelog/5978.added.md
[grammar] ~1-~1: Use a hyphen to join words.
Context: Merge core read only repositories on branch merge
(QB_NEW_EN_HYPHEN)
⏰ 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-invoke-demo-start
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-playwright
- 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: backend-tests-unit
🔇 Additional comments (9)
backend/infrahub/git/tasks.py (2)
455-455: LGTM - Logger initialization.Adding logger initialization improves observability for the merge workflow.
465-477: LGTM - Enhanced logging for STAGING merge path.The added logging statements provide clear visibility into the merge workflow progress.
backend/tests/unit/core/diff/test_coordinator_lock.py (2)
19-19: LGTM - Import for read-only repository schema.The import is correctly added to support registering read-only repository schemas in tests.
58-65: LGTM - Test setup extended for read-only repositories.The test fixture now registers both regular and read-only repository schemas, mirroring the production behavior introduced in this PR. The duplication is intentional to create two separate schema registrations.
backend/infrahub/core/merge.py (3)
9-9: LGTM - Import updated for read-only repositories.The import correctly includes
CoreReadOnlyRepositoryto support the new merge workflow.
226-227: LGTM - Repository merge logic separated.The refactoring separates read-only and regular repository merges, improving code clarity and maintainability.
249-273: LGTM - Regular repository merge logic preserved.The existing merge logic for regular repositories is correctly preserved with appropriate status checks and conditional merge criteria.
backend/infrahub/git/models.py (2)
91-91: Optional internal_status is properly handled—no changes needed.All code paths correctly handle
Nonevalues:
- Equality comparisons (
if model.internal_status == RepositoryInternalStatus.ACTIVE.value) safely returnFalsewhenNone, preventing unwanted execution pathsInfrahubRepository.new()uses a default value ("active") for the field, preventing runtime errors- No attribute access or method calls are performed on
internal_statuswithout prior validation
95-95: No issues found - code safely handles optionaldefault_branchfield.The field
default_branch_name: str | NoneinInfrahubRepositoryBase(backend/infrahub/git/base.py:143) is properly handled through a property at line 175-176 that returnsself.default_branch_name or registry.default_branch. Even whenmodel.default_branchisNoneand passed toInfrahubRepository.init()asdefault_branch_name, accessingrepo.default_branchsafely returns a fallback value, ensuring all code paths receive a valid string.
| async def merge_core_read_only_repositories(self) -> None: | ||
| repos_in_main_list = await NodeManager.query(schema=CoreReadOnlyRepository, db=self.db) | ||
| repos_in_main = {repo.id: repo for repo in repos_in_main_list} | ||
|
|
||
| repos_in_branch_list = await NodeManager.query( | ||
| schema=CoreReadOnlyRepository, db=self.db, branch=self.source_branch | ||
| ) | ||
| for repo in repos_in_branch_list: | ||
| if repo.id not in repos_in_main: | ||
| continue | ||
|
|
||
| model = GitRepositoryMerge( | ||
| repository_id=repo.id, | ||
| repository_name=repo.name.value, | ||
| source_branch=self.source_branch.name, | ||
| destination_branch=self.destination_branch.name, | ||
| destination_branch_id=str(self.destination_branch.get_uuid()), | ||
| ) | ||
| await self.workflow.submit_workflow(workflow=GIT_REPOSITORIES_MERGE, parameters={"model": model}) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify whether read-only repositories should have status checks.
The merge_core_read_only_repositories method merges all read-only repositories found in both branches without checking internal_status (like the INACTIVE check at line 260 in merge_core_repositories) or conditional merge criteria (like sync_with_git at line 263). This could potentially merge repositories that shouldn't be merged.
Run this script to check if read-only repositories have an internal_status field:
If read-only repositories do have an internal_status field, consider adding similar checks as in merge_core_repositories.
🏁 Script executed:
#!/bin/bash
# Check the CoreReadOnlyRepository protocol definition
ast-grep --pattern $'class CoreReadOnlyRepository($$$) {
$$$
}'Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for CoreReadOnlyRepository definition/import
rg -n "CoreReadOnlyRepository" --type=py -B2 -A5 | head -50
Length of output: 4349
🏁 Script executed:
#!/bin/bash
# Look at the merge_core_repositories method to understand the pattern at lines 260 and 263
rg -n "def merge_core_repositories" --type=py -A35 backend/infrahub/core/merge.pyLength of output: 1551
🏁 Script executed:
#!/bin/bash
# Search for internal_status field usage in the codebase
rg -n "internal_status" --type=py -B2 -A2 | head -60Length of output: 4149
🏁 Script executed:
#!/bin/bash
# Search for sync_with_git field usage
rg -n "sync_with_git" --type=py -B2 -A2 | head -60Length of output: 4110
---
🏁 Script executed:
```shell
#!/bin/bash
# Find CoreReadOnlyRepository protocol definition
fd "protocols" --type=d backend/infrahub/core/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for the protocol definition more broadly
rg -n "CoreReadOnlyRepository" --type=py -B3 -A20 backend/infrahub/core/protocols.pyLength of output: 668
🏁 Script executed:
#!/bin/bash
# Check the entire merge_core_read_only_repositories context in the merge.py file
rg -n "def merge_core_read_only_repositories" --type=py -B5 -A30 backend/infrahub/core/merge.pyLength of output: 1846
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or context about read_only repository merging
rg -n "read.only|read_only" --type=py -B3 -A3 backend/infrahub/core/merge.pyLength of output: 479
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for CoreGenericRepository definition to see if it has internal_status
rg -n "class CoreGenericRepository" --type=py -A30 backend/infrahub/core/protocols.py
Length of output: 908
🏁 Script executed:
#!/bin/bash
# Search for internal_status definition in protocols
rg -n "internal_status" --type=py -B2 -A2 backend/infrahub/core/protocols.pyLength of output: 161
🏁 Script executed:
#!/bin/bash
# Check git history or comments around merge_core_read_only_repositories
git log --oneline -n 20 --all -- backend/infrahub/core/merge.py | head -20Length of output: 1082
🏁 Script executed:
#!/bin/bash
# Look for TODO or FIXME comments in merge.py related to read-only repos
rg -n "TODO|FIXME" --type=py -B2 -A2 backend/infrahub/core/merge.pyLength of output: 276
Add status and sync checks to merge_core_read_only_repositories to match regular repository merge logic.
Read-only repositories inherit internal_status from CoreGenericRepository (line 119 in protocols.py). The method currently merges all read-only repos found in both branches without checking:
- Status check (like line 260 in
merge_core_repositories): Skip INACTIVE repositories - Conditional merge (like line 263): Only merge if
sync_with_gitis enabled ORinternal_statusis STAGING
Apply the same checks to merge_core_read_only_repositories and pass internal_status to the GitRepositoryMerge model for consistency.
🤖 Prompt for AI Agents
In backend/infrahub/core/merge.py around lines 229 to 247, the
merge_core_read_only_repositories function currently enqueues merges for every
read-only repo present in both branches; update it to mirror
merge_core_repositories by (1) skipping repositories whose internal_status is
INACTIVE, (2) only enqueuing a merge when repo.sync_with_git is True OR
repo.internal_status is STAGING, and (3) include the repository's
internal_status when constructing the GitRepositoryMerge model so the workflow
receives that status; implement these checks before creating the model and
submitting the workflow.
| @@ -0,0 +1 @@ | |||
| Merge core read only repositories on branch merge | |||
There was a problem hiding this comment.
Use hyphenation for compound adjective.
The phrase "read only" should be hyphenated as "read-only" when used as a compound adjective.
Apply this diff:
-Merge core read only repositories on branch merge
+Merge core read-only repositories on branch merge📝 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.
| Merge core read only repositories on branch merge | |
| Merge core read-only repositories on branch merge |
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Use a hyphen to join words.
Context: Merge core read only repositories on branch merge
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In changelog/5978.added.md around lines 1 to 1 the phrase "read only" is used as
a compound adjective; update it to "read-only" so the line reads "Merge core
read-only repositories on branch merge".
CodSpeed Performance ReportMerging #7459 will not alter performanceComparing Summary
|
|
I wonder if an issue with this is that the repositories are branch agnostic nodes but for the read-only repositories the commit is a branch aware attribute. So perhaps the issue here is actually that we don't merge such an attribute. Can can talk about this in the backend meeting tomorrow. |
Fixes #5978
This PR adds functionality for merging modified read only repositories from other branches into main branch. Existing implementation takes care of merging modified core repositories.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation