IFC-1984: Update git sync to consider new configuration #7486
Conversation
This PR adds `sync_branch_names` parameter to git settings so that users can add branches that they want to be synced with git instead of syncing all branches like we currently do. This parameter can also be a regex so that we can match branch names when syncing
This PR updates the git sync to check the new configuration `sync_branch_names` and if it is present it matches the branch name with the names in the config skips adding it as a remote branch if it does not match.
github-actions
bot
added
the
group/backend
CodSpeed Performance ReportMerging #7486 will not alter performanceComparing Summary
|
WalkthroughA new async method Pre-merge checks and finishing touches✅ Passed checks (3 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). (8)
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 |
solababs
force-pushed
the
sb-20251027-update-git-sync-ifc-1984
branch
from
6394fff to
ecb1481
Compare
solababs
force-pushed
the
sb-20251027-update-git-sync-ifc-1984
branch
from
ecb1481 to
cd7db02
Compare
…o sb-20251027-update-git-sync-ifc-1984
…o sb-20251027-update-git-sync-ifc-1984
…o sb-20251027-update-git-sync-ifc-1984
….com/opsmill/infrahub into sb-20251027-update-git-sync-ifc-1984
| ): | ||
| raise ValidationError( | ||
| f"Branch name '{obj.name}' does not match import_sync_branch_names: {config.SETTINGS.git.import_sync_branch_names}" | ||
| ) |
There was a problem hiding this comment.
I don't think we should restrict the creation of these branches within Infrahub itself. I.e. the setting should only restrict which branches are automatically created due to imports from Git, not control the naming of branches that users can create within Infrahub.
backend/infrahub/core/utils.py
Outdated
| for branch_filter in config.SETTINGS.git.import_sync_branch_names: | ||
| if re.fullmatch(branch_filter, branch_short_name) or branch_filter == branch_short_name: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
I think we want to avoid adding more stuff within the infrahub.core namespace so I'd instead move this to infrahub.git.
One thought would be if we saved the compiled regex entries directly within the config object in a private attribute we could access them all and just do a .match for all entries within the list as we'd already have them compiled.
Also I'd suggest that we combine this function with the logic we have in git/base.py:
if config.SETTINGS.git.import_sync_branch_names and not branch_name_in_sync_branchesCurrently the branch_name_in_sync_branches() function might behave in an unexpected way if used in isolation and config.SETTINGS.git.import_sync_branch_names didn't contain any entries. If we instead had a function that would return True if we should skip the import, if users hadn't configured this option it would always return True, otherwise it would return True if the branch name didn't contain a match for the regex filters.
backend/infrahub/git/base.py
Outdated
| branch_short_name=short_name | ||
| ): | ||
| skipped_branch_names.append(short_name) | ||
| continue |
There was a problem hiding this comment.
Here we must also account for the default branch, any branch in Infrahub marked as "sync with git", and potentially any "default branch" configured on the repo itself (this is typically always "main" but it can differ).
| assert isinstance(remote_branches, dict) | ||
| assert sorted(remote_branches.keys()) == ["branch01", "branch02"] | ||
|
|
||
| config.SETTINGS.git.import_sync_branch_names = [] |
There was a problem hiding this comment.
Instead of changing the config like this the test and resetting it back to another value afterwards I'd suggest that we use a fixture similar to this one:
@pytest.fixture
def query_limit_of_one() -> Generator[None, None, None]:
original_query_size_limit = config.SETTINGS.database.query_size_limit
config.SETTINGS.database.query_size_limit = 1
yield
config.SETTINGS.database.query_size_limit = original_query_size_limit| assert sorted(remote_branches.keys()) == ["branch01", "branch02", "clean-branch", "main"] | ||
|
|
||
|
|
||
| async def test_get_branches_from_remote_in_import_sync_branch_names(git_repo_01: InfrahubRepository): |
There was a problem hiding this comment.
Minor detail here, I just merged a PR to develop that went through and added return values to all of the tests that didn't have them. There's still some work to be done with other functions before it's enforced but please add a ) -> None: as the return value here.
![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 (1)
backend/tests/unit/git/conftest.py (1)
1364-1455: Consider consolidating duplicated mock fixtures.
mock_get_branch_git_repo_01andmock_get_branch_git_repo_03share significant structural duplication. Both define similar GraphQL callbacks with overlapping logic for "main" and "branch01" cases.Consider parameterizing a single fixture or extracting common callback logic to reduce duplication:
def _create_branch_response(branch_name: str, is_default: bool = False, sync_with_git: bool = False): """Helper to create branch response data.""" return { "data": { "Branch": [{ "id": "eca306cf-662e-4e03-8180-2b788b191d3c", "name": branch_name, "sync_with_git": sync_with_git, "is_default": is_default, "origin_branch": None, "branched_from": "2023-02-17T09:30:17.811719Z", "is_isolated": False, "has_schema_changes": False, }] } }Then use this helper in both fixtures to reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/infrahub/git/base.py(5 hunks)backend/infrahub/git/utils.py(3 hunks)backend/tests/unit/git/conftest.py(2 hunks)backend/tests/unit/git/test_git_repository.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/git/utils.pybackend/infrahub/git/base.pybackend/tests/unit/git/conftest.pybackend/tests/unit/git/test_git_repository.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/utils.pybackend/infrahub/git/base.pybackend/tests/unit/git/conftest.pybackend/tests/unit/git/test_git_repository.py
backend/tests/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place backend tests in
backend/tests/
Files:
backend/tests/unit/git/conftest.pybackend/tests/unit/git/test_git_repository.py
🧠 Learnings (1)
📚 Learning: 2025-08-15T08:57:37.880Z
Learnt from: BaptisteGi
Repo: opsmill/infrahub PR: 7044
File: docs/docs/guides/change-approval-workflow.mdx:93-107
Timestamp: 2025-08-15T08:57:37.880Z
Learning: In Infrahub's change approval workflow documentation, the "Restricted Access" role intentionally includes `global:edit_default_branch:allow_all` permission despite being described as allowing edits only to branches other than default. This configuration is expected behavior according to the maintainers.
Applied to files:
backend/infrahub/git/base.py
🧬 Code graph analysis (4)
backend/infrahub/git/utils.py (2)
backend/infrahub/config.py (1)
git(893-894)backend/tests/unit/git/conftest.py (1)
import_sync_branch_names(1357-1361)
backend/infrahub/git/base.py (3)
backend/infrahub/config.py (1)
git(893-894)backend/infrahub/git/utils.py (1)
branch_name_in_sync_branches(175-179)backend/tests/unit/git/conftest.py (1)
import_sync_branch_names(1357-1361)
backend/tests/unit/git/conftest.py (2)
backend/infrahub/config.py (1)
git(893-894)backend/tests/conftest.py (1)
branch(1256-1260)
backend/tests/unit/git/test_git_repository.py (2)
backend/tests/unit/git/conftest.py (5)
git_repo_01_w_client(169-173)mock_get_branch_git_repo_01(1365-1415)import_sync_branch_names(1357-1361)mock_get_branch_git_repo_03(1419-1455)git_repo_01(135-147)backend/infrahub/git/base.py (1)
get_filtered_remote_branches(777-779)
⏰ 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). (5)
- GitHub Check: validate-generated-documentation
- GitHub Check: graphql-schema
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-integration
- GitHub Check: backend-tests-unit
🔇 Additional comments (2)
backend/tests/unit/git/conftest.py (1)
1356-1361: LGTM! Fixture pattern for config override.The fixture correctly saves, overrides, and restores
config.SETTINGS.git.import_sync_branch_names, following the recommended pattern from past reviews.backend/tests/unit/git/test_git_repository.py (1)
1087-1106: LGTM! Comprehensive test coverage.The three test functions provide good coverage of the filtering logic:
- When all relevant branches match the sync filter
- When only some branches are found in GraphQL
- When no import_sync_branch_names are configured (all branches returned)
The tests correctly include return type annotations as requested in past reviews.
backend/infrahub/git/base.py
Outdated
| try: | ||
| branch = await self.client.branch.get(branch_name=short_name) | ||
| except BranchNotFoundError: | ||
| ... |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use self.sdk instead of self.client and clarify exception handling.
Line 501 accesses self.client which may be None. The ellipsis (...) as an exception handler on line 503 is valid but unconventional.
try:
- branch = await self.client.branch.get(branch_name=short_name)
+ branch = await self.sdk.branch.get(branch_name=short_name)
except BranchNotFoundError:
- ...
+ pass # Branch doesn't exist in Infrahub, will check sync filters belowThe self.sdk property automatically initializes the client if needed (line 180-184), making it safer than direct self.client access.
📝 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.
| try: | |
| branch = await self.client.branch.get(branch_name=short_name) | |
| except BranchNotFoundError: | |
| ... | |
| try: | |
| branch = await self.sdk.branch.get(branch_name=short_name) | |
| except BranchNotFoundError: | |
| pass # Branch doesn't exist in Infrahub, will check sync filters below |
🤖 Prompt for AI Agents
In backend/infrahub/git/base.py around lines 500-503, replace direct use of
self.client with self.sdk (which lazily initializes the client) and change the
bare ellipsis in the BranchNotFoundError handler to an explicit action: await
branch = await self.sdk.branch.get(branch_name=short_name) and in the except
BranchNotFoundError block either return None (or raise a clearer custom
exception) and/or log the missing-branch condition with context so the behavior
is explicit and not silently ignored.
backend/infrahub/git/base.py
Outdated
| async def get_filtered_remote_branches(self) -> dict[str, BranchInRemote]: | ||
| remote_branches = self.get_branches_from_remote() | ||
| return await self.filter_remote_branches(branches=remote_branches) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add docstring to public method.
The get_filtered_remote_branches method is missing a docstring as required by coding guidelines.
As per coding guidelines:
async def get_filtered_remote_branches(self) -> dict[str, BranchInRemote]:
+ """Retrieve remote branches and apply sync filtering.
+
+ Returns:
+ Dictionary of filtered remote branches that should be synced.
+ """
remote_branches = self.get_branches_from_remote()
return await self.filter_remote_branches(branches=remote_branches)📝 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 get_filtered_remote_branches(self) -> dict[str, BranchInRemote]: | |
| remote_branches = self.get_branches_from_remote() | |
| return await self.filter_remote_branches(branches=remote_branches) | |
| async def get_filtered_remote_branches(self) -> dict[str, BranchInRemote]: | |
| """Retrieve remote branches and apply sync filtering. | |
| Returns: | |
| Dictionary of filtered remote branches that should be synced. | |
| """ | |
| remote_branches = self.get_branches_from_remote() | |
| return await self.filter_remote_branches(branches=remote_branches) |
🤖 Prompt for AI Agents
In backend/infrahub/git/base.py around lines 777 to 779, add a concise docstring
to the public async method get_filtered_remote_branches describing its purpose,
parameters (none), return type (dict[str, BranchInRemote]) and behavior (calls
get_branches_from_remote then filters via filter_remote_branches), following
project docstring style (one-line summary plus short elaboration if needed) and
include any raised exceptions or side effects if applicable.
backend/infrahub/git/utils.py
Outdated
| def branch_name_in_sync_branches(branch_short_name: str) -> bool: | ||
| for branch_filter in config.SETTINGS.git.import_sync_branch_names: | ||
| if re.fullmatch(branch_filter, branch_short_name) or branch_filter == branch_short_name: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add docstring and clarify matching logic.
The function is missing a docstring, which is required by the coding guidelines. Additionally, the dual matching logic (regex + exact match) could be simplified or explained.
As per coding guidelines, add a Google-style docstring:
def branch_name_in_sync_branches(branch_short_name: str) -> bool:
+ """Check if a branch name matches any configured sync filters.
+
+ Filters can be exact strings or regular expressions. The function attempts
+ both regex matching (via re.fullmatch) and exact string matching.
+
+ Args:
+ branch_short_name: The short name of the branch to check.
+
+ Returns:
+ True if the branch name matches any filter, False otherwise.
+ """
for branch_filter in config.SETTINGS.git.import_sync_branch_names:
if re.fullmatch(branch_filter, branch_short_name) or branch_filter == branch_short_name:
return True
return FalseNote: The exact match check is redundant when branch_filter is a plain string (regex fullmatch will handle it). Consider whether this fallback is intentional for edge cases or if re.fullmatch() alone is sufficient.
🤖 Prompt for AI Agents
In backend/infrahub/git/utils.py around lines 175 to 179, add a Google-style
docstring that explains the function purpose, arguments, return value, and
examples; then clarify the matching logic by either (a) removing the redundant
exact-equality fallback and relying solely on re.fullmatch(branch_filter,
branch_short_name) (since a plain string pattern will still full-match), or (b)
if the exact-match branch is intentional for an edge case, document that
behaviour in the docstring and add a brief code comment explaining why the
fallback is kept. Ensure the docstring follows project style and update
tests/comments if you change behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/infrahub/git/base.py (2)
777-779: Documentget_filtered_remote_branchesThis new public async helper also needs a Google-style docstring per our Python standards. Please add a short summary plus Returns section.
async def get_filtered_remote_branches(self) -> dict[str, BranchInRemote]: + """Retrieve remote branches and apply the sync filter. + + Returns: + Mapping of remote branches that passed filter_remote_branches. + """ remote_branches = self.get_branches_from_remote() return await self.filter_remote_branches(branches=remote_branches)
490-526: Add the required docstring forfilter_remote_branchesProject Python guidelines call for Google-style docstrings on every function. Please document the new method with Args/Returns so future readers understand the filter contract.
async def filter_remote_branches(self, branches: dict[str, BranchInRemote]) -> dict[str, BranchInRemote]: + """Filter remote branches according to import_sync_branch_names. + + Args: + branches: Mapping of branch names to remote branch metadata. + + Returns: + Filtered mapping containing only branches that satisfy sync rules or exemptions. + """ if not config.SETTINGS.git.import_sync_branch_names or not self.client: return branches
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/git/base.py(5 hunks)python_sdk(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- python_sdk
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/git/base.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/base.py
🧠 Learnings (1)
📚 Learning: 2025-08-15T08:57:37.880Z
Learnt from: BaptisteGi
Repo: opsmill/infrahub PR: 7044
File: docs/docs/guides/change-approval-workflow.mdx:93-107
Timestamp: 2025-08-15T08:57:37.880Z
Learning: In Infrahub's change approval workflow documentation, the "Restricted Access" role intentionally includes `global:edit_default_branch:allow_all` permission despite being described as allowing edits only to branches other than default. This configuration is expected behavior according to the maintainers.
Applied to files:
backend/infrahub/git/base.py
🧬 Code graph analysis (1)
backend/infrahub/git/base.py (4)
backend/infrahub/exceptions.py (1)
BranchNotFoundError(194-200)backend/infrahub/config.py (2)
logging(929-930)git(893-894)backend/infrahub/core/branch/models.py (1)
Branch(28-536)backend/infrahub/git/utils.py (1)
branch_name_in_sync_branches(175-179)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/infrahub/git/base.py (2)
777-779: Add docstring to public method.This method is missing a docstring. Per coding guidelines, document the purpose, return value, and behavior.
Apply this diff:
async def get_filtered_remote_branches(self) -> dict[str, BranchInRemote]: + """Retrieve remote branches and apply sync filtering. + + Fetches all branches from the remote repository and filters them based on + import_sync_branch_names configuration. + + Returns: + Dictionary of filtered remote branches that should be synced. + """ remote_branches = self.get_branches_from_remote()As per coding guidelines.
503-503: Replace ellipsis with explicit pass.Using ellipsis (
...) as an exception handler is unconventional. Usepasswith a comment explaining why the exception is caught.Apply this diff:
except BranchNotFoundError: - ... + pass # Branch doesn't exist in Infrahub, will check import_sync filters below
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/git/base.py(5 hunks)backend/infrahub/git/utils.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/infrahub/git/utils.py
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/git/base.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/base.py
🧠 Learnings (1)
📚 Learning: 2025-08-15T08:57:37.880Z
Learnt from: BaptisteGi
Repo: opsmill/infrahub PR: 7044
File: docs/docs/guides/change-approval-workflow.mdx:93-107
Timestamp: 2025-08-15T08:57:37.880Z
Learning: In Infrahub's change approval workflow documentation, the "Restricted Access" role intentionally includes `global:edit_default_branch:allow_all` permission despite being described as allowing edits only to branches other than default. This configuration is expected behavior according to the maintainers.
Applied to files:
backend/infrahub/git/base.py
🧬 Code graph analysis (1)
backend/infrahub/git/base.py (5)
backend/infrahub/exceptions.py (1)
BranchNotFoundError(194-200)backend/infrahub/config.py (2)
logging(929-930)git(893-894)backend/infrahub/core/branch/models.py (1)
Branch(28-536)backend/infrahub/git/utils.py (1)
branch_name_in_import_sync_branches(175-179)backend/tests/unit/git/conftest.py (1)
import_sync_branch_names(1357-1361)
⏰ 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: validate-generated-documentation
🔇 Additional comments (2)
backend/infrahub/git/base.py (2)
14-14: LGTM: Imports are appropriate.The new imports support the branch filtering functionality and are all used correctly in the implementation.
Also applies to: 20-20, 22-22, 36-36
793-793: LGTM: Filtering correctly integrated.The change correctly applies branch filtering to the comparison flow by using the new
get_filtered_remote_branches()method with properawait.
backend/infrahub/git/base.py
Outdated
| async def filter_remote_branches(self, branches: dict[str, BranchInRemote]) -> dict[str, BranchInRemote]: | ||
| if not config.SETTINGS.git.import_sync_branch_names: | ||
| return branches | ||
|
|
||
| filtered_branches = {} | ||
| skipped_branch_names = [] | ||
|
|
||
| for short_name, branch_data in branches.items(): | ||
| branch = None | ||
|
|
||
| try: | ||
| branch = await self.sdk.branch.get(branch_name=short_name) | ||
| except BranchNotFoundError: | ||
| ... | ||
|
|
||
| branch_exists_import_sync_condition = branch and ( | ||
| branch.name not in {GLOBAL_BRANCH_NAME, registry.default_branch} | ||
| and not branch.is_default | ||
| and not branch.sync_with_git | ||
| and not branch_name_in_import_sync_branches(branch_short_name=short_name) | ||
| ) | ||
| branch_does_not_exist_import_sync_condition = not branch and not branch_name_in_import_sync_branches( | ||
| branch_short_name=short_name | ||
| ) | ||
|
|
||
| if branch_exists_import_sync_condition or branch_does_not_exist_import_sync_condition: | ||
| skipped_branch_names.append(short_name) | ||
| continue | ||
|
|
||
| filtered_branches[short_name] = branch_data | ||
|
|
||
| if skipped_branch_names: | ||
| log.debug( | ||
| f"Skipped the following branches {skipped_branch_names} " | ||
| f"because no match was found in import_sync_branch_names {config.SETTINGS.git.import_sync_branch_names}" | ||
| ) | ||
| return filtered_branches |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add docstring to public method.
This method is missing a docstring. Per coding guidelines, all functions should have docstrings with a brief description, Args, Returns, and any Raises sections.
Apply this diff to add a docstring:
async def filter_remote_branches(self, branches: dict[str, BranchInRemote]) -> dict[str, BranchInRemote]:
+ """Filter remote branches based on sync configuration.
+
+ Applies import_sync_branch_names filtering to remote branches. Branches are kept if they match
+ the configured patterns or meet special conditions (default, global, sync_with_git).
+
+ Args:
+ branches: Dictionary of remote branches to filter.
+
+ Returns:
+ Filtered dictionary containing only branches that should be synced.
+ """
if not config.SETTINGS.git.import_sync_branch_names:As per coding guidelines.
📝 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 filter_remote_branches(self, branches: dict[str, BranchInRemote]) -> dict[str, BranchInRemote]: | |
| if not config.SETTINGS.git.import_sync_branch_names: | |
| return branches | |
| filtered_branches = {} | |
| skipped_branch_names = [] | |
| for short_name, branch_data in branches.items(): | |
| branch = None | |
| try: | |
| branch = await self.sdk.branch.get(branch_name=short_name) | |
| except BranchNotFoundError: | |
| ... | |
| branch_exists_import_sync_condition = branch and ( | |
| branch.name not in {GLOBAL_BRANCH_NAME, registry.default_branch} | |
| and not branch.is_default | |
| and not branch.sync_with_git | |
| and not branch_name_in_import_sync_branches(branch_short_name=short_name) | |
| ) | |
| branch_does_not_exist_import_sync_condition = not branch and not branch_name_in_import_sync_branches( | |
| branch_short_name=short_name | |
| ) | |
| if branch_exists_import_sync_condition or branch_does_not_exist_import_sync_condition: | |
| skipped_branch_names.append(short_name) | |
| continue | |
| filtered_branches[short_name] = branch_data | |
| if skipped_branch_names: | |
| log.debug( | |
| f"Skipped the following branches {skipped_branch_names} " | |
| f"because no match was found in import_sync_branch_names {config.SETTINGS.git.import_sync_branch_names}" | |
| ) | |
| return filtered_branches | |
| async def filter_remote_branches(self, branches: dict[str, BranchInRemote]) -> dict[str, BranchInRemote]: | |
| """Filter remote branches based on sync configuration. | |
| Applies import_sync_branch_names filtering to remote branches. Branches are kept if they match | |
| the configured patterns or meet special conditions (default, global, sync_with_git). | |
| Args: | |
| branches: Dictionary of remote branches to filter. | |
| Returns: | |
| Filtered dictionary containing only branches that should be synced. | |
| """ | |
| if not config.SETTINGS.git.import_sync_branch_names: | |
| return branches | |
| filtered_branches = {} | |
| skipped_branch_names = [] | |
| for short_name, branch_data in branches.items(): | |
| branch = None | |
| try: | |
| branch = await self.sdk.branch.get(branch_name=short_name) | |
| except BranchNotFoundError: | |
| ... | |
| branch_exists_import_sync_condition = branch and ( | |
| branch.name not in {GLOBAL_BRANCH_NAME, registry.default_branch} | |
| and not branch.is_default | |
| and not branch.sync_with_git | |
| and not branch_name_in_import_sync_branches(branch_short_name=short_name) | |
| ) | |
| branch_does_not_exist_import_sync_condition = not branch and not branch_name_in_import_sync_branches( | |
| branch_short_name=short_name | |
| ) | |
| if branch_exists_import_sync_condition or branch_does_not_exist_import_sync_condition: | |
| skipped_branch_names.append(short_name) | |
| continue | |
| filtered_branches[short_name] = branch_data | |
| if skipped_branch_names: | |
| log.debug( | |
| f"Skipped the following branches {skipped_branch_names} " | |
| f"because no match was found in import_sync_branch_names {config.SETTINGS.git.import_sync_branch_names}" | |
| ) | |
| return filtered_branches |
🤖 Prompt for AI Agents
In backend/infrahub/git/base.py around lines 490 to 526, the public async method
filter_remote_branches is missing a docstring; add a comprehensive docstring
immediately above the method that briefly describes its purpose (filters a
mapping of remote branch short names to BranchInRemote according to
import_sync_branch_names and existing local branch properties), documents Args
(branches: dict[str, BranchInRemote] — mapping of short branch names to remote
branch data), Returns (dict[str, BranchInRemote] — filtered mapping of branches
that should be processed), and Raises (document that BranchNotFoundError may be
encountered/handled within the method or note that exceptions from
self.sdk.branch.get may be raised), and mention any important side-effects or
config usage (uses config.SETTINGS.git.import_sync_branch_names and logs skipped
branch names); keep the wording concise and follow existing project docstring
style (brief description, Args, Returns, Raises).
backend/infrahub/git/base.py
Outdated
|
|
||
| async def get_filtered_remote_branches(self) -> dict[str, BranchInRemote]: | ||
| remote_branches = self.get_branches_from_remote() | ||
| return await self.filter_remote_branches(branches=remote_branches) |
There was a problem hiding this comment.
As we don't call filter_remote_branches in other locations and always call .get_filtered_remote_branches() I wonder if it's clearer if we remove the filter_remote_branches() method and just have that code inline within get_filtered_remote_branches()?
backend/infrahub/git/base.py
Outdated
| branch = None | ||
|
|
||
| try: | ||
| branch = await self.sdk.branch.get(branch_name=short_name) |
There was a problem hiding this comment.
I think this could potentially be problematic if the imported repository has a very high number of branches as we'd always try to get the branch name from the API using the SDK. The branches should always be loaded in the registry. so if we instead have something like this we'd just look in a local dictionary and avoid the delay.
branch = registry.get_branch_from_registry(branch=short_name)Alternatively we could add a method in the registry that would return all registry branches (i.e. just return the self.branch dictionary)
backend/infrahub/git/base.py
Outdated
| ... | ||
|
|
||
| branch_exists_import_sync_condition = branch and ( | ||
| branch.name not in {GLOBAL_BRANCH_NAME, registry.default_branch} |
There was a problem hiding this comment.
The GLOBAL_BRANCH_NAME is -global- which is an invalid branch name in Git and as such should never come from a git remote. However the registry itself could have a default branch (self.default_branch) in most cases it will be exactly the same but it doesn't have to be, for that reason we should also add the default branch of the repository to this entry.
backend/infrahub/git/base.py
Outdated
|
|
||
| branch_exists_import_sync_condition = branch and ( | ||
| branch.name not in {GLOBAL_BRANCH_NAME, registry.default_branch} | ||
| and not branch.is_default |
There was a problem hiding this comment.
As we already check for registry.default_branch perhaps this "not branch.is_default` is redundant?
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
Can you also please add a towncrier newsfragment of the "added" kind that adds this as a new feature?
This PR updates the git sync to check the new configuration
import_sync_branch_namesand if it is present it matches the branch name with the names in the config and skips adding it as a remote branch if it does not match with exception to allowsync_with_git==Trueis_default==TrueSummary by CodeRabbit
New Features
Tests