Add proposed change event when multiple approvals are revoked #7033
Conversation
|
Warning Rate limit exceeded@gmazoyer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughAdds a new ProposedChangeApprovalsRevokedEvent (with reviewer_accounts map), an EventType constant, GraphQL type wiring, task-manager handling for the event group, re-exports the event, and documentation describing the new infrahub.proposed_change.approvals_revoked event. Changes
Sequence Diagram(s)sequenceDiagram
participant Producer as ProposedChange source
participant Events as Infrahub Events
participant Bus as Event Bus / Handlers
participant TaskMgr as Task Manager
Producer->>Events: Create ProposedChangeApprovalsRevokedEvent(reviewer_accounts, payload)
Events->>Events: get_related() → per-reviewer related node entries
Events->>Bus: Publish infrahub.proposed_change.approvals_revoked
Bus->>TaskMgr: Handle event
TaskMgr->>TaskMgr: Map to proposed-change group (use _return_proposed_change_event)
TaskMgr->>Bus: (optional) dispatch processed/enriched payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
group/backend
CodSpeed Performance ReportMerging #7033 will not alter performanceComparing Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/infrahub/events/proposed_change_action.py (2)
50-52: Optional reviewer fields: good change; clarify descriptions to reflect optionalityThis aligns with the PR objective. Please make the optionality explicit in the field descriptions to avoid ambiguity for downstream consumers and docs.
Apply this diff to refine the descriptions:
- reviewer_account_id: str | None = Field(None, description="The ID of the user who reviewed the proposed change") - reviewer_account_name: str | None = Field(None, description="The name of the user who reviewed the proposed change") - reviewer_former_decision: str | None = Field(None, description="The former decision made by the reviewer") + reviewer_account_id: str | None = Field( + None, description="Optional. The ID of the reviewer when the revocation is tied to a user action." + ) + reviewer_account_name: str | None = Field( + None, description="Optional. The name of the reviewer when the revocation is tied to a user action." + ) + reviewer_former_decision: str | None = Field( + None, description="Optional. The previous decision that was revoked (for example: approved, rejected)." + )
54-66: Document get_related behavior and tighten semantics per coding guidelinesCoding guidelines require docstrings for all functions. Add a concise docstring that explains when reviewer-related resources are included.
Apply this diff to add a Google-style docstring:
def get_related(self) -> list[dict[str, str]]: + """ + Build the list of related resources for this event. + + Returns: + A list of related resources. The reviewer account resource is included + only when both reviewer_account_id and reviewer_account_name are set. + """ related = super().get_related() if self.reviewer_account_id and self.reviewer_account_name: related.append( { "prefect.resource.id": self.reviewer_account_id, "prefect.resource.role": "infrahub.related.node", "infrahub.node.kind": InfrahubKind.GENERICACCOUNT, "infrahub.node.id": self.reviewer_account_id, "infrahub.reviewer.account.name": self.reviewer_account_name, } ) return related
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/infrahub/events/proposed_change_action.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/events/proposed_change_action.py
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use type hints for all function parameters and return values in Python files
Use async/await whenever possible in Python code
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
All Python functions must have docstrings
Always use triple quotes (""") for docstrings in Python
Follow Google-style docstring format in Python files
Include sections in Python docstrings when applicable: Brief one-line description, Detailed description, Args/Parameters (without typing), Returns, Raises, Examples
Use ruff and mypy for type checking and validations in Python files
Format all Python files usingpoetry run invoke format
Validate Python formatting and linting usingpoetry run invoke lint
Files:
backend/infrahub/events/proposed_change_action.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E-testing-version-upgrade / From 1.1.0
- GitHub Check: backend-benchmark
- GitHub Check: backend-docker-integration
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-tests-unit
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-integration
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/infrahub/events/proposed_change_action.py (1)
56-65: Consider emitting related reviewer when only ID is known; make name optional in payloadCurrently the related node is only emitted when both ID and name are present. Emitting the related node when the ID is present (and adding the name only if available) preserves a stable relationship graph while still supporting automated revocations. Downstream code typically uses
.get(...)on the name.Apply this diff:
- if self.reviewer_account_id and self.reviewer_account_name: - related.append( - { - "prefect.resource.id": self.reviewer_account_id, - "prefect.resource.role": "infrahub.related.node", - "infrahub.node.kind": InfrahubKind.GENERICACCOUNT, - "infrahub.node.id": self.reviewer_account_id, - "infrahub.reviewer.account.name": self.reviewer_account_name, - } - ) + if self.reviewer_account_id: + payload = { + "prefect.resource.id": self.reviewer_account_id, + "prefect.resource.role": "infrahub.related.node", + "infrahub.node.kind": InfrahubKind.GENERICACCOUNT, + "infrahub.node.id": self.reviewer_account_id, + } + if self.reviewer_account_name: + payload["infrahub.reviewer.account.name"] = self.reviewer_account_name + related.append(payload)
🧹 Nitpick comments (3)
backend/infrahub/events/proposed_change_action.py (2)
50-52: Optional reviewer fields: correct direction; add a class docstring per guidelinesThe optionalization aligns with the PR objective. Please add a class docstring to explain the semantics (automated revocations, optional fields), per our docstring policy.
Apply this diff:
class ProposedChangeReviewRevokedEvent(ProposedChangeEvent): + """Event generated when a proposed change review decision has been revoked. + + This event supports automated revocations where reviewer information may be absent. + + Attributes: + reviewer_account_id: The ID of the user who reviewed the proposed change (optional). + reviewer_account_name: The name of the user who reviewed the proposed change (optional). + reviewer_former_decision: The former decision made by the reviewer (optional). + """ reviewer_account_id: str | None = Field(None, description="The ID of the user who reviewed the proposed change") reviewer_account_name: str | None = Field(None, description="The name of the user who reviewed the proposed change") reviewer_former_decision: str | None = Field(None, description="The former decision made by the reviewer")
68-72: Add docstring toget_resourceand verify locallyThe docstring you’ve added clearly explains the purpose and behavior of
get_resourceand meets our guidelines for optional fields.Please ensure everything passes the standard backend checks by running locally:
invoke format --checkinvoke lintmypypytest -qbackend/infrahub/graphql/types/event.py (1)
133-136: Make descriptions reflect optional semantics and “former” decisionThe fields were correctly made optional. Please adjust descriptions to reflect optionality (automated revocations) and fix the “former decision” wording to match the Python model.
Apply this diff:
- reviewer_account_id = String(required=False, description="The ID of the user who reviewed the proposed change") - reviewer_account_name = String(required=False, description="The name of the user who reviewed the proposed change") - reviewer_former_decision = String(required=False, description="The decision made by the reviewer") + reviewer_account_id = String( + required=False, + description="The ID of the user who reviewed the proposed change (optional for automated revocations)", + ) + reviewer_account_name = String( + required=False, + description="The name of the user who reviewed the proposed change (optional for automated revocations)", + ) + reviewer_former_decision = String( + required=False, + description="The former decision made by the reviewer (optional)", + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/infrahub/events/proposed_change_action.py(1 hunks)backend/infrahub/graphql/types/event.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/graphql/types/event.pybackend/infrahub/events/proposed_change_action.py
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use type hints for all function parameters and return values in Python files
Use async/await whenever possible in Python code
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
All Python functions must have docstrings
Always use triple quotes (""") for docstrings in Python
Follow Google-style docstring format in Python files
Include sections in Python docstrings when applicable: Brief one-line description, Detailed description, Args/Parameters (without typing), Returns, Raises, Examples
Use ruff and mypy for type checking and validations in Python files
Format all Python files usingpoetry run invoke format
Validate Python formatting and linting usingpoetry run invoke lint
Files:
backend/infrahub/graphql/types/event.pybackend/infrahub/events/proposed_change_action.py
🧬 Code Graph Analysis (1)
backend/infrahub/graphql/types/event.py (1)
backend/infrahub/core/attribute.py (2)
String(679-681)description(771-778)
⏰ 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). (4)
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-integration
- GitHub Check: Cloudflare Pages
gmazoyer
force-pushed
the
gma-20250813-support-ihp-22
branch
from
5a565f0 to
7e7fa10
Compare
|
Wouldn't we need to have that info populated in order to see who's approval that got revoked? I think we should have that info as we are revoking the previous approval? |
gmazoyer
force-pushed
the
gma-20250813-support-ihp-22
branch
from
7e7fa10 to
a72726a
Compare
Do you mean that we should raise one event per revoked approval? E.g. if a proposed change has been approved 5 times, raise 5 approval revoked events. |
gmazoyer
force-pushed
the
gma-20250813-support-ihp-22
branch
from
a72726a to
ebe84e7
Compare
gmazoyer
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/infrahub/events/proposed_change_action.py (2)
153-177: Confirm this satisfies the PR objective; old revoke events still require reviewer fieldsThe PR objective states reviewer info becomes optional for events based on ProposedChangeReviewRevokedEvent. This new aggregated event helps for bulk revocations but the singular ProposedChangeReviewRevokedEvent still requires reviewer fields. If the goal is to allow omission there as well (e.g., for automated revocations), we should update that class too in a follow-up.
I can prepare a follow-up patch to make reviewer fields optional in ProposedChangeReviewRevokedEvent (and adjust get_related/get_resource accordingly) and update docs/GraphQL schema. Should I proceed?
153-177: Add unit tests for validation and related generationPlease add tests to cover:
- successful creation with both lists provided and equal length
- omission of both lists
- error when only one list is provided
- error when lengths differ
- get_related returns only proposed change when lists omitted
- get_related includes per-reviewer nodes and includes name only when present
I can scaffold tests under backend/tests/events/test_proposed_change_action.py to align with pytest. Want me to open a draft with test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
backend/infrahub/core/constants/__init__.py(1 hunks)backend/infrahub/events/__init__.py(2 hunks)backend/infrahub/events/proposed_change_action.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/core/constants/__init__.pybackend/infrahub/events/__init__.pybackend/infrahub/events/proposed_change_action.py
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use type hints for all function parameters and return values in Python files
Use async/await whenever possible in Python code
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
All Python functions must have docstrings
Always use triple quotes (""") for docstrings in Python
Follow Google-style docstring format in Python files
Include sections in Python docstrings when applicable: Brief one-line description, Detailed description, Args/Parameters (without typing), Returns, Raises, Examples
Use ruff and mypy for type checking and validations in Python files
Format all Python files usingpoetry run invoke format
Validate Python formatting and linting usingpoetry run invoke lint
Files:
backend/infrahub/core/constants/__init__.pybackend/infrahub/events/__init__.pybackend/infrahub/events/proposed_change_action.py
🧬 Code Graph Analysis (1)
backend/infrahub/events/__init__.py (1)
backend/infrahub/events/proposed_change_action.py (1)
ProposedChangeApprovalsRevokedEvent(153-177)
⏰ 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). (6)
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-tests-functional
- GitHub Check: backend-benchmark
- GitHub Check: backend-tests-integration
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-tests-unit
🔇 Additional comments (4)
backend/infrahub/events/__init__.py (1)
8-8: Public re-export present — please update docs & subscription registriesImport and all for ProposedChangeApprovalsRevokedEvent are present; quick scan shows the class and constant exist. Please ensure docs and any event-subscription registries are updated to include this new event.
Files found by the scan:
- backend/infrahub/events/init.py — imports and all include ProposedChangeApprovalsRevokedEvent
- backend/infrahub/events/proposed_change_action.py — defines class ProposedChangeApprovalsRevokedEvent (event_name = "...proposed_change.approvals_revoked")
- backend/infrahub/core/constants/init.py — defines PROPOSED_CHANGE_APPROVALS_REVOKED
No other references were found by this quick scan; if docs or registries live elsewhere, please update them accordingly.
backend/infrahub/events/proposed_change_action.py (3)
1-1: Import of Self is appropriate for Pydantic v2 validatorsThis is correct usage for annotating validator return types.
3-3: pydantic imports appropriate for new validator and fieldsNo issues here.
165-177: Do not make reviewer names optional — model and consumers require themThe repository enforces reviewer names and downstream code expects them, so the suggested change (zip + optional names) is incorrect.
- backend/infrahub/events/proposed_change_action.py — ProposedChangeApprovalsRevokedEvent defines reviewer_account_ids and reviewer_account_names as required (Field(...)), validates equal length in model_validator, and the get_related() loop indexes into reviewer_account_names (lines ~154–177).
- backend/infrahub/task_manager/event.py — event mapping reads "infrahub.reviewer.account.name" into reviewer_account_name (lines ~200–206) and expects it to be present.
- backend/infrahub/graphql/types/event.py — reviewer_account_name is declared required in the GraphQL types.
- docs/docs/reference/infrahub-events.mdx — documentation lists these fields as required.
Action: ignore the proposed diff that omits names. If you intend to make names optional, update the Pydantic model (allow optional names), remove/adjust the length validator, and update all consumers (task_manager, GraphQL schema, docs) to tolerate missing names.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/infrahub/task_manager/event.py (1)
4-4: Drop unused defaultdict import if refactor is appliedIf you adopt the refactor to return a plain dict, this import becomes unused and should be removed.
-from collections import defaultdict
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/infrahub/graphql/types/event.py(2 hunks)backend/infrahub/task_manager/event.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/task_manager/event.pybackend/infrahub/graphql/types/event.py
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use type hints for all function parameters and return values in Python files
Use async/await whenever possible in Python code
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
All Python functions must have docstrings
Always use triple quotes (""") for docstrings in Python
Follow Google-style docstring format in Python files
Include sections in Python docstrings when applicable: Brief one-line description, Detailed description, Args/Parameters (without typing), Returns, Raises, Examples
Use ruff and mypy for type checking and validations in Python files
Format all Python files usingpoetry run invoke format
Validate Python formatting and linting usingpoetry run invoke lint
Files:
backend/infrahub/task_manager/event.pybackend/infrahub/graphql/types/event.py
🧬 Code Graph Analysis (2)
backend/infrahub/task_manager/event.py (4)
backend/infrahub/types.py (1)
Any(312-319)backend/infrahub/core/task/task.py (1)
related(31-34)backend/infrahub/core/relationship/model.py (1)
append(629-641)backend/infrahub/computed_attribute/models.py (1)
get(62-65)
backend/infrahub/graphql/types/event.py (1)
backend/infrahub/events/proposed_change_action.py (1)
ProposedChangeApprovalsRevokedEvent(153-177)
⏰ 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). (6)
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-validate-generated
- GitHub Check: backend-tests-integration
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-unit
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
backend/infrahub/task_manager/event.py (1)
255-259: LGTM: approvals_revoked specifics merged correctlyMerging the aggregated reviewers payload alongside the standard proposed change data for the approvals_revoked event is the right approach and consistent with existing patterns.
backend/infrahub/graphql/types/event.py (2)
139-150: LGTM: New GraphQL type models multiple revoked approvals clearlyThe new fields (pluralized, non-null lists of non-null strings) are well named and documented. This aligns with the event’s aggregated semantics.
236-236: LGTM: EVENT_TYPES mapping updatedThe new event is properly registered, ensuring correct type resolution.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/infrahub/events/proposed_change_action.py (2)
163-167: Document validator intent; optionally allow omission semantics if required laterCurrent validator correctly enforces equal lengths. Add a docstring to clarify the intent. If you later decide to allow IDs without names (per review discussion), you’ll need to relax this validator in tandem with type changes.
Apply this minimal documentation diff:
@model_validator(mode="after") def check_same_length(self) -> Self: + """Ensure reviewer_account_ids and reviewer_account_names are aligned. + + Raises: + ValueError: If the two lists have different lengths. + """ if len(self.reviewer_account_ids) != len(self.reviewer_account_names): raise ValueError("reviewer_account_ids and reviewer_account_names must have the same number of items") return selfAlternative (if you choose to allow omission of names/IDs as a pair, consistent with earlier feedback): make both fields optional and validate paired presence or omission.
-class ProposedChangeApprovalsRevokedEvent(ProposedChangeEvent): - reviewer_account_ids: list[str] = Field( - default_factory=list, description="IDs of accounts whose approval was revoked" - ) - reviewer_account_names: list[str] = Field( - default_factory=list, description="Names of accounts whose approval was revoked" - ) +class ProposedChangeApprovalsRevokedEvent(ProposedChangeEvent): + reviewer_account_ids: list[str] | None = Field( + default=None, description="IDs of accounts whose approval was revoked (optional)" + ) + reviewer_account_names: list[str] | None = Field( + default=None, description="Names of accounts whose approval were revoked (optional)" + ) @@ - @model_validator(mode="after") - def check_same_length(self) -> Self: - if len(self.reviewer_account_ids) != len(self.reviewer_account_names): - raise ValueError("reviewer_account_ids and reviewer_account_names must have the same number of items") - return self + @model_validator(mode="after") + def check_same_length(self) -> Self: + """Allow omission of both fields, enforce paired presence and equal lengths when provided.""" + if self.reviewer_account_ids is None and self.reviewer_account_names is None: + return self + if (self.reviewer_account_ids is None) != (self.reviewer_account_names is None): + raise ValueError("reviewer_account_ids and reviewer_account_names must be provided together or omitted") + assert self.reviewer_account_ids is not None and self.reviewer_account_names is not None + if len(self.reviewer_account_ids) != len(self.reviewer_account_names): + raise ValueError("reviewer_account_ids and reviewer_account_names must have the same number of items") + return selfNote: adopting the alternative requires updating GraphQL schema (backend/infrahub/graphql/types/event.py) to make both fields optional. I can prepare that follow-up if you want.
153-162: Add a class docstring to meet repository docstring requirementsPer coding guidelines, classes/methods must have docstrings. Please document the event’s purpose and field semantics (including the aggregation behavior and that lists may be empty).
Apply this diff:
-class ProposedChangeApprovalsRevokedEvent(ProposedChangeEvent): +class ProposedChangeApprovalsRevokedEvent(ProposedChangeEvent): + """Event generated when one or more existing approvals on a proposed change are revoked. + + Detailed description: + This event aggregates multiple approval revocations that happen within a single transaction. + Reviewer identity details may not always be available; in such cases, the lists can be empty. + + Args: + reviewer_account_ids: IDs of accounts whose approval was revoked. Can be empty when unknown. + reviewer_account_names: Names corresponding to reviewer_account_ids. Must align in order with IDs. + """ reviewer_account_ids: list[str] = Field( default_factory=list, description="IDs of accounts whose approval was revoked" ) reviewer_account_names: list[str] = Field( default_factory=list, description="Names of accounts whose approval was revoked" )If you want, I can also add a brief docstring to each new method in this class and wire up unit tests for validator behavior and related-node emission.
🧹 Nitpick comments (2)
backend/infrahub/events/proposed_change_action.py (2)
169-181: Make related-node generation robust to missing names and simplify with zip()Even if names are present today, making the name attribute optional in the related payload improves resilience and mirrors how single-reviewer events are evolving. Also, add a docstring for compliance and readability.
Apply this diff:
- def get_related(self) -> list[dict[str, str]]: - related = super().get_related() - for i, account_id in enumerate(self.reviewer_account_ids): - related.append( - { - "prefect.resource.id": account_id, - "prefect.resource.role": "infrahub.related.node", - "infrahub.node.kind": InfrahubKind.GENERICACCOUNT, - "infrahub.node.id": account_id, - "infrahub.reviewer.account.name": self.reviewer_account_names[i], - } - ) - return related + def get_related(self) -> list[dict[str, str]]: + """Build related resources for revoked reviewers. + + Returns: + A list of related node descriptors, one entry per revoked reviewer. + """ + related = super().get_related() + for account_id, account_name in zip(self.reviewer_account_ids, self.reviewer_account_names): + payload = { + "prefect.resource.id": account_id, + "prefect.resource.role": "infrahub.related.node", + "infrahub.node.kind": InfrahubKind.GENERICACCOUNT, + "infrahub.node.id": account_id, + } + if account_name: + payload["infrahub.reviewer.account.name"] = account_name + related.append(payload) + return related
153-159: Consider a mapping (id -> name) instead of two parallel listsTwo parallel lists require length/ordering invariants and are error-prone for producers/consumers. A dict[str, str] better expresses the pairing and simplifies validation and usage, especially for payload consumers (as noted by @ogenstad).
If you choose this model, update GraphQL accordingly (e.g., expose a list of objects with id/name or a key-value map). Suggested change here:
-class ProposedChangeApprovalsRevokedEvent(ProposedChangeEvent): - reviewer_account_ids: list[str] = Field( - default_factory=list, description="IDs of accounts whose approval was revoked" - ) - reviewer_account_names: list[str] = Field( - default_factory=list, description="Names of accounts whose approval was revoked" - ) +class ProposedChangeApprovalsRevokedEvent(ProposedChangeEvent): + reviewers: dict[str, str] = Field( + default_factory=dict, + description="Mapping of reviewer account ID -> reviewer account name for approvals that were revoked", + ) @@ - @model_validator(mode="after") - def check_same_length(self) -> Self: - if len(self.reviewer_account_ids) != len(self.reviewer_account_names): - raise ValueError("reviewer_account_ids and reviewer_account_names must have the same number of items") - return self + @model_validator(mode="after") + def check_same_length(self) -> Self: + """No-op for mapping-based representation; reserved for future constraints.""" + return self @@ - def get_related(self) -> list[dict[str, str]]: - related = super().get_related() - for i, account_id in enumerate(self.reviewer_account_ids): - related.append( - { - "prefect.resource.id": account_id, - "prefect.resource.role": "infrahub.related.node", - "infrahub.node.kind": InfrahubKind.GENERICACCOUNT, - "infrahub.node.id": account_id, - "infrahub.reviewer.account.name": self.reviewer_account_names[i], - } - ) - return related + def get_related(self) -> list[dict[str, str]]: + related = super().get_related() + for account_id, account_name in self.reviewers.items(): + payload = { + "prefect.resource.id": account_id, + "prefect.resource.role": "infrahub.related.node", + "infrahub.node.kind": InfrahubKind.GENERICACCOUNT, + "infrahub.node.id": account_id, + } + if account_name: + payload["infrahub.reviewer.account.name"] = account_name + related.append(payload) + return relatedHappy to prep the corresponding GraphQL schema update and event consumer adjustments if you want to take this route.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/infrahub/events/proposed_change_action.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/events/proposed_change_action.py
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use type hints for all function parameters and return values in Python files
Use async/await whenever possible in Python code
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
All Python functions must have docstrings
Always use triple quotes (""") for docstrings in Python
Follow Google-style docstring format in Python files
Include sections in Python docstrings when applicable: Brief one-line description, Detailed description, Args/Parameters (without typing), Returns, Raises, Examples
Use ruff and mypy for type checking and validations in Python files
Format all Python files usingpoetry run invoke format
Validate Python formatting and linting usingpoetry run invoke lint
Files:
backend/infrahub/events/proposed_change_action.py
🧬 Code Graph Analysis (1)
backend/infrahub/events/proposed_change_action.py (1)
backend/infrahub/graphql/types/event.py (1)
ProposedChangeApprovalsRevokedEvent(139-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: E2E-testing-version-upgrade / From 1.1.0
- GitHub Check: backend-benchmark
- GitHub Check: E2E-testing-playwright
- GitHub Check: validate-generated-documentation
- GitHub Check: backend-validate-generated
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-unit
- GitHub Check: backend-tests-integration
🔇 Additional comments (2)
backend/infrahub/events/proposed_change_action.py (2)
161-162: LGTM: event_name and placement align with PR objectivesThe event is correctly named and namespaced, and the class is placed alongside related proposed change events for discoverability.
3-3: Pydantic v2 confirmed — no action requiredpyproject.toml files declare Pydantic v2 (examples found:
pydantic = "~2.10",pydantic = "^2.10.6") andpydantic-settings = "~2.8". Usingmodel_validator(Pydantic v2) is safe across the repo.
- Matches observed:
- pydantic = "~2.10"
- pydantic-settings = "~2.8"
- pydantic = "^2.10.6"
github-actions
bot
added
the
type/documentation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/infrahub/graphql/types/event.py (1)
139-145: Expose reviewer_accounts on the GraphQL type for parity with the event payload/docsRight now this type only exposes payload. Given the Python event carries reviewer_accounts and the docs list it explicitly, exposing it here avoids forcing clients to unpack payload to get core fields.
Apply this diff to add the field:
class ProposedChangeApprovalsRevokedEvent(ObjectType): class Meta: interfaces = (EventNodeInterface,) - payload = Field(GenericScalar, required=True) + reviewer_accounts = Field( + GenericScalar, + required=True, + description="ID to name map of accounts whose approval was revoked", + ) + payload = Field(GenericScalar, required=True)Note: Per backend/infrahub/events/proposed_change_action.py, the event dataclass exposes reviewer_accounts, so this should be present on the top-level event dict similarly to other Proposed Change review events. If not, we can wire a resolver to derive it from payload or related_nodes. Do you want me to provide that fallback?
docs/docs/reference/infrahub-events.mdx (1)
372-398: Add a concise Description to match the style of other event sectionsOther events in this doc include a Description line. Add one here to clarify the multi-revocation semantics and distinguish it from the singular approval_revoked event.
Apply this diff:
### Proposed Change Approvals Revoked Event **Type**: infrahub.proposed_change.approvals_revoked +**Description**: Event generated when multiple approvals have been revoked in a single transaction. For a single approval revocation, see `infrahub.proposed_change.approval_revoked`. <!-- vale off --> **Uses node_kind filter for webhooks**: `false` <!-- vale on -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/infrahub/events/proposed_change_action.py(1 hunks)backend/infrahub/graphql/types/event.py(2 hunks)backend/infrahub/task_manager/event.py(3 hunks)docs/docs/reference/infrahub-events.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/infrahub/task_manager/event.py
- backend/infrahub/events/proposed_change_action.py
🧰 Additional context used
📓 Path-based instructions (7)
backend/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Run backend tests with
pytestor viainvoketasks
Files:
backend/infrahub/graphql/types/event.py
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use type hints for all function parameters and return values in Python files
Use async/await whenever possible in Python code
Useasync deffor asynchronous functions in Python
Useawaitfor asynchronous calls in Python
Use Pydantic models for dataclasses in Python
All Python functions must have docstrings
Always use triple quotes (""") for docstrings in Python
Follow Google-style docstring format in Python files
Include sections in Python docstrings when applicable: Brief one-line description, Detailed description, Args/Parameters (without typing), Returns, Raises, Examples
Use ruff and mypy for type checking and validations in Python files
Format all Python files usingpoetry run invoke format
Validate Python formatting and linting usingpoetry run invoke lint
Files:
backend/infrahub/graphql/types/event.py
docs/docs/reference/**/*.{md,mdx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Write reference documentation in
docs/docs/reference/
Files:
docs/docs/reference/infrahub-events.mdx
docs/**/*.mdx
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Use Docusaurus markdown/MDX features in documentation
Files:
docs/docs/reference/infrahub-events.mdx
docs/**/*
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Develop documentation in
docs/, preview withinvoke docs.build docs.serve, and validate withinvoke docs.validate
Files:
docs/docs/reference/infrahub-events.mdx
**/*.{md,mdx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{md,mdx}: Lint markdown files using markdownlint with.markdownlint.yamlconfiguration
Documentation content must be accurate, reflect the latest version of Infrahub, and have correct markdown formatting, spelling, and grammar
Files:
docs/docs/reference/infrahub-events.mdx
**/*.mdx
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.mdx: Documentation in MDX files should follow the Diataxis framework structure (Tutorials, How-to guides, Explanation, Reference)
Documentation in MDX files should use a professional but approachable tone, avoid jargon unless defined, and use plain language with technical precision
Documentation in MDX files should be concise and direct, preferring short, active sentences and reducing fluff
Documentation in MDX files should be informative over promotional, focusing on explaining how and why, not on marketing
Documentation in MDX files should be consistent and structured, following a predictable pattern across sections and documents
How-to guides in MDX files should use conditional imperatives and focus on practical tasks and problems, addressing the user directly with imperative verbs
Topics in MDX files should use a discursive, reflective tone, include context and rationale, make connections between concepts, and present alternative perspectives
Always define new terms when first used in documentation, using callouts or glossary links if possible
Prefer domain-relevant language that reflects the user's perspective and be consistent with Infrahub's data model and UI naming conventions in documentation
Guides in documentation must address a specific, practical problem or task, with clear titles, logical steps, and validation steps
Topics in documentation must provide genuine understanding, include background and context, make connections to related concepts, and remain focused on explanation
Files:
docs/docs/reference/infrahub-events.mdx
🧬 Code Graph Analysis (1)
backend/infrahub/graphql/types/event.py (1)
backend/infrahub/events/proposed_change_action.py (1)
ProposedChangeApprovalsRevokedEvent(153-172)
⏰ 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). (11)
- GitHub Check: E2E-testing-version-upgrade / From 1.1.0
- GitHub Check: validate-generated-documentation
- GitHub Check: E2E-testing-playwright
- GitHub Check: backend-tests-functional
- GitHub Check: backend-validate-generated
- GitHub Check: backend-tests-unit
- GitHub Check: backend-tests-integration
- GitHub Check: backend-benchmark
- GitHub Check: documentation
- GitHub Check: backend-docker-integration
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
backend/infrahub/graphql/types/event.py (1)
230-231: LGTM — ProposedChangeApprovalsRevokedEvent is defined, exported and correctly wiredConfirmed: the event class, its event_name, public export, and GraphQL mapping are present.
- backend/infrahub/events/proposed_change_action.py — class ProposedChangeApprovalsRevokedEvent (≈line 153) with event_name = f"{EVENT_NAMESPACE}.proposed_change.approvals_revoked"
- backend/infrahub/events/init.py — ProposedChangeApprovalsRevokedEvent is imported and included in all
- backend/infrahub/graphql/types/event.py — EVENT_TYPES maps events.ProposedChangeApprovalsRevokedEvent.event_name to ProposedChangeApprovalsRevokedEvent (≈line 230)
- backend/infrahub/core/constants/init.py — PROPOSED_CHANGE_APPROVALS_REVOKED constant defined
- backend/infrahub/task_manager/event.py — case handling for "infrahub.proposed_change.approvals_revoked"
- docs/docs/reference/infrahub-events.mdx — docs reference infrahub.proposed_change.approvals_revoked
The original mapping is correct and resolve_type will return the intended GraphQL type.
gmazoyer
force-pushed
the
gma-20250813-support-ihp-22
branch
from
21148c9 to
8afb6fa
Compare
This change adds a new event to be raised when multiple approvals were revoked during a single transaction.
Summary by CodeRabbit
New Features
Improvements
Chores