fix: Move deployments between systems #255
Conversation
WalkthroughThis pull request includes significant updates across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
packages/job-dispatch/src/deployment-update.ts (1)
81-87: Simplify fetching the system associated with the updated deploymentSince
updatedDeployment.systemIdis already known, consider using a relation or join to fetch the system in a single query.You might modify the code as follows to streamline the retrieval:
- const sys = await db - .select() - .from(SCHEMA.system) - .where(eq(SCHEMA.system.id, updatedDeployment.systemId)) - .then(takeFirst); + const updatedDeploymentWithSystem = await db.query.deployment.findFirst({ + where: eq(SCHEMA.deployment.id, updatedDeployment.id), + with: { system: true }, + }); + return updatedDeploymentWithSystem;packages/job-dispatch/src/events/handlers/resource-removed.ts (1)
23-25: Consider storing the event even if no runhooks are presentThe early return when
runhooks.length === 0prevents the event from being inserted into the database. If it's important to log all resource removal events, consider inserting the event regardless of the presence of runhooks.You might adjust the code as follows:
if (runhooks.length === 0) { + await db.insert(SCHEMA.event).values(event); return; } await db.insert(SCHEMA.event).values(event);apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx (2)
6-6: Consider more specific importsInstead of using wildcard import
* as schema, consider importing specific entities to improve code maintainability and bundle size optimization.-import * as schema from "@ctrlplane/db/schema"; +import { Deployment, System, deploymentSchema } from "@ctrlplane/db/schema";Also applies to: 19-25
131-154: Add loading state for system changesThe system selection UI should reflect when a system change is in progress. Consider adding a loading state to prevent multiple submissions and improve user experience.
<FormField control={form.control} name="systemId" render={({ field: { value, onChange } }) => ( <FormItem> <FormLabel>System</FormLabel> <FormControl> - <Select value={value} onValueChange={onChange}> + <Select + value={value} + onValueChange={onChange} + disabled={form.formState.isSubmitting} + > <SelectTrigger> <SelectValue placeholder="Select a system" /> </SelectTrigger> <SelectContent> {systems.map((system) => ( <SelectItem key={system.id} value={system.id}> {system.name} </SelectItem> ))} </SelectContent> </Select> </FormControl> <FormMessage /> </FormItem> )} />packages/api/src/router/deployment.ts (1)
Line range hint
474-479: Verify event handling failures during deployment deletion.The code uses
Promise.allSettledwhich is good for handling multiple events, but we should verify the behavior when events fail. Consider logging failed events and potentially notifying administrators.const events = await getEventsForDeploymentRemoved(dep, dep.systemId); -await Promise.allSettled(events.map(handleEvent)); +const results = await Promise.allSettled(events.map(handleEvent)); + +// Log any failed events +const failedEvents = results + .filter((r): r is PromiseRejectedResult => r.status === 'rejected') + .map(r => r.reason); +if (failedEvents.length > 0) { + console.error('Failed to handle some deployment removal events:', failedEvents); + // Consider notifying administrators or adding metrics +} + return ctx.db .delete(deployment) .where(eq(deployment.id, input)) .returning() .then(takeFirst);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/page.tsx(3 hunks)packages/api/src/router/deployment.ts(3 hunks)packages/db/src/schema/deployment.ts(1 hunks)packages/job-dispatch/src/deployment-update.ts(1 hunks)packages/job-dispatch/src/events/handlers/resource-removed.ts(1 hunks)packages/job-dispatch/src/events/index.ts(1 hunks)packages/job-dispatch/src/events/triggers/deployment-removed.ts(1 hunks)packages/job-dispatch/src/events/triggers/index.ts(1 hunks)packages/job-dispatch/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/job-dispatch/src/events/triggers/index.ts
🔇 Additional comments (7)
packages/job-dispatch/src/events/index.ts (1)
8-9: Verify the impact of removing the event insertion into the database
The handleEvent function no longer inserts the event into the database. Ensure that this change doesn't affect any downstream processes that rely on events being stored.
Run the following script to check for dependencies on event storage:
packages/job-dispatch/src/index.ts (1)
3-3: New module deployment-update.js successfully exported
The addition of export * from "./deployment-update.js"; correctly exposes the new deployment update functionalities.
packages/job-dispatch/src/events/triggers/deployment-removed.ts (1)
11-18: Passing systemId as a parameter enhances flexibility
By accepting systemId as a parameter, the function is more flexible and can handle cases where the system ID differs from the deployment's associated system. This is beneficial for scenarios where the deployment is moved between systems.
packages/db/src/schema/deployment.ts (1)
73-73: LGTM: Type export enhances type safety
The new UpdateDeployment type correctly infers its structure from the updateDeployment schema, providing proper type safety for deployment updates.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/page.tsx (2)
150-151: LGTM: Improved workspace ID handling
Good refactoring to extract workspaceId early and use it consistently throughout the component. The systems list fetch is appropriately placed after workspace validation.
Also applies to: 154-154
167-167: Verify system access permissions
Ensure that the user has permissions to view and select from all systems in the workspace. Consider filtering the systems list based on user permissions.
Also applies to: 177-177
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx (1)
30-35: Consider adding validation for system selection
Add validation to ensure the selected system is valid and the user has appropriate permissions.
| if (prevDeployment.systemId !== updatedDeployment.systemId) | ||
| await handleDeploymentSystemChanged( | ||
| updatedDeployment, | ||
| prevDeployment.systemId, | ||
| userId, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add braces to multi-line if statement for clarity
The if statement spans multiple lines and calls an asynchronous function. To improve readability and prevent potential errors, it's recommended to wrap the block in braces.
Apply this diff:
if (prevDeployment.systemId !== updatedDeployment.systemId)
- await handleDeploymentSystemChanged(
- updatedDeployment,
- prevDeployment.systemId,
- userId,
- );
+ {
+ await handleDeploymentSystemChanged(
+ updatedDeployment,
+ prevDeployment.systemId,
+ userId,
+ );
+ }📝 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.
| if (prevDeployment.systemId !== updatedDeployment.systemId) | |
| await handleDeploymentSystemChanged( | |
| updatedDeployment, | |
| prevDeployment.systemId, | |
| userId, | |
| ); | |
| if (prevDeployment.systemId !== updatedDeployment.systemId) { | |
| await handleDeploymentSystemChanged( | |
| updatedDeployment, | |
| prevDeployment.systemId, | |
| userId, | |
| ); | |
| } |
| const updatedDeployment = await db | ||
| .update(SCHEMA.deployment) | ||
| .set(data) | ||
| .where(eq(SCHEMA.deployment.id, deploymentId)) | ||
| .returning() | ||
| .then(takeFirst); | ||
|
|
There was a problem hiding this comment.
Check if updatedDeployment is undefined after the update
After updating the deployment, updatedDeployment might be undefined if the update did not affect any rows. Add error handling to manage this scenario.
Apply this diff to ensure updatedDeployment is valid:
const updatedDeployment = await db
.update(SCHEMA.deployment)
.set(data)
.where(eq(SCHEMA.deployment.id, deploymentId))
.returning()
.then(takeFirst);
+ if (!updatedDeployment) {
+ throw new Error(`Failed to update deployment with id ${deploymentId}`);
+ }📝 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.
| const updatedDeployment = await db | |
| .update(SCHEMA.deployment) | |
| .set(data) | |
| .where(eq(SCHEMA.deployment.id, deploymentId)) | |
| .returning() | |
| .then(takeFirst); | |
| const updatedDeployment = await db | |
| .update(SCHEMA.deployment) | |
| .set(data) | |
| .where(eq(SCHEMA.deployment.id, deploymentId)) | |
| .returning() | |
| .then(takeFirst); | |
| if (!updatedDeployment) { | |
| throw new Error(`Failed to update deployment with id ${deploymentId}`); | |
| } | |
| const prevDeployment = await db | ||
| .select() | ||
| .from(SCHEMA.deployment) | ||
| .where(eq(SCHEMA.deployment.id, deploymentId)) | ||
| .then(takeFirst); | ||
|
|
There was a problem hiding this comment.
Ensure prevDeployment is valid and handle potential errors
The variable prevDeployment may be undefined if no deployment matches the deploymentId. Consider adding error handling to manage this scenario, possibly by throwing an error if prevDeployment is not found.
Apply this diff to handle the potential undefined value:
const prevDeployment = await db
.select()
.from(SCHEMA.deployment)
.where(eq(SCHEMA.deployment.id, deploymentId))
.then(takeFirst);
+ if (!prevDeployment) {
+ throw new Error(`Deployment with id ${deploymentId} not found`);
+ }📝 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.
| const prevDeployment = await db | |
| .select() | |
| .from(SCHEMA.deployment) | |
| .where(eq(SCHEMA.deployment.id, deploymentId)) | |
| .then(takeFirst); | |
| const prevDeployment = await db | |
| .select() | |
| .from(SCHEMA.deployment) | |
| .where(eq(SCHEMA.deployment.id, deploymentId)) | |
| .then(takeFirst); | |
| if (!prevDeployment) { | |
| throw new Error(`Deployment with id ${deploymentId} not found`); | |
| } |
| const events = await getEventsForDeploymentRemoved(deployment, prevSystemId); | ||
| await Promise.allSettled(events.map(handleEvent)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle rejections from Promise.allSettled when processing events
Currently, the results of Promise.allSettled are not being checked. If any of the handleEvent promises are rejected, the errors will be silently ignored. Consider handling the rejections to ensure that any errors are properly logged or addressed.
Apply this diff to handle rejections:
const events = await getEventsForDeploymentRemoved(deployment, prevSystemId);
- await Promise.allSettled(events.map(handleEvent));
+ const results = await Promise.allSettled(events.map(handleEvent));
+ results.forEach((result) => {
+ if (result.status === 'rejected') {
+ // Handle or log the error
+ console.error('Error handling event:', result.reason);
+ }
+ });Committable suggestion skipped: line range outside the PR's diff.
| .then((updatedDeployment) => { | ||
| if ( | ||
| data.slug !== deployment.slug || | ||
| updatedDeployment.systemId !== deployment.systemId | ||
| ) | ||
| router.replace( | ||
| `/${workspaceSlug}/systems/${systemSlug}/deployments/${data.slug}`, | ||
| `/${workspaceSlug}/systems/${updatedDeployment.system.slug}/deployments/${data.slug}`, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add confirmation dialog for system changes
Moving a deployment between systems is a significant operation that might have downstream effects. Consider adding a confirmation dialog when the system is changed.
const onSubmit = handleSubmit((data) => {
updateDeployment
.mutateAsync({ id: deployment.id, data })
.then((updatedDeployment) => {
- if (
- data.slug !== deployment.slug ||
- updatedDeployment.systemId !== deployment.systemId
- )
- router.replace(
- `/${workspaceSlug}/systems/${updatedDeployment.system.slug}/deployments/${data.slug}`,
- );
+ const isSystemChanged = updatedDeployment.systemId !== deployment.systemId;
+ const isSlugChanged = data.slug !== deployment.slug;
+
+ if (isSystemChanged) {
+ // Add confirmation dialog here
+ const confirmed = window.confirm(
+ "Moving a deployment to a different system may affect its behavior. Are you sure?"
+ );
+ if (!confirmed) return;
+ }
+
+ if (isSystemChanged || isSlugChanged) {
+ router.replace(
+ `/${workspaceSlug}/systems/${updatedDeployment.system.slug}/deployments/${data.slug}`,
+ );
+ }
router.refresh();
})Committable suggestion skipped: line range outside the PR's diff.
| .input(z.object({ id: z.string().uuid(), data: updateDeploymentSchema })) | ||
| .mutation(({ ctx, input }) => | ||
| ctx.db | ||
| .update(deployment) | ||
| .set(input.data) | ||
| .where(eq(deployment.id, input.id)) | ||
| .returning(), | ||
| updateDeployment(input.id, input.data, ctx.session.user.id), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the external updateDeployment call.
The update mutation now delegates to updateDeployment from job-dispatch, which is good for centralization. However, we should ensure proper error handling for this external call.
.input(z.object({ id: z.string().uuid(), data: updateDeploymentSchema }))
-.mutation(({ ctx, input }) =>
- updateDeployment(input.id, input.data, ctx.session.user.id),
+.mutation(async ({ ctx, input }) => {
+ try {
+ return await updateDeployment(input.id, input.data, ctx.session.user.id);
+ } catch (error) {
+ // Log the error for debugging
+ console.error('Failed to update deployment:', error);
+ throw new Error('Failed to update deployment. Please try again.');
+ }
+}),📝 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.
| .input(z.object({ id: z.string().uuid(), data: updateDeploymentSchema })) | |
| .mutation(({ ctx, input }) => | |
| ctx.db | |
| .update(deployment) | |
| .set(input.data) | |
| .where(eq(deployment.id, input.id)) | |
| .returning(), | |
| updateDeployment(input.id, input.data, ctx.session.user.id), | |
| .input(z.object({ id: z.string().uuid(), data: updateDeploymentSchema })) | |
| .mutation(async ({ ctx, input }) => { | |
| try { | |
| return await updateDeployment(input.id, input.data, ctx.session.user.id); | |
| } catch (error) { | |
| // Log the error for debugging | |
| console.error('Failed to update deployment:', error); | |
| throw new Error('Failed to update deployment. Please try again.'); | |
| } | |
| }), |
| @@ -33,14 +33,15 @@ | |||
| runbookVariable, | |||
| runhook, | |||
| system, | |||
| updateDeployment, | |||
| updateDeployment as updateDeploymentSchema, | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to new function names needs attention
The verification revealed several concerns:
updateDeploymentis still being used across multiple files in the codebase, suggesting incomplete migration- While
getEventsForDeploymentDeletedhas been fully removed, both new names (updateDeploymentSchemaandgetEventsForDeploymentRemoved) appear in the expected locations
Files needing attention for updateDeployment usage:
- packages/api/src/router/deployment-variable.ts
- packages/db/src/schema/deployment.ts
- packages/db/src/schema/deployment-variables.ts
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/_components/EditDeploymentDialog.tsx
🔗 Analysis chain
Verify consistent usage of renamed functions across the codebase.
The imports have been renamed to better reflect their purposes:
updateDeployment→updateDeploymentSchemagetEventsForDeploymentDeleted→getEventsForDeploymentRemoved
Also applies to: 42-42, 44-44
Let me modify the search patterns to get more accurate results.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that old function names are not used elsewhere in the codebase
# Test 1: Check for any remaining usage of old function names
echo "Checking for old function names..."
rg -l "updateDeployment(?!Schema)" || echo "No occurrences found"
rg -l "getEventsForDeploymentDeleted" || echo "No occurrences found"
# Test 2: Verify the new function signatures
echo -e "\nVerifying new function signatures..."
ast-grep --pattern 'function updateDeploymentSchema($_) { $$$ }'
ast-grep --pattern 'function getEventsForDeploymentRemoved($_) { $$$ }'
Length of output: 731
Script:
#!/bin/bash
# Description: Verify function names with improved search patterns
# Check for updateDeployment usage (excluding Schema)
echo "Checking updateDeployment usage..."
rg -l --pcre2 'updateDeployment(?!Schema)' || echo "No occurrences found"
# Check for getEventsForDeploymentDeleted
echo -e "\nChecking getEventsForDeploymentDeleted usage..."
rg -l 'getEventsForDeploymentDeleted' || echo "No occurrences found"
# Check for new function names
echo -e "\nChecking new function names..."
rg -l 'updateDeploymentSchema' || echo "No updateDeploymentSchema found"
rg -l 'getEventsForDeploymentRemoved' || echo "No getEventsForDeploymentRemoved found"
# Check the actual implementations
echo -e "\nChecking implementations..."
rg -A 2 'function updateDeploymentSchema' || echo "No updateDeploymentSchema implementation found"
rg -A 2 'function getEventsForDeploymentRemoved' || echo "No getEventsForDeploymentRemoved implementation found"
Length of output: 1463
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/job-dispatch/src/deployment-update.ts(1 hunks)
🔇 Additional comments (3)
packages/job-dispatch/src/deployment-update.ts (3)
125-130: Ensure prevDeployment is valid and handle potential errors
The previous review comment about handling undefined prevDeployment is still valid.
131-137: Check if updatedDeployment is undefined after the update
The previous review comment about handling undefined updatedDeployment is still valid.
138-143: Add braces to multi-line if statement for clarity
The previous review comment about adding braces to the if statement is still valid.
| const sys = await db | ||
| .select() | ||
| .from(SCHEMA.system) | ||
| .where(eq(SCHEMA.system.id, updatedDeployment.systemId)) | ||
| .then(takeFirst); | ||
|
|
||
| return { ...updatedDeployment, system: sys }; | ||
| }; |
There was a problem hiding this comment.
Add error handling for system query and validate return object
The final system query could fail silently, and the return object structure should be validated before returning.
Apply this diff:
- const sys = await db
+ const system = await db
.select()
.from(SCHEMA.system)
.where(eq(SCHEMA.system.id, updatedDeployment.systemId))
.then(takeFirst);
+
+ if (!system) {
+ throw new Error(
+ `System not found for updated deployment: ${updatedDeployment.systemId}`
+ );
+ }
- return { ...updatedDeployment, system: sys };
+ return { ...updatedDeployment, system };📝 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.
| const sys = await db | |
| .select() | |
| .from(SCHEMA.system) | |
| .where(eq(SCHEMA.system.id, updatedDeployment.systemId)) | |
| .then(takeFirst); | |
| return { ...updatedDeployment, system: sys }; | |
| }; | |
| const system = await db | |
| .select() | |
| .from(SCHEMA.system) | |
| .where(eq(SCHEMA.system.id, updatedDeployment.systemId)) | |
| .then(takeFirst); | |
| if (!system) { | |
| throw new Error( | |
| `System not found for updated deployment: ${updatedDeployment.systemId}` | |
| ); | |
| } | |
| return { ...updatedDeployment, system }; | |
| }; |
| const getResourcesOnlyInNewSystem = async ( | ||
| newSystemId: string, | ||
| oldSystemId: string, | ||
| ) => { | ||
| const hasFilter = isNotNull(SCHEMA.environment.resourceFilter); | ||
| const newSystem = await db.query.system.findFirst({ | ||
| where: eq(SCHEMA.system.id, newSystemId), | ||
| with: { environments: { where: hasFilter } }, | ||
| }); | ||
|
|
||
| const oldSystem = await db.query.system.findFirst({ | ||
| where: eq(SCHEMA.system.id, oldSystemId), | ||
| with: { environments: { where: hasFilter } }, | ||
| }); | ||
|
|
||
| if (newSystem == null || oldSystem == null) return []; | ||
|
|
||
| const newSystemFilter: ResourceCondition = { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.Or, | ||
| conditions: newSystem.environments | ||
| .flatMap((env) => env.resourceFilter) | ||
| .filter(isPresent), | ||
| }; | ||
|
|
||
| const notInOldSystemFilter: ResourceCondition = { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.Or, | ||
| not: true, | ||
| conditions: oldSystem.environments | ||
| .flatMap((env) => env.resourceFilter) | ||
| .filter(isPresent), | ||
| }; | ||
|
|
||
| const filter: ResourceCondition = { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.And, | ||
| conditions: [newSystemFilter, notInOldSystemFilter], | ||
| }; | ||
|
|
||
| return db.query.resource.findMany({ | ||
| where: and( | ||
| SCHEMA.resourceMatchesMetadata(db, filter), | ||
| isNull(SCHEMA.resource.deletedAt), | ||
| ), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database queries
The function should handle potential database errors that might occur during system queries. Currently, errors would propagate up the call stack without any context about the operation that failed.
Apply this diff:
const getResourcesOnlyInNewSystem = async (
newSystemId: string,
oldSystemId: string,
) => {
const hasFilter = isNotNull(SCHEMA.environment.resourceFilter);
- const newSystem = await db.query.system.findFirst({
- where: eq(SCHEMA.system.id, newSystemId),
- with: { environments: { where: hasFilter } },
- });
+ try {
+ const newSystem = await db.query.system.findFirst({
+ where: eq(SCHEMA.system.id, newSystemId),
+ with: { environments: { where: hasFilter } },
+ });
- const oldSystem = await db.query.system.findFirst({
- where: eq(SCHEMA.system.id, oldSystemId),
- with: { environments: { where: hasFilter } },
- });
+ const oldSystem = await db.query.system.findFirst({
+ where: eq(SCHEMA.system.id, oldSystemId),
+ with: { environments: { where: hasFilter } },
+ });
- if (newSystem == null || oldSystem == null) return [];
+ if (newSystem == null || oldSystem == null) return [];
+ // ... rest of the function
+ } catch (error) {
+ throw new Error(
+ `Failed to fetch systems for comparison: ${error.message}`,
+ { cause: error }
+ );
+ }Committable suggestion skipped: line range outside the PR's diff.
| export const handleDeploymentSystemChanged = async ( | ||
| deployment: SCHEMA.Deployment, | ||
| prevSystemId: string, | ||
| userId?: string, | ||
| ) => { | ||
| const resourcesOnlyInNewSystem = await getResourcesOnlyInNewSystem( | ||
| deployment.systemId, | ||
| prevSystemId, | ||
| ); | ||
|
|
||
| const events = resourcesOnlyInNewSystem.map((resource) => ({ | ||
| action: "deployment.resource.removed" as const, | ||
| payload: { deployment, resource }, | ||
| })); | ||
| await Promise.allSettled(events.map(handleEvent)); | ||
|
|
||
| const isDeploymentHook = and( | ||
| eq(SCHEMA.hook.scopeType, "deployment"), | ||
| eq(SCHEMA.hook.scopeId, deployment.id), | ||
| ); | ||
| await db.query.hook | ||
| .findMany({ | ||
| where: isDeploymentHook, | ||
| with: { runhooks: { with: { runbook: true } } }, | ||
| }) | ||
| .then((hooks) => { | ||
| const runbookIds = hooks.flatMap((h) => | ||
| h.runhooks.map((rh) => rh.runbook.id), | ||
| ); | ||
| return db | ||
| .update(SCHEMA.runbook) | ||
| .set({ systemId: deployment.systemId }) | ||
| .where(inArray(SCHEMA.runbook.id, runbookIds)); | ||
| }); | ||
|
|
||
| const createTriggers = | ||
| userId != null | ||
| ? createReleaseJobTriggers(db, "new_release").causedById(userId) | ||
| : createReleaseJobTriggers(db, "new_release"); | ||
| await createTriggers | ||
| .deployments([deployment.id]) | ||
| .resources(resourcesOnlyInNewSystem.map((r) => r.id)) | ||
| .filter(isPassingReleaseStringCheckPolicy) | ||
| .then(createJobApprovals) | ||
| .insert() | ||
| .then((triggers) => | ||
| dispatchReleaseJobTriggers(db) | ||
| .releaseTriggers(triggers) | ||
| .filter(isPassingAllPolicies) | ||
| .dispatch(), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Wrap related database operations in a transaction
Multiple database operations (runbook updates and trigger creation) should be wrapped in a transaction to ensure data consistency in case of failures.
Apply this diff:
export const handleDeploymentSystemChanged = async (
deployment: SCHEMA.Deployment,
prevSystemId: string,
userId?: string,
) => {
+ return db.transaction(async (tx) => {
const resourcesOnlyInNewSystem = await getResourcesOnlyInNewSystem(
deployment.systemId,
prevSystemId,
);
const events = resourcesOnlyInNewSystem.map((resource) => ({
action: "deployment.resource.removed" as const,
payload: { deployment, resource },
}));
await Promise.allSettled(events.map(handleEvent));
const isDeploymentHook = and(
eq(SCHEMA.hook.scopeType, "deployment"),
eq(SCHEMA.hook.scopeId, deployment.id),
);
- await db.query.hook
+ await tx.query.hook
.findMany({
where: isDeploymentHook,
with: { runhooks: { with: { runbook: true } } },
})
.then((hooks) => {
const runbookIds = hooks.flatMap((h) =>
h.runhooks.map((rh) => rh.runbook.id),
);
- return db
+ return tx
.update(SCHEMA.runbook)
.set({ systemId: deployment.systemId })
.where(inArray(SCHEMA.runbook.id, runbookIds));
});
const createTriggers =
userId != null
- ? createReleaseJobTriggers(db, "new_release").causedById(userId)
- : createReleaseJobTriggers(db, "new_release");
+ ? createReleaseJobTriggers(tx, "new_release").causedById(userId)
+ : createReleaseJobTriggers(tx, "new_release");
await createTriggers
.deployments([deployment.id])
.resources(resourcesOnlyInNewSystem.map((r) => r.id))
.filter(isPassingReleaseStringCheckPolicy)
.then(createJobApprovals)
.insert()
.then((triggers) =>
- dispatchReleaseJobTriggers(db)
+ dispatchReleaseJobTriggers(tx)
.releaseTriggers(triggers)
.filter(isPassingAllPolicies)
.dispatch(),
);
+ });
};📝 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.
| export const handleDeploymentSystemChanged = async ( | |
| deployment: SCHEMA.Deployment, | |
| prevSystemId: string, | |
| userId?: string, | |
| ) => { | |
| const resourcesOnlyInNewSystem = await getResourcesOnlyInNewSystem( | |
| deployment.systemId, | |
| prevSystemId, | |
| ); | |
| const events = resourcesOnlyInNewSystem.map((resource) => ({ | |
| action: "deployment.resource.removed" as const, | |
| payload: { deployment, resource }, | |
| })); | |
| await Promise.allSettled(events.map(handleEvent)); | |
| const isDeploymentHook = and( | |
| eq(SCHEMA.hook.scopeType, "deployment"), | |
| eq(SCHEMA.hook.scopeId, deployment.id), | |
| ); | |
| await db.query.hook | |
| .findMany({ | |
| where: isDeploymentHook, | |
| with: { runhooks: { with: { runbook: true } } }, | |
| }) | |
| .then((hooks) => { | |
| const runbookIds = hooks.flatMap((h) => | |
| h.runhooks.map((rh) => rh.runbook.id), | |
| ); | |
| return db | |
| .update(SCHEMA.runbook) | |
| .set({ systemId: deployment.systemId }) | |
| .where(inArray(SCHEMA.runbook.id, runbookIds)); | |
| }); | |
| const createTriggers = | |
| userId != null | |
| ? createReleaseJobTriggers(db, "new_release").causedById(userId) | |
| : createReleaseJobTriggers(db, "new_release"); | |
| await createTriggers | |
| .deployments([deployment.id]) | |
| .resources(resourcesOnlyInNewSystem.map((r) => r.id)) | |
| .filter(isPassingReleaseStringCheckPolicy) | |
| .then(createJobApprovals) | |
| .insert() | |
| .then((triggers) => | |
| dispatchReleaseJobTriggers(db) | |
| .releaseTriggers(triggers) | |
| .filter(isPassingAllPolicies) | |
| .dispatch(), | |
| ); | |
| }; | |
| export const handleDeploymentSystemChanged = async ( | |
| deployment: SCHEMA.Deployment, | |
| prevSystemId: string, | |
| userId?: string, | |
| ) => { | |
| return db.transaction(async (tx) => { | |
| const resourcesOnlyInNewSystem = await getResourcesOnlyInNewSystem( | |
| deployment.systemId, | |
| prevSystemId, | |
| ); | |
| const events = resourcesOnlyInNewSystem.map((resource) => ({ | |
| action: "deployment.resource.removed" as const, | |
| payload: { deployment, resource }, | |
| })); | |
| await Promise.allSettled(events.map(handleEvent)); | |
| const isDeploymentHook = and( | |
| eq(SCHEMA.hook.scopeType, "deployment"), | |
| eq(SCHEMA.hook.scopeId, deployment.id), | |
| ); | |
| await tx.query.hook | |
| .findMany({ | |
| where: isDeploymentHook, | |
| with: { runhooks: { with: { runbook: true } } }, | |
| }) | |
| .then((hooks) => { | |
| const runbookIds = hooks.flatMap((h) => | |
| h.runhooks.map((rh) => rh.runbook.id), | |
| ); | |
| return tx | |
| .update(SCHEMA.runbook) | |
| .set({ systemId: deployment.systemId }) | |
| .where(inArray(SCHEMA.runbook.id, runbookIds)); | |
| }); | |
| const createTriggers = | |
| userId != null | |
| ? createReleaseJobTriggers(tx, "new_release").causedById(userId) | |
| : createReleaseJobTriggers(tx, "new_release"); | |
| await createTriggers | |
| .deployments([deployment.id]) | |
| .resources(resourcesOnlyInNewSystem.map((r) => r.id)) | |
| .filter(isPassingReleaseStringCheckPolicy) | |
| .then(createJobApprovals) | |
| .insert() | |
| .then((triggers) => | |
| dispatchReleaseJobTriggers(tx) | |
| .releaseTriggers(triggers) | |
| .filter(isPassingAllPolicies) | |
| .dispatch(), | |
| ); | |
| }); | |
| }; |
Summary by CodeRabbit
Release Notes
New Features
EditDeploymentSectionwith a newsystemIdselection field, allowing users to choose from available systems.Bug Fixes
Documentation
Chores