Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds the "Delete Branch After Merge" feature (INFP-389). Introduces two config flags: 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
changed the title
github-actions
Bot
added
the
type/spec
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
dev/wip/ifc-2336/implementation-plan.md (1)
22-22: Clarify the test count breakdown.The breakdown "5 unit + 4 functional + 3 unit" is confusing because it splits unit tests into two groups. Based on the phase table (lines 15-20), the actual breakdown is:
- Phase 1: 3 unit tests
- Phase 2: 4 functional tests
- Phase 3: 3 unit tests
- Phase 4: 2 unit tests
Consider revising to: "Total Tests: 12 tests (8 unit + 4 functional)" for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/wip/ifc-2336/implementation-plan.md` at line 22, Update the ambiguous test summary on the line containing "**Total Tests:** 12 tests (5 unit + 4 functional + 3 unit)" so it clearly reflects the phase breakdown: replace it with "**Total Tests: 12 tests (8 unit + 4 functional)**" and ensure the phase table entries (Phase 1: 3 unit tests, Phase 2: 4 functional tests, Phase 3: 3 unit tests, Phase 4: 2 unit tests) remain consistent with that revised summary.dev/wip/ifc-2336/phase-1.md (1)
42-42: Consider shortening the validator method name.The method name
validate_git_branch_deletion_requires_branch_deletionis very long. Consider a more concise name likevalidate_git_deletion_requires_branch_deletionorcheck_git_deletion_dependencyfor improved readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/wip/ifc-2336/phase-1.md` at line 42, Rename the long validator method validate_git_branch_deletion_requires_branch_deletion to a shorter, clear name (e.g., validate_git_deletion_requires_branch_deletion or check_git_deletion_dependency) and update all call sites and any type references (including its return annotation Self) accordingly; ensure method references in tests, decorators, registrations, and imports are updated to the new name to avoid breakage and run tests to verify.dev/specs/infp-389-branch-merge-delete/tasks.md (1)
76-76: Break down the complex T010 description for readability.The description at line 76 is extremely long (200+ words) and describes complex nested conditional logic all in one sentence. Consider breaking it into a bulleted sub-task list or numbered steps for easier comprehension during implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/infp-389-branch-merge-delete/tasks.md` at line 76, The long T010 instruction should be split into a clear implementation checklist: update the delete_branch() Prefect flow to accept an optional parameter delete_git_branch: bool | None = None; after the existing graph deletion steps (after await query.execute(db=db) and await super().delete(db=db)) add logic that, if obj.sync_with_git is True AND config.SETTINGS.main.delete_branch_after_merge is True AND config.SETTINGS.git.delete_git_branch_after_merge is True, queries all CoreRepository nodes via registry.manager.all(db=db, kind=InfrahubKind.REPOSITORY) (or the nearby pattern) and for each repository calls await get_workflow().submit_workflow(workflow=GIT_REPOSITORY_DELETE_BRANCH, context=context, parameters={"model": GitRepositoryDeleteBranch(repository_id=..., repository_name=..., repository_kind=..., branch_name=branch, context=context).model_dump()}); ensure delete_branch() signature is updated to include the new parameter and that you only implement the plumbing for delete_git_branch (leave override logic for the later task T012).dev/specs/infp-389-branch-merge-delete/contracts/graphql-branch-delete.md (1)
69-71: Consider documenting the non-atomic behavior more prominently.Lines 70-71 state that Git deletion failures are logged but don't cause the mutation to return
ok: false. This means the operation is non-atomic: the Infrahub branch may be deleted successfully while the Git branch deletion fails silently (from the mutation response perspective).While this is clearly documented in the notes section, consider adding a warning or note in the main behavior table (lines 50-59) to make this edge case more visible during implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/infp-389-branch-merge-delete/contracts/graphql-branch-delete.md` around lines 69 - 71, Add a prominent note to the main behavior table explaining the non-atomic nature of the Branch Delete mutation: indicate that the mutation accepts the delete_git_branch parameter (passed through to the BRANCH_DELETE workflow) and that failures to delete the Git branch are logged to the task log and do not cause the mutation to return ok: false; reference the BRANCH_DELETE workflow and delete_git_branch parameter so implementers see the exact symbols to check and ensure the warning is visible alongside the other behavior rows rather than only in the notes section.dev/specs/infp-389-branch-merge-delete/contracts/workflow-git-branch-delete.md (1)
45-52: Clarify explicit skip logic for read-only repositories.The contract says
CoreReadOnlyRepositorybranches are not deleted, but the behavior section does not state where that guard is enforced. Please add an explicit step/condition so implementation is deterministic.Also applies to: 74-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/specs/infp-389-branch-merge-delete/contracts/workflow-git-branch-delete.md` around lines 45 - 52, The workflow is missing an explicit guard for read-only repos before attempting deletion; update the branch-delete procedure (the logic that initializes InfrahubRepository and calls repo.delete_branch_in_git) to check whether the repository instance is a CoreReadOnlyRepository (or repo.is_read_only / similar property) and, if so, log a clear info/message that branch deletion is skipped for read-only repositories and return early without attempting worktree removal, ref deletion, or push; ensure the same explicit check is added where related behavior is described (the other occurrence around lines 74-76) so the skip is deterministic and documented in the flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/specs/infp-389-branch-merge-delete/quickstart.md`:
- Around line 227-230: Update the edge-case note in quickstart.md to reflect the
resolved behavior from dev/wip/ifc-2336/phase-2.md: remove the "may require
additional investigation" qualifier and state that branch deletion is gated by
the reference count of open proposed changes—branches are retained as long as
ANY open proposed change references them and are deleted only when that
reference count reaches zero; reference the BRANCH_CANCEL_PROPOSED_CHANGES
workflow and merge_branch() to clarify that deletion occurs only after all
referencing proposed changes are closed (i.e., when reference count == 0).
In `@dev/specs/infp-389-branch-merge-delete/research.md`:
- Around line 51-52: Update the condition in the delete_branch() task that
checks the settings key: replace references to
config.SETTINGS.git.delete_branch_after_merge with the correct config key
config.SETTINGS.git.delete_git_branch_after_merge so the logic that queries
CoreRepository objects and enqueues GIT_REPOSITORY_DELETE_BRANCH runs only when
the proper setting is enabled; ensure any tests or comments referencing the old
key are updated to use delete_git_branch_after_merge and that the conditional in
backend/infrahub/core/branch/tasks.py uses the corrected attribute name.
- Line 14: The Decision text incorrectly names the Git setting as
delete_branch_after_merge; update the spec so that MainSettings keeps
delete_branch_after_merge: bool = False but GitSettings defines
delete_git_branch_after_merge: bool = False instead, and adjust any references
in the Decision paragraph and elsewhere in this document to use the symbol
delete_git_branch_after_merge for GitSettings to match the rest of the spec set
and avoid confusion with MainSettings.
In `@dev/specs/infp-389-branch-merge-delete/tasks.md`:
- Line 76: The delete_branch() flow must distinguish auto vs manual
Git-deletion: add an optional parameter delete_git_branch: bool | None = None to
delete_branch signature, and after the graph and super().delete calls check
deletion using obj.sync_with_git,
config.SETTINGS.git.delete_git_branch_after_merge, and
config.SETTINGS.main.delete_branch_after_merge combined with the explicit
parameter so an explicit delete_git_branch=True forces deletion; implement the
decision exactly as: should_delete_git = (obj.sync_with_git and
config.SETTINGS.git.delete_git_branch_after_merge and (delete_git_branch is True
or (delete_git_branch is None and
config.SETTINGS.main.delete_branch_after_merge))); if should_delete_git, query
all repositories via registry.manager.all(db=db, kind=InfrahubKind.REPOSITORY)
(or the surrounding-task pattern) and for each repo call await
get_workflow().submit_workflow(workflow=GIT_REPOSITORY_DELETE_BRANCH,
context=context, parameters={"model":
GitRepositoryDeleteBranch(repository_id=..., repository_name=...,
repository_kind=..., branch_name=branch, context=context).model_dump()}) to
enqueue the branch-deletion work.
In `@dev/wip/ifc-2336/phase-3.md`:
- Around line 91-104: The code currently fans out to every CoreRepository
(repositories = await client.filters(kind=CoreRepository)) which can delete the
branch across unrelated repos; change the query or add a guard so only
repositories synced to the branch are targeted. Replace or augment the
client.filters call to return only repos that list the branch (e.g., filter by a
synced_branches/sync_policy field) or, before calling batch.add, check a
repository property (e.g., repository.synced_branches or repository.sync_policy)
and skip repos that are not associated with the branch; keep the rest of the
batch logic (batch.add with git_branch_delete, client, branch,
repository_name/repository_id/repository_location) unchanged.
- Around line 20-22: The workflow shape is inconsistent: the diff adds a fan-out
GIT_REPOSITORIES_DELETE_BRANCH but the contract expects per-repository
GIT_REPOSITORY_DELETE_BRANCH; remove the aggregate workflow and implement the
per-repo flow instead—implement delete_git_branch() and git_branch_delete in
git/tasks.py as a per-repository task, update delete_branch() to accept
delete_from_git and have it iterate repositories and submit
GIT_REPOSITORY_DELETE_BRANCH for each repo (rather than a single
GIT_REPOSITORIES_DELETE_BRANCH fan-out), ensuring all references use the
GIT_REPOSITORY_DELETE_BRANCH constant and the
delete_git_branch()/git_branch_delete handlers.
- Around line 160-162: The should_delete_git condition uses the wrong config
namespace: change the reference from settings.main.delete_git_branch_after_merge
to settings.git.delete_git_branch_after_merge inside the expression that
computes should_delete_git (the line using get_settings(), delete_from_git, and
obj.sync_with_git) so it reads: should_delete_git =
(settings.git.delete_git_branch_after_merge or delete_from_git) and
obj.sync_with_git; this ensures the flag is read from the Git settings
namespace.
In `@dev/wip/ifc-2336/phase-4.md`:
- Around line 1-105: The issue is a naming inconsistency between
"delete_from_git" used in this Phase 4 doc and "delete_git_branch" used in the
contract and test names; pick one canonical argument name and update all
references to match (either rename the new Input field
BranchDeleteInput.delete_from_git to delete_git_branch, update
BranchDelete.Arguments and the mutate() signature and parameter usage, and
rename the unit tests test_branch_delete_with_git_option_triggers_git_deletion /
test_branch_delete_without_git_option_skips_git_deletion to use the chosen
name), and also update the contract file
dev/specs/infp-389-branch-merge-delete/contracts/graphql-branch-delete.md so the
GraphQL argument name is identical across BranchDeleteInput, BranchDelete, the
workflow parameter keys, and tests.
- Line 77: The code incorrectly treats the typed input BranchDeleteInput
instance `data` as a dict by calling data.get("delete_from_git", False); change
this to use attribute access (e.g., use data.delete_from_git) or use
getattr(data, "delete_from_git", False) so you respect the BranchDeleteInput
type and its default behavior instead of dict-style access.
---
Nitpick comments:
In `@dev/specs/infp-389-branch-merge-delete/contracts/graphql-branch-delete.md`:
- Around line 69-71: Add a prominent note to the main behavior table explaining
the non-atomic nature of the Branch Delete mutation: indicate that the mutation
accepts the delete_git_branch parameter (passed through to the BRANCH_DELETE
workflow) and that failures to delete the Git branch are logged to the task log
and do not cause the mutation to return ok: false; reference the BRANCH_DELETE
workflow and delete_git_branch parameter so implementers see the exact symbols
to check and ensure the warning is visible alongside the other behavior rows
rather than only in the notes section.
In
`@dev/specs/infp-389-branch-merge-delete/contracts/workflow-git-branch-delete.md`:
- Around line 45-52: The workflow is missing an explicit guard for read-only
repos before attempting deletion; update the branch-delete procedure (the logic
that initializes InfrahubRepository and calls repo.delete_branch_in_git) to
check whether the repository instance is a CoreReadOnlyRepository (or
repo.is_read_only / similar property) and, if so, log a clear info/message that
branch deletion is skipped for read-only repositories and return early without
attempting worktree removal, ref deletion, or push; ensure the same explicit
check is added where related behavior is described (the other occurrence around
lines 74-76) so the skip is deterministic and documented in the flow.
In `@dev/specs/infp-389-branch-merge-delete/tasks.md`:
- Line 76: The long T010 instruction should be split into a clear implementation
checklist: update the delete_branch() Prefect flow to accept an optional
parameter delete_git_branch: bool | None = None; after the existing graph
deletion steps (after await query.execute(db=db) and await
super().delete(db=db)) add logic that, if obj.sync_with_git is True AND
config.SETTINGS.main.delete_branch_after_merge is True AND
config.SETTINGS.git.delete_git_branch_after_merge is True, queries all
CoreRepository nodes via registry.manager.all(db=db,
kind=InfrahubKind.REPOSITORY) (or the nearby pattern) and for each repository
calls await
get_workflow().submit_workflow(workflow=GIT_REPOSITORY_DELETE_BRANCH,
context=context, parameters={"model":
GitRepositoryDeleteBranch(repository_id=..., repository_name=...,
repository_kind=..., branch_name=branch, context=context).model_dump()}); ensure
delete_branch() signature is updated to include the new parameter and that you
only implement the plumbing for delete_git_branch (leave override logic for the
later task T012).
In `@dev/wip/ifc-2336/implementation-plan.md`:
- Line 22: Update the ambiguous test summary on the line containing "**Total
Tests:** 12 tests (5 unit + 4 functional + 3 unit)" so it clearly reflects the
phase breakdown: replace it with "**Total Tests: 12 tests (8 unit + 4
functional)**" and ensure the phase table entries (Phase 1: 3 unit tests, Phase
2: 4 functional tests, Phase 3: 3 unit tests, Phase 4: 2 unit tests) remain
consistent with that revised summary.
In `@dev/wip/ifc-2336/phase-1.md`:
- Line 42: Rename the long validator method
validate_git_branch_deletion_requires_branch_deletion to a shorter, clear name
(e.g., validate_git_deletion_requires_branch_deletion or
check_git_deletion_dependency) and update all call sites and any type references
(including its return annotation Self) accordingly; ensure method references in
tests, decorators, registrations, and imports are updated to the new name to
avoid breakage and run tests to verify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca3f181f-046a-4626-8b9f-3654c17f6d08
📒 Files selected for processing (13)
dev/specs/infp-389-branch-merge-delete/contracts/graphql-branch-delete.mddev/specs/infp-389-branch-merge-delete/contracts/rest-config-response.mddev/specs/infp-389-branch-merge-delete/contracts/workflow-git-branch-delete.mddev/specs/infp-389-branch-merge-delete/data-model.mddev/specs/infp-389-branch-merge-delete/plan.mddev/specs/infp-389-branch-merge-delete/quickstart.mddev/specs/infp-389-branch-merge-delete/research.mddev/specs/infp-389-branch-merge-delete/tasks.mddev/wip/ifc-2336/implementation-plan.mddev/wip/ifc-2336/phase-1.mddev/wip/ifc-2336/phase-2.mddev/wip/ifc-2336/phase-3.mddev/wip/ifc-2336/phase-4.md
Comment on lines
+227
to
+230
|
|
||
| **Expected**: Branch is NOT deleted (second proposed change still open). Branch only deleted after all proposed changes are resolved. | ||
|
|
||
| > **Note**: This edge case may require additional investigation during implementation. The current `BRANCH_CANCEL_PROPOSED_CHANGES` workflow is called as part of `merge_branch()`. Whether to gate deletion on "all PCs merged" vs "any PC merged" needs verification against actual behavior. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "quickstart.md" | head -20Repository: opsmill/infrahub
Length of output: 1853
🏁 Script executed:
find . -type f -name "phase-2.md" | head -20Repository: opsmill/infrahub
Length of output: 1853
🏁 Script executed:
# Read lines 220-240 of quickstart.md to see the context around lines 227-230
sed -n '220,240p' ./dev/specs/infp-389-branch-merge-delete/quickstart.mdRepository: opsmill/infrahub
Length of output: 1853
🏁 Script executed:
# Read the phase-2.md file, particularly lines 59-74
sed -n '50,85p' ./dev/wip/ifc-2336/phase-2.mdRepository: opsmill/infrahub
Length of output: 1853
🏁 Script executed:
# Search for references to branch deletion and proposed changes to understand broader context
rg "BRANCH_CANCEL_PROPOSED_CHANGES|branch.*delet" --type md -C 3 | head -80Repository: opsmill/infrahub
Length of output: 1853
🏁 Script executed:
# Get broader context around phase-2.md lines 50-65 to see the full discussion
sed -n '40,70p' ./dev/wip/ifc-2336/phase-2.mdRepository: opsmill/infrahub
Length of output: 1853
Update quickstart.md edge case note to reference the resolved behavior from phase-2.md.
The uncertainty note in lines 227-230 has been resolved in dev/wip/ifc-2336/phase-2.md (lines 43-55). The correct behavior is: branches should be deleted only when no open proposed changes remain (not a binary "all merged" vs "any merged" decision). Remove the "may require additional investigation" language and update the note to state: branches are retained as long as ANY open proposed change references them, and deleted only when the reference count reaches zero.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/specs/infp-389-branch-merge-delete/quickstart.md` around lines 227 - 230,
Update the edge-case note in quickstart.md to reflect the resolved behavior from
dev/wip/ifc-2336/phase-2.md: remove the "may require additional investigation"
qualifier and state that branch deletion is gated by the reference count of open
proposed changes—branches are retained as long as ANY open proposed change
references them and are deleted only when that reference count reaches zero;
reference the BRANCH_CANCEL_PROPOSED_CHANGES workflow and merge_branch() to
clarify that deletion occurs only after all referencing proposed changes are
closed (i.e., when reference count == 0).
Comment on lines
+20
to
+22
| - [ ] Add `GIT_REPOSITORIES_DELETE_BRANCH` workflow definition to catalogue | ||
| - [ ] Implement `delete_git_branch()` flow + `git_branch_delete` task in `git/tasks.py` | ||
| - [ ] Update `delete_branch()` to accept `delete_from_git` param and trigger git deletion |
Contributor
There was a problem hiding this comment.
Workflow shape conflicts with the contract set in this PR.
This phase defines GIT_REPOSITORIES_DELETE_BRANCH + a fan-out flow over all repositories, but the contract/plan defines GIT_REPOSITORY_DELETE_BRANCH as a per-repository workflow submission from branch deletion logic. Please align to one model to avoid divergent implementations.
Also applies to: 72-77, 84-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/wip/ifc-2336/phase-3.md` around lines 20 - 22, The workflow shape is
inconsistent: the diff adds a fan-out GIT_REPOSITORIES_DELETE_BRANCH but the
contract expects per-repository GIT_REPOSITORY_DELETE_BRANCH; remove the
aggregate workflow and implement the per-repo flow instead—implement
delete_git_branch() and git_branch_delete in git/tasks.py as a per-repository
task, update delete_branch() to accept delete_from_git and have it iterate
repositories and submit GIT_REPOSITORY_DELETE_BRANCH for each repo (rather than
a single GIT_REPOSITORIES_DELETE_BRANCH fan-out), ensuring all references use
the GIT_REPOSITORY_DELETE_BRANCH constant and the
delete_git_branch()/git_branch_delete handlers.
Comment on lines
+1
to
+105
| # Phase 4: Manual Delete with Git Option | ||
|
|
||
| **Status:** ⬜ Todo | ||
| **Priority:** P3 | ||
| **Requirements:** FR-008, FR-009 | ||
| **Depends on:** Phase 3 | ||
|
|
||
| --- | ||
|
|
||
| ## Goal | ||
|
|
||
| Expose `delete_from_git` as an optional parameter on the `BranchDelete` GraphQL mutation. This lets the UI pass `delete_from_git: true` on manual deletion, triggering Git branch deletion even when the global `delete_git_branch_after_merge` setting is disabled. The actual deletion logic is already handled by `delete_branch()` after Phase 3. | ||
|
|
||
| --- | ||
|
|
||
| ## Checklist | ||
|
|
||
| - [ ] Add `BranchDeleteInput` class (new — `BranchNameInput` is unchanged) | ||
| - [ ] Switch `BranchDelete.Arguments` and `mutate()` type hint from `BranchNameInput` to `BranchDeleteInput` | ||
| - [ ] Pass `delete_from_git` through to the `BRANCH_DELETE` workflow parameters | ||
| - [ ] Write unit tests | ||
|
|
||
| --- | ||
|
|
||
| ## Implementation | ||
|
|
||
| ### 4.1 Add BranchDeleteInput | ||
|
|
||
| **File:** `backend/infrahub/graphql/mutations/branch.py` | ||
|
|
||
| `BranchNameInput` (line 108) is shared by `BranchRebase`, `BranchValidate`, and others — do not touch it. Add a new class immediately after it: | ||
|
|
||
| ```python | ||
| class BranchDeleteInput(InputObjectType): | ||
| name = String(required=False) | ||
| delete_from_git = Boolean(required=False, default_value=False) | ||
| ``` | ||
|
|
||
| ### 4.2 Switch BranchDelete to BranchDeleteInput | ||
|
|
||
| **File:** `backend/infrahub/graphql/mutations/branch.py` | ||
|
|
||
| `BranchDelete` currently uses `BranchNameInput` in two places (lines 120 and 132). Update both: | ||
|
|
||
| ```python | ||
| class BranchDelete(Mutation): | ||
| class Arguments: | ||
| data = BranchDeleteInput(required=True) # was BranchNameInput | ||
| context = ContextInput(required=False) | ||
| wait_until_completion = Boolean(required=False) | ||
|
|
||
| ok = Boolean() | ||
| task = Field(TaskInfo, required=False) | ||
|
|
||
| @classmethod | ||
| async def mutate( | ||
| cls, | ||
| root: dict, # noqa: ARG003 | ||
| info: GraphQLResolveInfo, | ||
| data: BranchDeleteInput, # was BranchNameInput | ||
| context: ContextInput | None = None, | ||
| wait_until_completion: bool = True, | ||
| ) -> Self: | ||
| ``` | ||
|
|
||
| ### 4.3 Pass delete_from_git to the workflow | ||
|
|
||
| In the `mutate()` body, extract the flag and include it in both workflow call sites (execute and submit): | ||
|
|
||
| ```python | ||
| graphql_context: GraphqlContext = info.context | ||
| obj = await Branch.get_by_name(db=graphql_context.db, name=str(data.name)) | ||
| await apply_external_context(graphql_context=graphql_context, context_input=context) | ||
|
|
||
| parameters = { | ||
| "branch": obj.name, | ||
| "delete_from_git": data.get("delete_from_git", False), | ||
| } | ||
|
|
||
| if wait_until_completion: | ||
| await graphql_context.active_service.workflow.execute_workflow( | ||
| workflow=BRANCH_DELETE, context=graphql_context.get_context(), parameters=parameters | ||
| ) | ||
| return cls(ok=True) | ||
|
|
||
| workflow = await graphql_context.active_service.workflow.submit_workflow( | ||
| workflow=BRANCH_DELETE, context=graphql_context.get_context(), parameters=parameters | ||
| ) | ||
| return cls(ok=True, task={"id": str(workflow.id)}) | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Tests | ||
|
|
||
| **File:** `backend/tests/unit/graphql/mutations/test_branch_delete.py` | ||
|
|
||
| - `test_branch_delete_with_git_option_triggers_git_deletion` — call mutation with `delete_from_git: true`, assert `GIT_REPOSITORIES_DELETE_BRANCH` workflow is submitted | ||
| - `test_branch_delete_without_git_option_skips_git_deletion` — call mutation with `delete_from_git: false` (and config disabled), assert git workflow not submitted | ||
|
|
||
| **Verification:** | ||
|
|
||
| ```bash | ||
| uv run pytest backend/tests/unit/graphql/mutations/test_branch_delete.py -v | ||
| ``` |
Contributor
There was a problem hiding this comment.
Naming inconsistency: delete_from_git vs delete_git_branch.
This document consistently uses delete_from_git as the parameter name (lines 12, 20, 36, 66, 77), but dev/specs/infp-389-branch-merge-delete/contracts/graphql-branch-delete.md uses delete_git_branch for the same GraphQL mutation argument (line 16, 22, 39). The test names at lines 98-99 also reference "delete_git_branch".
Standardize the naming across all specification documents to prevent implementation confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/wip/ifc-2336/phase-4.md` around lines 1 - 105, The issue is a naming
inconsistency between "delete_from_git" used in this Phase 4 doc and
"delete_git_branch" used in the contract and test names; pick one canonical
argument name and update all references to match (either rename the new Input
field BranchDeleteInput.delete_from_git to delete_git_branch, update
BranchDelete.Arguments and the mutate() signature and parameter usage, and
rename the unit tests test_branch_delete_with_git_option_triggers_git_deletion /
test_branch_delete_without_git_option_skips_git_deletion to use the chosen
name), and also update the contract file
dev/specs/infp-389-branch-merge-delete/contracts/graphql-branch-delete.md so the
GraphQL argument name is identical across BranchDeleteInput, BranchDelete, the
workflow parameter keys, and tests.
* Add config settings * update implementation plan and phase * update docs and schema * update frontend types * move git settings to appropiate GitSettings * fix lint error * update error message
github-actions
Bot
added
type/documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/infrahub/config.py (1)
1042-1046: Add a docstring to the new validator method.The validator logic looks good, but the new method should include a triple-quoted Google-style docstring for consistency with repo standards.
As per coding guidelines,
**/*.py: "Always use triple quotes (""") for Python docstrings" and "Follow Google-style docstring format in Python".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/config.py` around lines 1042 - 1046, Add a triple-quoted Google-style docstring to the validator method validate_git_branch_deletion_requires_branch_deletion explaining its purpose and behavior: state that it runs after model validation, enforces that git.delete_git_branch_after_merge can only be true when main.delete_branch_after_merge is true, and mention the ValueError raised when the constraint is violated; use triple double quotes (""") and follow the repo's Google-style formatting conventions.schema/openapi.json (1)
5027-5031: Avoid direct maintenance of this generated schema file.Please make sure this change comes from regenerating OpenAPI from source models/routers, not by editing
schema/openapi.jsondirectly.As per coding guidelines,
schema/openapi.json:Do not edit schema/openapi.json - this is an auto-generated OpenAPI schema file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schema/openapi.json` around lines 5027 - 5031, The change to the generated OpenAPI property "delete_branch_after_merge" was made directly in the generated schema; revert that manual edit and instead update the source model or router that defines this field (the model/route responsible for the Infrahub branch merge behavior) and then re-run the OpenAPI generation process to regenerate the schema; ensure the source change (model property or route parameter) carries the desired title/description/default and commit the regenerated OpenAPI output rather than editing schema/openapi.json by hand.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/unit/test_config.py`:
- Around line 55-59: The test
test_delete_git_branch_after_merge_without_delete_branch_after_merge_raises
should expect a pydantic ValidationError instead of ValueError because Settings
validation errors are wrapped; update the pytest.raises context to use
ValidationError from pydantic (ensure ValidationError is imported) when
constructing Settings(GitSettings(...), MainSettings(...)) so the test asserts
the correct exception type for model validation.
In `@schema/openapi.json`:
- Around line 5027-5031: The OpenAPI schema is missing the
git.delete_git_branch_after_merge property which exists in
backend/infrahub/config.py (class GitSettings) and is validated together with
MainSettings.delete_branch_after_merge by
validate_git_branch_deletion_requires_branch_deletion; update
schema/openapi.json to add a boolean property named
delete_git_branch_after_merge (with title/description/default matching project
conventions) under the appropriate GitSettings/ConfigAPI section so the contract
exposes GitSettings.delete_git_branch_after_merge consistently with
MainSettings.delete_branch_after_merge.
---
Nitpick comments:
In `@backend/infrahub/config.py`:
- Around line 1042-1046: Add a triple-quoted Google-style docstring to the
validator method validate_git_branch_deletion_requires_branch_deletion
explaining its purpose and behavior: state that it runs after model validation,
enforces that git.delete_git_branch_after_merge can only be true when
main.delete_branch_after_merge is true, and mention the ValueError raised when
the constraint is violated; use triple double quotes (""") and follow the repo's
Google-style formatting conventions.
In `@schema/openapi.json`:
- Around line 5027-5031: The change to the generated OpenAPI property
"delete_branch_after_merge" was made directly in the generated schema; revert
that manual edit and instead update the source model or router that defines this
field (the model/route responsible for the Infrahub branch merge behavior) and
then re-run the OpenAPI generation process to regenerate the schema; ensure the
source change (model property or route parameter) carries the desired
title/description/default and commit the regenerated OpenAPI output rather than
editing schema/openapi.json by hand.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1956d53-e615-4e2c-b838-45e2617e0cda
📒 Files selected for processing (7)
backend/infrahub/config.pybackend/tests/unit/test_config.pydev/wip/ifc-2336/implementation-plan.mddev/wip/ifc-2336/phase-1.mddocs/docs/reference/configuration.mdxfrontend/app/src/shared/api/rest/types.generated.tsschema/openapi.json
✅ Files skipped from review due to trivial changes (3)
- dev/wip/ifc-2336/phase-1.md
- docs/docs/reference/configuration.mdx
- dev/wip/ifc-2336/implementation-plan.md
Comment on lines
+55
to
+59
| def test_delete_git_branch_after_merge_without_delete_branch_after_merge_raises() -> None: | ||
| with pytest.raises(ValueError, match=re.escape("requires 'delete_branch_after_merge' to be enabled")): | ||
| Settings( | ||
| git=GitSettings(delete_git_branch_after_merge=True), main=MainSettings(delete_branch_after_merge=False) | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how similar validation cases are asserted across this repo.
# Locate model validators that raise ValueError in config.
rg -n -C3 '@model_validator|raise ValueError' backend/infrahub/config.py
# Inspect current exception assertions in config tests.
rg -n -C3 'pytest\.raises\((ValueError|ValidationError)' backend/tests/unit/test_config.pyRepository: opsmill/infrahub
Length of output: 3812
🏁 Script executed:
# Check imports in test_config.py to see if ValidationError is imported
head -30 backend/tests/unit/test_config.py
# Check the full imports section
rg -n "^import|^from" backend/tests/unit/test_config.py | head -20Repository: opsmill/infrahub
Length of output: 1007
🏁 Script executed:
# Run the specific tests to see if they pass or fail
cd backend && python -m pytest tests/unit/test_config.py::test_invalid_git_settings__sync_branch_names -v 2>&1 | head -100
cd backend && python -m pytest tests/unit/test_config.py::test_delete_git_branch_after_merge_without_delete_branch_after_merge_raises -v 2>&1 | head -100
cd backend && python -m pytest tests/unit/test_config.py::test_storage_max_file_size_rejects_invalid_values -v 2>&1 | head -100Repository: opsmill/infrahub
Length of output: 393
🏁 Script executed:
# Search for ValidationError usage in test files
rg -n "pytest\.raises\(ValidationError" backend/tests/ | head -20
# Search for ValueError usage in pytest.raises
rg -n "pytest\.raises\(ValueError" backend/tests/ | head -20Repository: opsmill/infrahub
Length of output: 4658
🏁 Script executed:
# Check if GitSettings has field validators that might raise ValueError differently
rg -n "class GitSettings" backend/infrahub/config.py -A 50 | head -80Repository: opsmill/infrahub
Length of output: 2397
Assert ValidationError instead of ValueError for Settings validation.
In Pydantic v2, exceptions raised inside @model_validator are automatically wrapped in ValidationError during model construction. Line 56 incorrectly expects ValueError directly. The repository pattern (e.g., line 70 in the same file) confirms that Pydantic validation errors should be caught as ValidationError.
💡 Proposed fix
def test_delete_git_branch_after_merge_without_delete_branch_after_merge_raises() -> None:
- with pytest.raises(ValueError, match=re.escape("requires 'delete_branch_after_merge' to be enabled")):
+ with pytest.raises(
+ ValidationError,
+ match=re.escape("'delete_git_branch_after_merge' requires 'delete_branch_after_merge' to be enabled"),
+ ):
Settings(
git=GitSettings(delete_git_branch_after_merge=True), main=MainSettings(delete_branch_after_merge=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.
Suggested change
| def test_delete_git_branch_after_merge_without_delete_branch_after_merge_raises() -> None: | |
| with pytest.raises(ValueError, match=re.escape("requires 'delete_branch_after_merge' to be enabled")): | |
| Settings( | |
| git=GitSettings(delete_git_branch_after_merge=True), main=MainSettings(delete_branch_after_merge=False) | |
| ) | |
| def test_delete_git_branch_after_merge_without_delete_branch_after_merge_raises() -> None: | |
| with pytest.raises( | |
| ValidationError, | |
| match=re.escape("'delete_git_branch_after_merge' requires 'delete_branch_after_merge' to be enabled"), | |
| ): | |
| Settings( | |
| git=GitSettings(delete_git_branch_after_merge=True), main=MainSettings(delete_branch_after_merge=False) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/unit/test_config.py` around lines 55 - 59, The test
test_delete_git_branch_after_merge_without_delete_branch_after_merge_raises
should expect a pydantic ValidationError instead of ValueError because Settings
validation errors are wrapped; update the pytest.raises context to use
ValidationError from pydantic (ensure ValidationError is imported) when
constructing Settings(GitSettings(...), MainSettings(...)) so the test asserts
the correct exception type for model validation.
* Add config settings * Add delete branch after merge * update implementation plan and phase * update implementation plan and phases * update docs and schema * update frontend types * cancel existing proposed changes on merge * move git settings to appropiate GitSettings * fix lint error * remove unnecessary test * update error message * remove unneeded cancel proposed change * Add delete branch from git (#8616) * Add delete branch from git * add GIT_REPOSITORIES_DELETE_BRANCH to workflow * update implementation plan * update git settings * add gogs * Add manual delete git branch (#8618) * Add manual delete git branch * update schema and impelementation plan * update graphql types * revert python_sdk
ogenstad
approved these changes
Mar 23, 2026
Contributor
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
As Coderabbit mentions due to the change of the git option some specs might be a bit outdated now. Could be nice to change that in the specs so that they aren't obsolete from the start.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
backend/infrahub/git/base.py (1)
543-551: Consider wrapping blocking git operations withasyncio.to_thread().The method is declared
asyncbut performs synchronous blocking I/O (repo.git.push,repo.delete_head). This will block the event loop during execution. While this is consistent with other methods in this class, consider wrapping the blocking operations:import asyncio async def delete_remote_branch(self, branch_name: str) -> None: """Delete branch_name from origin and remove the local tracking ref.""" if not self.has_origin: return repo = self.get_git_repo_main() await asyncio.to_thread(repo.git.push, "origin", "--delete", branch_name) local_branches = self.get_branches_from_local(include_worktree=False) if branch_name in local_branches: await asyncio.to_thread(repo.delete_head, branch_name, force=True)This would prevent blocking the event loop during git network operations.
As per coding guidelines: "Never block event loop with sync I/O".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/git/base.py` around lines 543 - 551, The async method delete_remote_branch performs blocking sync I/O (repo.git.push and repo.delete_head) and should offload these calls to threads: call get_git_repo_main and then run repo.git.push("origin","--delete", branch_name) via asyncio.to_thread (or equivalent), fetch local_branches via get_branches_from_local, and if branch_name in local_branches call repo.delete_head(branch_name, force=True) via asyncio.to_thread; keep the has_origin early return unchanged and preserve behavior but prevent blocking the event loop.backend/infrahub/git/tasks.py (1)
339-359: Consider adding return type hint for consistency.The task function lacks a return type hint. Other tasks in this file (e.g.,
git_branch_createat Line 303) also omit it, but adding-> Nonewould improve clarity per coding guidelines.Optional improvement
async def git_branch_delete( client: InfrahubClient, branch: str, repository_id: str, repository_name: str, repository_location: str, -) -> None: +) -> None:(Already has
-> None- disregard if present; if missing, add it)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/infrahub/git/tasks.py` around lines 339 - 359, The function git_branch_delete is missing an explicit return type hint; update its signature to include "-> None" (e.g., async def git_branch_delete(... ) -> None:) to match the file's style and clarify that it returns nothing; locate the async def git_branch_delete declaration and add the annotation, and mirror the same convention for other task functions like git_branch_create if they also lack it.backend/tests/functional/branch/test_delete_git_branch.py (1)
24-36: Consider adding a test for explicitdelete_from_git=Falsewhen config is enabled.The current test matrix covers:
Config Enabled sync_with_git delete_from_git Expected True True (not set) Triggered False True (not set) Not triggered True False (not set) Not triggered False True True Triggered False True False Not triggered Missing scenario:
config=True,sync_with_git=True,delete_from_git=False— this would test whether an explicitFalsecan override an enabled config to suppress deletion.🧪 Optional: Add test for explicit disable override
async def test_git_deletion_not_triggered_when_delete_from_git_false_overrides_config_enabled( self, initial_dataset: None, client: InfrahubClient, delete_git_branch_after_merge_reset_config: None ) -> None: config.SETTINGS.git.delete_git_branch_after_merge = True branch = await client.branch.create(branch_name="git_del_explicit_false_override", sync_with_git=True) with patch.object(WorkflowLocalExecution, "submit_workflow", new_callable=AsyncMock) as mock_submit: query = Mutation( mutation="BranchDelete", input_data={"data": {"name": branch.name, "delete_from_git": False}}, query={"ok": None}, ) await client.execute_graphql(query=query.render()) git_del_calls = [ c for c in mock_submit.call_args_list if c.kwargs.get("workflow") == GIT_REPOSITORIES_DELETE_BRANCH ] assert not git_del_calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/functional/branch/test_delete_git_branch.py` around lines 24 - 36, Add a new async test (e.g., test_git_deletion_not_triggered_when_delete_from_git_false_overrides_config_enabled) that sets config.SETTINGS.git.delete_git_branch_after_merge = True, creates a branch with client.branch.create(..., sync_with_git=True), then calls the BranchDelete GraphQL mutation with input {"name": branch.name, "delete_from_git": False} and patches WorkflowLocalExecution.submit_workflow to AsyncMock; finally assert no calls were made to submit_workflow with workflow == GIT_REPOSITORIES_DELETE_BRANCH (i.e., deletion workflow is not triggered). Use the existing patterns in TestDeleteBranchGitWorkflow, Mutation helper, and mock_submit call inspection to locate and implement the test.backend/tests/integration/git/test_delete_git_branch_gogs.py (1)
32-37: Consider checking the response status in_delete_branch_on_gogs.The function silently discards the HTTP response. If the deletion fails unexpectedly (e.g., 500 error), the test won't know. While Test 2 intentionally tolerates missing branches, Test 1's setup might benefit from knowing if the branch was actually deleted.
🔧 Optional: Return status or raise on unexpected errors
-def _delete_branch_on_gogs(base_url: str, token: str, repo_name: str, branch_name: str) -> None: - httpx.delete( +def _delete_branch_on_gogs(base_url: str, token: str, repo_name: str, branch_name: str) -> bool: + """Delete a branch on Gogs. Returns True if deleted, False if not found.""" + resp = httpx.delete( f"{base_url}/api/v1/repos/{GOGS_ADMIN}/{repo_name}/branches/{branch_name}", headers={"Authorization": f"token {token}"}, timeout=10.0, ) + if resp.status_code == 404: + return False + resp.raise_for_status() + return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/integration/git/test_delete_git_branch_gogs.py` around lines 32 - 37, The helper _delete_branch_on_gogs currently discards the httpx response; capture the response from httpx.delete, check its status_code, and surface failures: treat 200/204 (or 404 if tests tolerate missing branches) as OK/ignored, but for any other status raise an error (or call response.raise_for_status()) including the status and response text so Test 1 can fail on unexpected deletion errors; locate the call in function _delete_branch_on_gogs and modify it to inspect the response rather than dropping it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@changelog/`+bee22d82.added.md:
- Line 1: The changelog sentence is awkward and has typos; rewrite it to be
clear that branch deletion occurs after merge and that deleting the Git branch
depends on the main setting. Replace the line to say something like: enabling
delete_branch_after_merge allows Infrahub to delete branches after a merge, and
if delete_git_branch_after_merge is also set to True, Infrahub will delete the
corresponding Git branch as well (both variables default to False). Ensure you
use "synced" instead of "synched" and remove the misplaced "also."
- Line 1: Update the changelog entry to use the actual environment variable
names derived from the settings' env_prefixes: replace
`delete_branch_after_merge` with INFRAHUB_DELETE_BRANCH_AFTER_MERGE and replace
`delete_git_branch_after_merge` with INFRAHUB_GIT_DELETE_GIT_BRANCH_AFTER_MERGE;
you can verify these prefixes on MainSettings (env_prefix="INFRAHUB_") and
GitSettings (env_prefix="INFRAHUB_GIT_") in the backend/infrahub/config.py to
ensure the documented variable names match the runtime configuration.
- Line 1: Update the changelog entry to document the BranchDelete GraphQL
mutation's per-request override: mention the delete_from_git parameter (in
backend/infrahub/graphql/mutations/branch.py, BranchDelete mutation) and explain
it allows callers to override the global delete_git_branch_after_merge
environment setting to force or prevent deleting the git branch for that
specific request; briefly note default behavior when the parameter is omitted.
In `@dev/specs/infp-389-branch-merge-delete/contracts/graphql-branch-delete.md`:
- Line 80: The backward-compatibility note incorrectly references the parameter
name `delete_git_branch`; update that note to use the actual input field name
`delete_from_git` so it matches the input definition (see `delete_from_git` in
the input fields declared around the top and the mutation payload). Ensure any
mention of the old name is removed or replaced so the note reads that callers
omitting `delete_from_git` continue to work.
---
Nitpick comments:
In `@backend/infrahub/git/base.py`:
- Around line 543-551: The async method delete_remote_branch performs blocking
sync I/O (repo.git.push and repo.delete_head) and should offload these calls to
threads: call get_git_repo_main and then run repo.git.push("origin","--delete",
branch_name) via asyncio.to_thread (or equivalent), fetch local_branches via
get_branches_from_local, and if branch_name in local_branches call
repo.delete_head(branch_name, force=True) via asyncio.to_thread; keep the
has_origin early return unchanged and preserve behavior but prevent blocking the
event loop.
In `@backend/infrahub/git/tasks.py`:
- Around line 339-359: The function git_branch_delete is missing an explicit
return type hint; update its signature to include "-> None" (e.g., async def
git_branch_delete(... ) -> None:) to match the file's style and clarify that it
returns nothing; locate the async def git_branch_delete declaration and add the
annotation, and mirror the same convention for other task functions like
git_branch_create if they also lack it.
In `@backend/tests/functional/branch/test_delete_git_branch.py`:
- Around line 24-36: Add a new async test (e.g.,
test_git_deletion_not_triggered_when_delete_from_git_false_overrides_config_enabled)
that sets config.SETTINGS.git.delete_git_branch_after_merge = True, creates a
branch with client.branch.create(..., sync_with_git=True), then calls the
BranchDelete GraphQL mutation with input {"name": branch.name,
"delete_from_git": False} and patches WorkflowLocalExecution.submit_workflow to
AsyncMock; finally assert no calls were made to submit_workflow with workflow ==
GIT_REPOSITORIES_DELETE_BRANCH (i.e., deletion workflow is not triggered). Use
the existing patterns in TestDeleteBranchGitWorkflow, Mutation helper, and
mock_submit call inspection to locate and implement the test.
In `@backend/tests/integration/git/test_delete_git_branch_gogs.py`:
- Around line 32-37: The helper _delete_branch_on_gogs currently discards the
httpx response; capture the response from httpx.delete, check its status_code,
and surface failures: treat 200/204 (or 404 if tests tolerate missing branches)
as OK/ignored, but for any other status raise an error (or call
response.raise_for_status()) including the status and response text so Test 1
can fail on unexpected deletion errors; locate the call in function
_delete_branch_on_gogs and modify it to inspect the response rather than
dropping it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3535c4a9-fafe-4ff4-89df-5a71b62f24ce
📒 Files selected for processing (24)
backend/infrahub/config.pybackend/infrahub/core/branch/tasks.pybackend/infrahub/git/base.pybackend/infrahub/git/tasks.pybackend/infrahub/graphql/mutations/branch.pybackend/infrahub/proposed_change/tasks.pybackend/infrahub/workflows/catalogue.pybackend/tests/component/git/test_delete_git_branch.pybackend/tests/functional/branch/test_branch_delete_after_merge.pybackend/tests/functional/branch/test_delete_git_branch.pybackend/tests/functional/conftest.pybackend/tests/integration/git/conftest.pybackend/tests/integration/git/test_delete_git_branch_gogs.pychangelog/+bee22d82.added.mddev/specs/infp-389-branch-merge-delete/contracts/graphql-branch-delete.mddev/specs/infp-389-branch-merge-delete/contracts/rest-config-response.mddev/specs/infp-389-branch-merge-delete/contracts/workflow-git-branch-delete.mddev/specs/infp-389-branch-merge-delete/data-model.mddev/specs/infp-389-branch-merge-delete/plan.mddev/specs/infp-389-branch-merge-delete/research.mddev/specs/infp-389-branch-merge-delete/tasks.mddocs/docs/reference/configuration.mdxfrontend/app/src/shared/api/graphql/graphql-env.d.tsschema/schema.graphql
✅ Files skipped from review due to trivial changes (5)
- frontend/app/src/shared/api/graphql/graphql-env.d.ts
- docs/docs/reference/configuration.mdx
- dev/specs/infp-389-branch-merge-delete/plan.md
- dev/specs/infp-389-branch-merge-delete/contracts/rest-config-response.md
- dev/specs/infp-389-branch-merge-delete/data-model.md
🚧 Files skipped from review as they are similar to previous changes (2)
- dev/specs/infp-389-branch-merge-delete/contracts/workflow-git-branch-delete.md
- backend/infrahub/config.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Branches accumulate after merging, requiring manual cleanup. This PR adds optional automatic deletion of Infrahub branches (and optionally their linked Git branches) after a successful merge.
What changed
main.delete_branch_after_merge(defaultfalse): automatically deletes the Infrahub branch after a successful merge.git.delete_git_branch_after_merge(defaultfalse): additionally deletes the corresponding remote Git branch. Requiresmain.delete_branch_after_mergeto be enabled (validated at startup).BranchDeleteGraphQL mutation accepts a new optionaldelete_from_gitboolean flag, allowing callers to override the global Git-deletion setting per request.GIT_REPOSITORIES_DELETE_BRANCHworkflow that fans out branch deletion across allCoreRepositoryinstances.InfrahubRepositoryBasegainsorigin_has_branch()anddelete_remote_branch()methods used by the new workflow task.Config contract and GraphQL mutation interface are additive — no breaking changes. Existing
BranchDeletecallers withoutdelete_from_gitare unaffected.How to review
backend/infrahub/config.py— two new fields and the cross-field validator.backend/infrahub/core/branch/tasks.py— whereBRANCH_DELETEis submitted post-merge when the flag is set.backend/infrahub/graphql/mutations/branch.py— newBranchDeleteInputwithdelete_from_git.backend/infrahub/git/tasks.py+backend/infrahub/git/base.py— the new workflow/task and repository methods.backend/tests/component/git/,backend/tests/functional/branch/,backend/tests/integration/git/.The
schema/schema.graphqlandfrontend/app/src/shared/api/changes are generated — mechanical, low-risk.Risky area: the
delete_remote_branchmethod does agit push origin --deletefollowed by local ref cleanup. It silently skips repos where the branch doesn't exist on origin (origin_has_branchcheck), so fan-out failures in one repo are logged but don't block others.How to test
Enable the feature in config:
Merge a non-default branch and verify it is deleted from Infrahub (and from the remote Git repo if the git flag is set).
Impact & rollout
false— no behavior change on upgrade.main.delete_branch_after_merge,git.delete_git_branch_after_merge). Enablingdelete_git_branch_after_mergewithoutdelete_branch_after_mergeraises a startup error.Checklist
uv run towncrier create ...)Summary by CodeRabbit
New Features
Documentation
Tests