fix: Trigger release for all resources when moving deployment #257
Conversation
WalkthroughThe changes in this pull request primarily focus 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: 2
🧹 Outside diff range and nitpick comments (1)
packages/job-dispatch/src/deployment-update.ts (1)
76-76: Handle cases whenresourcesarray is emptyIf no resources match the
systemFilter,resourceswill be an empty array. Ensure that the subsequent operations can handle an empty array without causing errors.Consider adding a check before proceeding:
const resources = await db.query.resource.findMany({ /* ... */ }); + if (resources.length === 0) { + // No resources to process; handle accordingly + return; + } await createTriggers .deployments([deployment.id]) .resources(resources.map((r) => r.id))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/job-dispatch/src/deployment-update.ts(2 hunks)
🔇 Additional comments (5)
packages/job-dispatch/src/deployment-update.ts (5)
12-12: Import statements are correctly updated
The new imports getEventsForDeploymentRemoved and handleEvent are appropriately added for handling events related to deployment removal.
19-22: Function handleDeploymentSystemChanged signature updated appropriately
The function now accepts deployment, prevSystemId, and an optional userId, which aligns with the updated logic and ensures necessary data is available within the function.
26-26: Correctly querying the new system by deployment.systemId
Updating the query to use deployment.systemId ensures that the correct new system is fetched from the database.
30-30: Early return when newSystem is not found
Returning early if newSystem is null prevents potential errors from attempting to access properties on an undefined object.
46-46: Ensure systemFilter is valid before querying resources
Confirm that systemFilter is properly constructed and valid, especially after handling the case when conditions might be empty, to prevent unintended database queries.
✅ Verification successful
From analyzing the code, I can see that systemFilter is properly constructed and validated:
- The
systemFilteris constructed with a valid type and operator:
const systemFilter: ResourceCondition = {
type: FilterType.Comparison,
operator: ComparisonOperator.Or,
conditions: newSystem.environments
.flatMap((env) => env.resourceFilter)
.filter(isPresent),
};-
The
filter(isPresent)call ensures that only non-null/non-undefined resource filters are included in the conditions array. -
The filter is used in a database query through
SCHEMA.resourceMatchesMetadata(db, systemFilter), which is a valid schema operation. -
The codebase shows consistent handling of resource filters with proper type checking and validation throughout the application.
The filter construction is safe and properly validated
The systemFilter is properly constructed with null/undefined checks through filter(isPresent), ensuring that only valid resource filters are included in the conditions array before querying the database.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that systemFilter.conditions is not empty before querying resources.
# Test: Check if any environments have resource filters.
# Expect: systemFilter.conditions should contain at least one condition.
# Get count of environments with resource filters for the new system
psql -c "
SELECT COUNT(*) FROM environments
WHERE system_id = '${deployment.systemId}'
AND resource_filter IS NOT NULL;
"
Length of output: 200
Script:
#!/bin/bash
# Let's examine how systemFilter is constructed and validated
rg -A 10 "systemFilter" packages/job-dispatch/src/deployment-update.ts
# Also check for any validation functions related to filters
ast-grep --pattern 'function $FUNC_NAME($_) {
$$$
filter $$$
$$$
}'
# Check for any filter-related type definitions
rg -A 5 "type.*Filter"
Length of output: 71116
| await getEventsForDeploymentRemoved(deployment, prevSystemId).then((events) => | ||
| Promise.allSettled(events.map(handleEvent)), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Process results of Promise.allSettled to handle rejections
Currently, any rejections from handleEvent are silently ignored. Consider processing the results of Promise.allSettled to handle any errors appropriately.
Apply this diff to handle the results:
await getEventsForDeploymentRemoved(deployment, prevSystemId).then((events) =>
- Promise.allSettled(events.map(handleEvent)),
+ Promise.allSettled(events.map(handleEvent)).then((results) => {
+ results.forEach((result, index) => {
+ if (result.status === 'rejected') {
+ console.error(`Error handling event:`, result.reason);
+ // Additional error handling logic if necessary
+ }
+ });
+ }),
);📝 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.
| await getEventsForDeploymentRemoved(deployment, prevSystemId).then((events) => | |
| Promise.allSettled(events.map(handleEvent)), | |
| ); | |
| await getEventsForDeploymentRemoved(deployment, prevSystemId).then((events) => | |
| Promise.allSettled(events.map(handleEvent)).then((results) => { | |
| results.forEach((result, index) => { | |
| if (result.status === 'rejected') { | |
| console.error(`Error handling event:`, result.reason); | |
| // Additional error handling logic if necessary | |
| } | |
| }); | |
| }), | |
| ); |
| const systemFilter: ResourceCondition = { | ||
| type: FilterType.Comparison, | ||
| operator: ComparisonOperator.Or, | ||
| conditions: newSystem.environments | ||
| .flatMap((env) => env.resourceFilter) | ||
| .filter(isPresent), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle empty conditions in systemFilter
If newSystem.environments have no resourceFilter, the conditions array may be empty, which could lead to unintended behavior in SCHEMA.resourceMatchesMetadata. Consider adding a check to handle the case when conditions is empty.
Apply this diff to add a check:
const systemFilter: ResourceCondition = {
type: FilterType.Comparison,
operator: ComparisonOperator.Or,
conditions: newSystem.environments
.flatMap((env) => env.resourceFilter)
.filter(isPresent),
};
+
+ if (systemFilter.conditions.length === 0) {
+ // No conditions to filter resources; handle accordingly
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/job-dispatch/src/deployment-update.ts (2)
43-67: Add type validation for resource filtersWhile the function handles null checks well, it would benefit from runtime validation of the filter structure to ensure type safety.
Consider adding validation:
const getResourcesInNewSystem = async (deployment: SCHEMA.Deployment) => { const hasFilter = isNotNull(SCHEMA.environment.resourceFilter); const newSystem = await db.query.system.findFirst({ where: eq(SCHEMA.system.id, deployment.systemId), with: { environments: { where: hasFilter } }, }); if (newSystem == null) return []; const filters = newSystem.environments .map((env) => env.resourceFilter) .filter(isPresent); if (filters.length === 0) return []; + // Validate filter structure + const isValidFilter = (filter: unknown): filter is ResourceCondition => { + return ( + typeof filter === 'object' && + filter !== null && + 'type' in filter && + 'operator' in filter && + 'conditions' in filter + ); + }; + + if (!filters.every(isValidFilter)) { + throw new Error('Invalid resource filter structure detected'); + } const systemFilter: ResourceCondition = { type: FilterType.Comparison, operator: ComparisonOperator.Or, conditions: filters, }; return db.query.resource.findMany({ where: SCHEMA.resourceMatchesMetadata(db, systemFilter), }); };
Line range hint
69-97: Consider wrapping operations in a transactionThe function performs multiple sequential operations (event handling, runbook moving, and trigger creation) that should maintain consistency. Consider wrapping these operations in a transaction.
Consider this improvement:
export const handleDeploymentSystemChanged = async ( deployment: SCHEMA.Deployment, prevSystemId: string, userId?: string, ) => { + return db.transaction(async (tx) => { await getEventsForDeploymentRemoved(deployment, prevSystemId).then((events) => Promise.allSettled(events.map(handleEvent)), ); await moveRunbooksLinkedToHooksToNewSystem(deployment); const resources = await getResourcesInNewSystem(deployment); const createTriggers = userId != null ? createReleaseJobTriggers(tx, "new_release").causedById(userId) : createReleaseJobTriggers(tx, "new_release"); return createTriggers .deployments([deployment.id]) .resources(resources.map((r) => r.id)) .filter(isPassingReleaseStringCheckPolicy) .then(createJobApprovals) .insert() .then((triggers) => dispatchReleaseJobTriggers(tx) .releaseTriggers(triggers) .filter(isPassingAllPolicies) .dispatch(), ); + }).catch((error) => { + console.error('Failed to handle deployment system change:', error); + throw error; + }); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/job-dispatch/src/deployment-update.ts(2 hunks)
🔇 Additional comments (1)
packages/job-dispatch/src/deployment-update.ts (1)
74-75: Process results of Promise.allSettled to handle rejections
The previous review comment about handling Promise.allSettled rejections is still valid.
| const moveRunbooksLinkedToHooksToNewSystem = async ( | ||
| deployment: SCHEMA.Deployment, | ||
| ) => { | ||
| const isDeploymentHook = and( | ||
| eq(SCHEMA.hook.scopeType, "deployment"), | ||
| eq(SCHEMA.hook.scopeId, deployment.id), | ||
| ); | ||
|
|
||
| return 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)); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider using transactions
The function performs multiple database operations but lacks error handling. Additionally, the update operation should be part of a transaction to ensure data consistency.
Consider applying this improvement:
const moveRunbooksLinkedToHooksToNewSystem = async (
deployment: SCHEMA.Deployment,
) => {
+ return db.transaction(async (tx) => {
const isDeploymentHook = and(
eq(SCHEMA.hook.scopeType, "deployment"),
eq(SCHEMA.hook.scopeId, deployment.id),
);
- return db.query.hook
+ const hooks = 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
- .update(SCHEMA.runbook)
- .set({ systemId: deployment.systemId })
- .where(inArray(SCHEMA.runbook.id, runbookIds));
});
+
+ const runbookIds = hooks.flatMap((h) =>
+ h.runhooks.map((rh) => rh.runbook.id),
+ );
+
+ if (runbookIds.length === 0) return;
+
+ return tx
+ .update(SCHEMA.runbook)
+ .set({ systemId: deployment.systemId })
+ .where(inArray(SCHEMA.runbook.id, runbookIds));
+ }).catch((error) => {
+ console.error('Failed to move runbooks:', error);
+ throw error;
+ });
};📝 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 moveRunbooksLinkedToHooksToNewSystem = async ( | |
| deployment: SCHEMA.Deployment, | |
| ) => { | |
| const isDeploymentHook = and( | |
| eq(SCHEMA.hook.scopeType, "deployment"), | |
| eq(SCHEMA.hook.scopeId, deployment.id), | |
| ); | |
| return 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 moveRunbooksLinkedToHooksToNewSystem = async ( | |
| deployment: SCHEMA.Deployment, | |
| ) => { | |
| return db.transaction(async (tx) => { | |
| const isDeploymentHook = and( | |
| eq(SCHEMA.hook.scopeType, "deployment"), | |
| eq(SCHEMA.hook.scopeId, deployment.id), | |
| ); | |
| const hooks = await tx.query.hook | |
| .findMany({ | |
| where: isDeploymentHook, | |
| with: { runhooks: { with: { runbook: true } } }, | |
| }); | |
| const runbookIds = hooks.flatMap((h) => | |
| h.runhooks.map((rh) => rh.runbook.id), | |
| ); | |
| if (runbookIds.length === 0) return; | |
| return tx | |
| .update(SCHEMA.runbook) | |
| .set({ systemId: deployment.systemId }) | |
| .where(inArray(SCHEMA.runbook.id, runbookIds)); | |
| }).catch((error) => { | |
| console.error('Failed to move runbooks:', error); | |
| throw error; | |
| }); | |
| }; |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor