Conversation
WalkthroughThis change introduces a new "Version Selector" check for deployment versions, implemented across both the frontend and backend. On the backend, the previously monolithic deployment version checks router is split into modular routers for approvals, deny windows, and version selectors, each with their own files and utility helpers. The version selector logic evaluates deployment versions against policy-based selectors within environments. The frontend adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as VersionSelectorCheck (React)
participant API as TRPC API (versionSelector)
participant DB as Database
UI->>API: useQuery({ versionId, environmentId })
API->>DB: Fetch deployment version metadata
API->>DB: Fetch environment and system
API->>DB: Find release target for deployment/environment
API->>DB: Fetch applicable policies for release target
API->>API: Merge policies, check for version selector
alt Selector exists
API->>DB: Query version matches selector
API-->>UI: Return true/false
else No selector
API-->>UI: Return true
end
UI->>UI: Render status (loading, passing, failing)
Suggested reviewers
Poem
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/api/src/router/deployment-version-checks/utils.ts (1)
58-76: Consider adding more specific error messageThe function correctly fetches and returns release target data with added workspace ID. However, the error message could be more specific by including the workspace ID in the message.
- message: `Release target not found: ${deploymentId} ${environmentId}`, + message: `Release target not found for deployment ${deploymentId} in environment ${environmentId} for workspace ${workspaceId}`,packages/api/src/router/deployment-version-checks/approvals.ts (1)
67-82: Consider using a more specific permission for approval mutations.The authorization check is using
DeploymentVersionGetpermission, which is typically used for read operations. For a mutation that adds an approval record, a more specific permission likeDeploymentVersionApprovemight be more appropriate if it exists in your permission model.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/VersionSelector.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/nodes/EnvironmentNode.tsx(2 hunks)packages/api/src/router/deployment-version-checks.ts(0 hunks)packages/api/src/router/deployment-version-checks/approvals.ts(1 hunks)packages/api/src/router/deployment-version-checks/deny-window.ts(1 hunks)packages/api/src/router/deployment-version-checks/router.ts(1 hunks)packages/api/src/router/deployment-version-checks/utils.ts(1 hunks)packages/api/src/router/deployment-version-checks/version-selector.ts(1 hunks)packages/api/src/router/deployment-version.ts(1 hunks)packages/api/src/router/policy.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/api/src/router/deployment-version-checks.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/nodes/EnvironmentNode.tsxpackages/api/src/router/deployment-version.tspackages/api/src/router/deployment-version-checks/deny-window.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/VersionSelector.tsxpackages/api/src/router/policy.tspackages/api/src/router/deployment-version-checks/approvals.tspackages/api/src/router/deployment-version-checks/version-selector.tspackages/api/src/router/deployment-version-checks/router.tspackages/api/src/router/deployment-version-checks/utils.ts
🧬 Code Graph Analysis (6)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/nodes/EnvironmentNode.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/VersionSelector.tsx (1)
VersionSelectorCheck(4-35)
packages/api/src/router/deployment-version-checks/deny-window.ts (1)
packages/api/src/trpc.ts (2)
createTRPCRouter(55-55)protectedProcedure(173-173)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/VersionSelector.tsx (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/StatusIcons.tsx (3)
Loading(28-32)Passing(5-9)Failing(11-15)
packages/api/src/router/policy.ts (2)
packages/rule-engine/src/db/update-policy.ts (1)
updatePolicyInTx(139-192)packages/events/src/index.ts (1)
getQueue(28-34)
packages/api/src/router/deployment-version-checks/approvals.ts (7)
packages/api/src/trpc.ts (2)
createTRPCRouter(55-55)protectedProcedure(173-173)packages/api/src/router/deployment-version-checks/utils.ts (3)
getVersionWithMetadata(44-56)getAnyReleaseTargetForDeploymentAndEnvironment(58-76)getApplicablePoliciesWithoutResourceScope(7-42)packages/db/src/schema/release.ts (1)
releaseTarget(20-42)packages/rule-engine/src/utils/merge-policies.ts (1)
mergePolicies(29-49)packages/rule-engine/src/manager/version-manager.ts (1)
VersionReleaseManager(37-212)packages/rule-engine/src/manager/version-manager-rules.ts (1)
getVersionApprovalRules(61-67)packages/events/src/index.ts (1)
getQueue(28-34)
packages/api/src/router/deployment-version-checks/router.ts (3)
packages/api/src/trpc.ts (2)
createTRPCRouter(55-55)protectedProcedure(173-173)packages/api/src/router/deployment-version-checks/approvals.ts (1)
approvalRouter(20-126)packages/api/src/router/deployment-version-checks/deny-window.ts (1)
denyWindowRouter(7-24)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (23)
packages/api/src/router/deployment-version-checks/utils.ts (2)
7-42: Well-structured database query function with clear return typeThe
getApplicablePoliciesWithoutResourceScopefunction correctly retrieves policies linked to a release target without resource selectors. The query is well-structured with proper joins and where clauses.
44-56: Good error handling for missing versionsThe
getVersionWithMetadatafunction appropriately throws a TRPC error when a version isn't found, which will translate to a proper HTTP response. The metadata transformation from an array to an object is clean and effective.packages/api/src/router/deployment-version.ts (1)
48-48: Import path updated to reflect new modular structureThis change correctly updates the import path to use the new modular router structure, pointing to the specific router file.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/nodes/EnvironmentNode.tsx (2)
12-12: Clean import of the new VersionSelectorCheck componentThe import is correctly added, maintaining the pattern of other similar check components.
39-39: Good integration of the VersionSelectorCheck componentThe VersionSelectorCheck component is properly added to the UI alongside existing check components, and all required props are passed through using the spread syntax.
packages/api/src/router/policy.ts (2)
19-19: New import for event queue integration.Added the Channel and getQueue imports from the events module, which will be used to enqueue policy updates for asynchronous processing.
298-304: Implementation of event-driven policy updates.The update mutation now not only updates the policy in the database but also enqueues the updated policy object into a message queue for asynchronous processing. This is an important improvement that enables other parts of the system to react to policy changes through the event queue.
This approach is more maintainable and follows better separation of concerns by:
- Keeping the transaction focused on data persistence
- Delegating downstream effects to event handlers via the queue
- Supporting eventual consistency throughout the system
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(raw)/releases/[releaseId]/checks/_components/flow-diagram/checks/VersionSelector.tsx (6)
1-3: Clear import structure for the component.Appropriate imports for the API client and status icon components.
4-7: Well-defined component interface with proper typing.The component has a clean interface that accepts the necessary props with proper TypeScript types.
8-14: API query integration with appropriate default value.The component properly uses the TRPC query hook to fetch the version selector status and defaults to
falsewhen data is not available, which is a safe fallback.
16-21: Loading state handling with appropriate UI.The component correctly handles the loading state with a visual indicator and descriptive text.
23-28: Success state handling with appropriate UI.The component correctly handles the success state with a visual indicator and descriptive text.
30-35: Failure state handling with appropriate UI.The component correctly handles the failure state with a visual indicator and descriptive text.
packages/api/src/router/deployment-version-checks/router.ts (3)
1-11: Well-organized imports for the router module.The imports are appropriately grouped and include necessary dependencies for schema access, authorization, and sub-routers.
12-37: Well-implemented environment retrieval procedure with proper authorization.The
environmentsToCheckprocedure:
- Correctly validates input as a UUID
- Implements appropriate authorization checks based on system-level permissions
- Returns early with false if the deployment doesn't exist
- Efficiently queries for distinct environments associated with a deployment
38-41: Modular router structure with clear component organization.The router follows good practices by aggregating focused sub-routers for approvals, deny windows, and version selectors, promoting maintainability and separation of concerns.
packages/api/src/router/deployment-version-checks/version-selector.ts (5)
1-15: Well-organized imports with clear structure.The imports are appropriately grouped by their source and include necessary dependencies for database operations, schema access, and utility functions.
16-29: Well-defined procedure with proper input validation and authorization.The procedure:
- Validates input with proper zod schema for both versionId and environmentId
- Implements appropriate authorization checks based on deployment version permissions
30-47: Good error handling and data retrieval.The procedure correctly:
- Extracts input parameters
- Fetches the necessary version and environment data
- Throws a proper TRPCError with NOT_FOUND code if the environment doesn't exist
- Extracts workspace information from the system for further processing
48-61: Effective policy evaluation with appropriate default behavior.The code properly:
- Retrieves the applicable release target and policies
- Merges policies to create a combined policy view
- Extracts the version selector from the merged policy
- Returns true by default if no version selector is defined
63-71: Efficient version selector evaluation using database query.The implementation efficiently:
- Uses a database query to check if the version matches the selector criteria
- Applies both ID equality and custom selector conditions
- Returns a boolean result based on whether a matching version was found
packages/api/src/router/deployment-version-checks/approvals.ts (2)
20-65: Implementation looks good and follows best practices.This
statusendpoint correctly handles authorization, performs policy checks, and evaluates approval status. The code uses appropriate error handling through utility functions, maintains type safety with Zod validation, and ensures proper authorization checks.
83-125: Implementation looks robust, with proper queueing of follow-up evaluations.The mutation correctly:
- Inserts the approval record with appropriate metadata
- Fetches affected release targets
- Enqueues evaluation jobs to handle downstream effects
- Returns the created record
The code follows good practices by using bulk operations for the queue and properly tracking approval timestamps.
| import { z } from "zod"; | ||
|
|
||
| import { Permission } from "@ctrlplane/validators/auth"; | ||
|
|
||
| import { createTRPCRouter, protectedProcedure } from "../../trpc"; | ||
|
|
||
| export const denyWindowRouter = createTRPCRouter({ | ||
| status: protectedProcedure | ||
| .input( | ||
| z.object({ | ||
| workspaceId: z.string().uuid(), | ||
| versionId: z.string().uuid(), | ||
| environmentId: z.string().uuid(), | ||
| }), | ||
| ) | ||
| .meta({ | ||
| authorizationCheck: ({ canUser, input }) => | ||
| canUser.perform(Permission.DeploymentVersionGet).on({ | ||
| type: "deploymentVersion", | ||
| id: input.versionId, | ||
| }), | ||
| }) | ||
| .query(() => false), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement actual deny window check logic
The current implementation is a placeholder that always returns false, indicating that deny windows are never active. In a real-world scenario, you would want to implement the actual logic to check if the current time falls within any deny windows associated with the environment or version.
export const denyWindowRouter = createTRPCRouter({
status: protectedProcedure
.input(
z.object({
workspaceId: z.string().uuid(),
versionId: z.string().uuid(),
environmentId: z.string().uuid(),
}),
)
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser.perform(Permission.DeploymentVersionGet).on({
type: "deploymentVersion",
id: input.versionId,
}),
})
- .query(() => false),
+ .query(async ({ ctx, input }) => {
+ const { db } = ctx;
+ const { versionId, environmentId } = input;
+
+ // Get release target for this deployment and environment
+ const rt = await db.query.releaseTarget.findFirst({
+ where: and(
+ eq(SCHEMA.releaseTarget.environmentId, environmentId),
+ ),
+ with: {
+ deployment: {
+ with: {
+ versions: {
+ where: eq(SCHEMA.deploymentVersion.id, versionId)
+ }
+ }
+ }
+ }
+ });
+
+ if (!rt) return false;
+
+ // Get policies that apply to this release target
+ const policies = await getApplicablePoliciesWithoutResourceScope(db, rt.id);
+
+ // Check if any policy has active deny windows
+ const now = new Date();
+ return policies.some(policy =>
+ policy.denyWindows.some(window => {
+ const startTime = new Date(window.startTime);
+ const endTime = new Date(window.endTime);
+ return now >= startTime && now <= endTime;
+ })
+ );
+ }),
});Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Chores