-
Notifications
You must be signed in to change notification settings - Fork 57
fix: scope branch-merge and proposed-change-review events to the default branch (closes #9761) #10422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: scope branch-merge and proposed-change-review events to the default branch (closes #9761) #10422
Changes from 10 commits
8d0b079
d480917
8199e31
78abe0c
0c2612f
d122260
97d3b7b
9f0543a
910d4ff
116eab2
9bae3f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,30 @@ | |
| from infrahub.database import InfrahubDatabase | ||
|
|
||
|
|
||
| def _build_dispatcher( | ||
| db: InfrahubDatabase, | ||
| source_branch: Branch, | ||
| destination_branch: Branch, | ||
| event_service: MemoryInfrahubEvent, | ||
| ) -> PostMergeDispatcher: | ||
| workflow = WorkflowLocalExecution() | ||
| return PostMergeDispatcher( | ||
| repository_merge_dispatcher=RepositoryMergeDispatcher( | ||
| db=db, source_branch=source_branch, destination_branch=destination_branch, workflow=workflow | ||
| ), | ||
| workflow=workflow, | ||
| event_service=event_service, | ||
| default_branch=destination_branch, | ||
| ) | ||
|
|
||
|
|
||
| def _context(default_branch: Branch) -> InfrahubContext: | ||
| return InfrahubContext.init( | ||
| branch=default_branch, | ||
| account=AccountSession(account_id=str(uuid4()), auth_type=AuthType.NONE), | ||
| ) | ||
|
|
||
|
|
||
| class TestPostMergeSchemaEvent: | ||
| """A merge that applied schema changes emits a scoped SchemaUpdatedEvent for the destination branch. | ||
|
|
||
|
|
@@ -29,30 +53,6 @@ class TestPostMergeSchemaEvent: | |
| elements the merge actually changed. | ||
| """ | ||
|
|
||
| def _build_dispatcher( | ||
| self, | ||
| db: InfrahubDatabase, | ||
| source_branch: Branch, | ||
| destination_branch: Branch, | ||
| event_service: MemoryInfrahubEvent, | ||
| ) -> PostMergeDispatcher: | ||
| workflow = WorkflowLocalExecution() | ||
| return PostMergeDispatcher( | ||
| repository_merge_dispatcher=RepositoryMergeDispatcher( | ||
| db=db, source_branch=source_branch, destination_branch=destination_branch, workflow=workflow | ||
| ), | ||
| workflow=workflow, | ||
| event_service=event_service, | ||
| default_branch=destination_branch, | ||
| global_branch=registry.get_global_branch(), | ||
| ) | ||
|
|
||
| def _context(self, default_branch: Branch) -> InfrahubContext: | ||
| return InfrahubContext.init( | ||
| branch=default_branch, | ||
| account=AccountSession(account_id=str(uuid4()), auth_type=AuthType.NONE), | ||
| ) | ||
|
|
||
| async def test_emits_scoped_schema_updated_event_when_schema_changed( | ||
| self, | ||
| db: InfrahubDatabase, | ||
|
|
@@ -62,7 +62,7 @@ async def test_emits_scoped_schema_updated_event_when_schema_changed( | |
| ) -> None: | ||
| source_branch = await create_branch(branch_name="feature", db=db) | ||
| memory_event = MemoryInfrahubEvent() | ||
| dispatcher = self._build_dispatcher(db, source_branch, default_branch, memory_event) | ||
| dispatcher = _build_dispatcher(db, source_branch, default_branch, memory_event) | ||
|
|
||
| # A schema change confined to a derived-value definition on the destination branch. | ||
| base_schema = registry.schema.get_schema_branch(name=default_branch.name) | ||
|
|
@@ -77,7 +77,7 @@ async def test_emits_scoped_schema_updated_event_when_schema_changed( | |
| branch=source_branch, | ||
| proposed_change_id=None, | ||
| node_events=[], | ||
| context=self._context(default_branch), | ||
| context=_context(default_branch), | ||
| schema_diff=schema_diff, | ||
| schema_hash=candidate.get_hash(), | ||
| ) | ||
|
|
@@ -99,15 +99,58 @@ async def test_no_schema_updated_event_when_no_schema_change( | |
| ) -> None: | ||
| source_branch = await create_branch(branch_name="feature", db=db) | ||
| memory_event = MemoryInfrahubEvent() | ||
| dispatcher = self._build_dispatcher(db, source_branch, default_branch, memory_event) | ||
| dispatcher = _build_dispatcher(db, source_branch, default_branch, memory_event) | ||
|
|
||
| await dispatcher.dispatch_events( | ||
| branch=source_branch, | ||
| proposed_change_id=None, | ||
| node_events=[], | ||
| context=self._context(default_branch), | ||
| context=_context(default_branch), | ||
| schema_diff=None, | ||
| ) | ||
|
|
||
| assert not [event for event in memory_event.events if isinstance(event, SchemaUpdatedEvent)] | ||
| assert [event for event in memory_event.events if isinstance(event, BranchMergedEvent)] | ||
|
|
||
|
|
||
| class TestPostMergeBranchMergedEvent: | ||
| """The branch-merged event is scoped to the default branch for webhook matching. | ||
|
|
||
| Webhook branch scoping matches the event's `infrahub.branch` related-resource label, so a | ||
| Default-Branch scoped webhook fires for a merge only when that label is the default branch. The | ||
| event payload still identifies the branch that was merged. | ||
| """ | ||
|
|
||
| async def test_branch_merged_event_scoped_to_default_branch( | ||
| self, | ||
| db: InfrahubDatabase, | ||
| default_branch: Branch, | ||
| register_core_models_schema: SchemaBranch, | ||
| car_person_schema: SchemaBranch, | ||
| ) -> None: | ||
| source_branch = await create_branch(branch_name="feature", db=db) | ||
| memory_event = MemoryInfrahubEvent() | ||
| dispatcher = _build_dispatcher(db, source_branch, default_branch, memory_event) | ||
|
|
||
| await dispatcher.dispatch_events( | ||
| branch=source_branch, | ||
| proposed_change_id=None, | ||
| node_events=[], | ||
| context=_context(default_branch), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes you're right, thanks @gmazoyer |
||
| schema_diff=None, | ||
| ) | ||
|
|
||
| merged_events = [event for event in memory_event.events if isinstance(event, BranchMergedEvent)] | ||
| assert len(merged_events) == 1 | ||
| event = merged_events[0] | ||
|
|
||
| # Payload identity keeps naming the branch that was merged. | ||
| assert event.branch_name == source_branch.name | ||
| assert event.branch_id == str(source_branch.get_uuid()) | ||
|
|
||
| # The webhook scoping branch is the default branch, not the global branch. | ||
| branch_related = [ | ||
| entry for entry in event.get_related() if entry.get("prefect.resource.role") == "infrahub.branch" | ||
| ] | ||
| assert len(branch_related) == 1 | ||
| assert branch_related[0]["infrahub.resource.label"] == default_branch.name | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Branch merge and proposed change review events are now emitted on the default branch, so webhooks scoped to the default branch reliably match them regardless of which branch triggered the change. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -158,6 +158,15 @@ The `EventMeta` class provides rich context: | |
|
|
||
| Use `EventMeta.from_parent()` to create child events that maintain hierarchy. | ||
|
|
||
| ## Scoping branch for webhook matching | ||
|
|
||
| Webhook branch scoping matches an event against `meta.context.branch` (see [Webhooks](webhooks.md)). Not every branch-agnostic event overrides the caller's context, so the scoping branch is set per event: | ||
|
|
||
| - Proposed change merge and review events (merged, approved, rejected, and the approval/rejection revoke variants) are stamped to the default branch, so scoping is independent of the branch the mutation ran on. | ||
| - `branch.merged` is stamped to the default branch as well, since the merge lands there. Its payload still carries the merged branch in `branch_name` / `branch_id`; only the scoping branch is the default one. | ||
| - `branch.created` and `branch.deleted` are stamped to the global branch, pending a general rule for branch-agnostic node events. | ||
| - `branch.rebased` and `branch.migrated` inherit the caller's context branch; they are not overridden. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder whether it would be relevant to assign the target branch for those operations |
||
|
|
||
| ## Querying Events | ||
|
|
||
| Events can be queried through: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracted as this is now reused in other test classes