fix: Updating release channels will trigger and cancel jobs #274
Conversation
WalkthroughThis pull request introduces comprehensive changes to the handling of release environments and channels across multiple files. Modifications include the removal of certain properties and queries, the addition of new functions for managing release channel updates, and a refactor of UI components to streamline the rendering logic based on deployment states. These changes enhance the overall management of blocked environments and deployment statuses, affecting both frontend components and backend processing logic. Changes
Possibly Related PRs
Suggested Reviewers
Poem
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: 0
🧹 Nitpick comments (7)
packages/job-dispatch/src/environment-release-channel-update.ts (3)
35-36: Remove duplicate condition
Lines 35 and 36 both checkif (oldReleaseFilter == null && newReleaseFilter == null) return null;. This is redundant. Consider removing one of them.if (oldReleaseFilter == null && newReleaseFilter == null) return null; - if (oldReleaseFilter == null && newReleaseFilter == null) return null; if (oldReleaseFilter == null && newReleaseFilter != null) return {
208-250: Consider environment-level transaction
When updating release channels across multiple deployments, partial failures could occur. If a deployment fails, consider how to roll back or how to handle partial updates so that the environment remains consistent.
252-311: Handle environment-policy concurrency
This function updates release channels for every environment referencing the policy. Similar to the environment-level function, consider transaction boundaries or a backoff mechanism to ensure that simultaneous policy updates do not conflict.apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (2)
101-120: Consolidate conditional checks
Lines 101-115 render the already deployed release. Lines 116-120 render a deploy button if not deployed. Extracting this branching logic into a small helper function or clarifying the flow can improve readability.
121-137: Provide user-friendly messaging
When the environment is blocked by a release channel, you show a minimal message. Consider giving more context (e.g., “Your environment is restricted to specific releases. Please select an allowed channel to proceed.”), if that matches product requirements.packages/api/src/router/environment.ts (1)
Line range hint
365-388: Use transactions carefully
Here, you delete existing release channels and create new ones in the same block. It’s reasonable to ensure that all changes happen atomically. If further logic depends on successful updates of these channels, consider expanding the transaction boundaries.packages/api/src/router/environment-policy.ts (1)
428-449: Validate policy changes
After updating or clearing out channels, you gather them intoprevMapandnewMapand pass them tohandleEnvironmentPolicyReleaseChannelUpdate. Verify that partial updates do not leave any environment in a contradictory state—especially if new channels are set tonullwhile old ones remain in place.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx(0 hunks)packages/api/src/router/environment-policy.ts(3 hunks)packages/api/src/router/environment.ts(4 hunks)packages/job-dispatch/src/environment-release-channel-update.ts(1 hunks)packages/job-dispatch/src/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx
🧰 Additional context used
📓 Path-based instructions (5)
packages/job-dispatch/src/index.ts (1)
Pattern **/*.{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.
packages/api/src/router/environment-policy.ts (1)
Pattern **/*.{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.
packages/api/src/router/environment.ts (1)
Pattern **/*.{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.
packages/job-dispatch/src/environment-release-channel-update.ts (1)
Pattern **/*.{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)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (1)
Pattern **/*.{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.
🔇 Additional comments (12)
packages/job-dispatch/src/environment-release-channel-update.ts (3)
21-29: Clarify error handling in getReleaseFilter
This function may return a promise that could resolve to null. Be mindful to handle null values where getReleaseFilter is called, in case the specified release channel doesn't exist.
62-97: Ensure atomic cancellation of jobs
This function cancels pending jobs by updating their statuses. If partial write failures occur, some jobs might remain pending. Consider wrapping this logic in a transaction or verifying results more explicitly to avoid partial success.
138-181: Evaluate concurrency when filtering and triggering
The function updates or cancels old jobs, retrieves the latest releases, and triggers new jobs in parallel. Because each step returns early if certain conditions are unmet, there’s a low risk of race conditions, but ensure that upstream or downstream code does not assume these operations always occur in strict sequence.
packages/job-dispatch/src/index.ts (1)
4-4: Export statement organization
Exporting everything from environment-release-channel-update.js is a convenient way to integrate new functionalities. Just ensure there’s no naming conflict or unintentional re-export within the module.
apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/ReleaseEnvironmentCell.tsx (4)
15-15: Unify import style
You added JobStatus from @ctrlplane/validators/jobs. This is consistent with the rest of the imports. No issues noted.
40-46: Verify that release.blocked.useQuery returns valid data
Ensure that the blockedEnvsResult matches the environment’s ID type exactly. If there is any mismatch, blockedEnv might not be discovered properly.
90-93: Double-check status checks
You exclude blocked release channels only if there isn’t a job in progress. Confirm that this logic aligns with the desired UX—some users might expect the environment to remain blocked even if a job is currently in progress.
153-155: Edge case handling
This final fallback states “Release not deployed.” Ensure that this scenario logically excludes all blocked or in-progress states and that it doesn’t cause confusion about the environment’s real status.
packages/api/src/router/environment.ts (2)
344-352: Ensure consistent environment ID checks
You fetch the previous release channels by matching environmentReleaseChannel.environmentId to the input ID. Verify that the environment truly belongs to the user’s requested system or scope if multi-tenancy checks are critical.
390-407: Finalize release channel map changes
After the new maps are created, you call handleEnvironmentReleaseChannelUpdate with the environment ID. Confirm that any partial operation inside handleEnvironmentReleaseChannelUpdate won’t leave the environment in an inconsistent state if the function throws.
packages/api/src/router/environment-policy.ts (2)
33-33: Import for environment-policy release channel
The import of handleEnvironmentPolicyReleaseChannelUpdate from @ctrlplane/job-dispatch is consistent with the environment’s need to keep policy and environment data in sync. No issues noted.
381-388: Prevent accidental deletion
When fetching prevReleaseChannels, ensure that the environment-policy combination is valid. If this operation erroneously picks up an unrelated policy’s channels, channels could be unintentionally reset.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/job-dispatch/src/environment-release-channel-update.ts (4)
21-29: Ensure consistent return types and consider using async/await
getReleaseFiltercan return either an immediatenullor a Promise whenchannelIdis defined. For consistency, consider returning a promise in both cases by usingPromise.resolve(null)or rewriting the function asasync. This leads to a more predictable usage pattern across the codebase and ensures uniform error handling.-const getReleaseFilter = (channelId: string | null) => - channelId != null - ? db - .select() - .from(SCHEMA.releaseChannel) - .where(eq(SCHEMA.releaseChannel.id, channelId)) - .then(takeFirstOrNull) - .then((r) => r?.releaseFilter ?? null) - : null; +const getReleaseFilter = async (channelId: string | null): Promise<ReleaseCondition | null> => { + if (!channelId) { + return null; + } + const record = await db + .select() + .from(SCHEMA.releaseChannel) + .where(eq(SCHEMA.releaseChannel.id, channelId)) + .then(takeFirstOrNull); + return record?.releaseFilter ?? null; +};
31-59: Add explanatory comment for branching logic
The branching logic for creating the exclusion filter is clear but fairly dense. Adding a short inline comment explaining how each branch works (e.g., when old and new differ or are null) could improve maintainability and make debugging easier.
97-113: Consider using async/await for better readability
Similar togetReleaseFilter, using the async/await pattern here might improve readability and consistency across the code. Currently, the chained.then()calls are valid, but making it consistently async/await can streamline the style.
251-310: Policy-driven environment updates
The function effectively handles partial overrides by skipping updates for environments that have environment-level release channels. If environment-level overrides do not exist, it correctly delegates tohandleReleaseChannelUpdate. Consider combining common logic withhandleEnvironmentReleaseChannelUpdateif future changes require more uniform handling, but as it stands, this approach is acceptable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/job-dispatch/src/environment-release-channel-update.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/job-dispatch/src/environment-release-channel-update.ts (1)
Pattern **/*.{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.
🔇 Additional comments (6)
packages/job-dispatch/src/environment-release-channel-update.ts (6)
1-19: Imports and general organization look good
All the imported modules and libraries appear to be correctly referenced and semantically appropriate. The structure of the file, starting with imports, is coherent and well-structured.
61-95: Job cancellation logic is solid
This function cleanly composes a DB query to find and cancel the correct pending jobs. The early return on no filter and on no matching jobs is concise and optimal. This is well-structured and easy to read.
114-135: Release job trigger sequence appears correct
The sequence of creating triggers, applying policy checks, generating approvals, and ultimately dispatching these jobs is logically consistent. This function uses an early return when there are no triggers, avoiding unnecessary steps.
137-179: Covers primary channel-update cases effectively
The logic checks for no-op updates when old and new channels match, cancels excluded releases, and triggers jobs for newly matched releases. This ensures minimal redundant operations. The parallel invocation of cancellation and triggering (via Promise.all) is efficient. Overall, the approach here is .
181-206: Policy channel retrieval
This function is neatly structured to aggregate channel relationships by deploymentId. The use of Lodash for grouping and mapping is correctly applied and easy to follow.
207-249: Environment release-channel updates
Merging policy-level and environment-level channels, then iterating over deployment IDs is done cleanly. The promises are well-orchestrated. No immediate issues stand out.
| _.mapValues( | ||
| _.keyBy(rows, (r) => r.deploymentId), | ||
| (r) => r.channelId, | ||
| ), |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/api/src/router/environment.ts (1)
Line range hint
344-405: Consider adding error handling for the update operation.The update operation should handle potential errors and provide appropriate feedback.
.mutation(async ({ ctx, input }) => { + try { const prevReleaseChannels = await ctx.db // ... existing code ... await handleEnvironmentReleaseChannelUpdate(input.id, prevMap, newMap); + } catch (error) { + console.error(`Failed to update release channels for environment ${input.id}:`, error); + throw new Error(`Failed to update release channels: ${error instanceof Error ? error.message : 'Unknown error'}`); + } }),
🧹 Nitpick comments (4)
packages/job-dispatch/src/environment-release-channel-update.ts (4)
31-59: Consider simplifying the filter creation logic.The nested conditional structure could be simplified using early returns and combining similar conditions.
const createFilterForExcludedReleases = ( oldReleaseFilter: ReleaseCondition | null, newReleaseFilter: ReleaseCondition | null, ): ReleaseCondition | null => { - if (oldReleaseFilter == null && newReleaseFilter == null) return null; - if (oldReleaseFilter == null && newReleaseFilter != null) + if (oldReleaseFilter == null) { + return newReleaseFilter == null ? null : { + type: FilterType.Comparison, + not: true, + operator: ComparisonOperator.And, + conditions: [newReleaseFilter], + }; + } + if (newReleaseFilter == null) return null; + return { + type: FilterType.Comparison, + operator: ComparisonOperator.And, + conditions: [ + { + type: FilterType.Comparison, + not: true, + operator: ComparisonOperator.And, + conditions: [newReleaseFilter], + }, + oldReleaseFilter, + ], + }; - return { - type: FilterType.Comparison, - not: true, - operator: ComparisonOperator.And, - conditions: [newReleaseFilter], - }; - if (oldReleaseFilter != null && newReleaseFilter == null) return null; - if (oldReleaseFilter != null && newReleaseFilter != null) - return { - type: FilterType.Comparison, - operator: ComparisonOperator.And, - conditions: [ - { - type: FilterType.Comparison, - not: true, - operator: ComparisonOperator.And, - conditions: [newReleaseFilter], - }, - oldReleaseFilter, - ], - }; - return null; };
137-179: Add logging for better debugging and monitoring.Consider adding logging statements to track the progress and outcomes of channel updates.
const handleReleaseChannelUpdate = async ( environmentId: string, deploymentId: string, oldChannelId: string | null, newChannelId: string | null, ) => { + console.log(`Updating release channel for environment ${environmentId}, deployment ${deploymentId}`); if (oldChannelId === newChannelId) return; const [oldReleaseFilter, newReleaseFilter] = await Promise.all([ getReleaseFilter(oldChannelId), getReleaseFilter(newChannelId), ]); + console.log(`Old filter: ${JSON.stringify(oldReleaseFilter)}, New filter: ${JSON.stringify(newReleaseFilter)}`); const excludedReleasesFilter = createFilterForExcludedReleases( oldReleaseFilter, newReleaseFilter, ); const cancelJobsPromise = cancelJobsForExcludedReleases( environmentId, deploymentId, excludedReleasesFilter, ); const [latestReleaseMatchingNewFilter, latestReleaseMatchingOldFilter] = await Promise.all([ getLatestReleaseMatchingFilter(deploymentId, newReleaseFilter), getLatestReleaseMatchingFilter(deploymentId, oldReleaseFilter), ]); + console.log(`Latest releases - New: ${latestReleaseMatchingNewFilter?.id}, Old: ${latestReleaseMatchingOldFilter?.id}`); if ( latestReleaseMatchingNewFilter == null || latestReleaseMatchingNewFilter.id === latestReleaseMatchingOldFilter?.id ) return; const triggerJobsPromise = triggerJobsForRelease( environmentId, latestReleaseMatchingNewFilter.id, ); await Promise.all([cancelJobsPromise, triggerJobsPromise]); + console.log(`Completed release channel update for environment ${environmentId}`); };
181-205: Consider improving readability of data transformation.The lodash chain could be replaced with more readable array methods.
const getPolicyReleaseChannels = (environmentId: string) => db .select({ deploymentId: SCHEMA.environmentPolicyReleaseChannel.deploymentId, channelId: SCHEMA.environmentPolicyReleaseChannel.channelId, }) .from(SCHEMA.environment) .innerJoin( SCHEMA.environmentPolicy, eq(SCHEMA.environment.policyId, SCHEMA.environmentPolicy.id), ) .innerJoin( SCHEMA.environmentPolicyReleaseChannel, eq( SCHEMA.environmentPolicyReleaseChannel.policyId, SCHEMA.environmentPolicy.id, ), ) .where(eq(SCHEMA.environment.id, environmentId)) - .then((rows) => - _.mapValues( - _.keyBy(rows, (r) => r.deploymentId), - (r) => r.channelId, - ), + .then((rows) => + Object.fromEntries( + rows.map(row => [row.deploymentId, row.channelId]) + ), );
207-247: Add JSDoc comments and improve type safety.Consider adding documentation and type assertions for better maintainability.
+/** + * Updates release channels for an environment and handles associated job updates. + * @param environmentId - The ID of the environment to update + * @param prevReleaseChannels - Previous mapping of deployment IDs to channel IDs + * @param newReleaseChannels - New mapping of deployment IDs to channel IDs + * @returns Promise that resolves when all updates are complete + */ export const handleEnvironmentReleaseChannelUpdate = async ( environmentId: string, prevReleaseChannels: Record<string, string>, newReleaseChannels: Record<string, string>, ) => { + if (!environmentId) throw new Error('Environment ID is required'); const deploymentIds = await db .select() .from(SCHEMA.environment) // ... rest of the function
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/api/src/router/environment-policy.ts(3 hunks)packages/api/src/router/environment.ts(4 hunks)packages/job-dispatch/src/environment-release-channel-update.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/router/environment-policy.ts
🧰 Additional context used
📓 Path-based instructions (2)
packages/api/src/router/environment.ts (1)
Pattern **/*.{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.
packages/job-dispatch/src/environment-release-channel-update.ts (1)
Pattern **/*.{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.
| const cancelJobsForExcludedReleases = async ( | ||
| environmentId: string, | ||
| deploymentId: string, | ||
| excludedReleasesFilter: ReleaseCondition | null, | ||
| ) => { | ||
| if (excludedReleasesFilter == null) return; | ||
|
|
||
| const jobsToCancel = await db | ||
| .select() | ||
| .from(SCHEMA.job) | ||
| .innerJoin( | ||
| SCHEMA.releaseJobTrigger, | ||
| eq(SCHEMA.releaseJobTrigger.jobId, SCHEMA.job.id), | ||
| ) | ||
| .innerJoin( | ||
| SCHEMA.release, | ||
| eq(SCHEMA.releaseJobTrigger.releaseId, SCHEMA.release.id), | ||
| ) | ||
| .where( | ||
| and( | ||
| eq(SCHEMA.release.deploymentId, deploymentId), | ||
| eq(SCHEMA.releaseJobTrigger.environmentId, environmentId), | ||
| eq(SCHEMA.job.status, JobStatus.Pending), | ||
| SCHEMA.releaseMatchesCondition(db, excludedReleasesFilter), | ||
| ), | ||
| ) | ||
| .then((rows) => rows.map((r) => r.job.id)); | ||
|
|
||
| if (jobsToCancel.length === 0) return; | ||
|
|
||
| await db | ||
| .update(SCHEMA.job) | ||
| .set({ status: JobStatus.Cancelled }) | ||
| .where(inArray(SCHEMA.job.id, jobsToCancel)); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider wrapping job cancellation in a transaction.
The job cancellation operation should be atomic to prevent partial updates.
const cancelJobsForExcludedReleases = async (
environmentId: string,
deploymentId: string,
excludedReleasesFilter: ReleaseCondition | null,
) => {
if (excludedReleasesFilter == null) return;
+ return db.transaction(async (tx) => {
const jobsToCancel = await db
.select()
.from(SCHEMA.job)
.innerJoin(
SCHEMA.releaseJobTrigger,
eq(SCHEMA.releaseJobTrigger.jobId, SCHEMA.job.id),
)
.innerJoin(
SCHEMA.release,
eq(SCHEMA.releaseJobTrigger.releaseId, SCHEMA.release.id),
)
.where(
and(
eq(SCHEMA.release.deploymentId, deploymentId),
eq(SCHEMA.releaseJobTrigger.environmentId, environmentId),
eq(SCHEMA.job.status, JobStatus.Pending),
SCHEMA.releaseMatchesCondition(db, excludedReleasesFilter),
),
)
.then((rows) => rows.map((r) => r.job.id));
if (jobsToCancel.length === 0) return;
- await db
+ await tx
.update(SCHEMA.job)
.set({ status: JobStatus.Cancelled })
.where(inArray(SCHEMA.job.id, jobsToCancel));
+ });
};📝 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 cancelJobsForExcludedReleases = async ( | |
| environmentId: string, | |
| deploymentId: string, | |
| excludedReleasesFilter: ReleaseCondition | null, | |
| ) => { | |
| if (excludedReleasesFilter == null) return; | |
| const jobsToCancel = await db | |
| .select() | |
| .from(SCHEMA.job) | |
| .innerJoin( | |
| SCHEMA.releaseJobTrigger, | |
| eq(SCHEMA.releaseJobTrigger.jobId, SCHEMA.job.id), | |
| ) | |
| .innerJoin( | |
| SCHEMA.release, | |
| eq(SCHEMA.releaseJobTrigger.releaseId, SCHEMA.release.id), | |
| ) | |
| .where( | |
| and( | |
| eq(SCHEMA.release.deploymentId, deploymentId), | |
| eq(SCHEMA.releaseJobTrigger.environmentId, environmentId), | |
| eq(SCHEMA.job.status, JobStatus.Pending), | |
| SCHEMA.releaseMatchesCondition(db, excludedReleasesFilter), | |
| ), | |
| ) | |
| .then((rows) => rows.map((r) => r.job.id)); | |
| if (jobsToCancel.length === 0) return; | |
| await db | |
| .update(SCHEMA.job) | |
| .set({ status: JobStatus.Cancelled }) | |
| .where(inArray(SCHEMA.job.id, jobsToCancel)); | |
| }; | |
| const cancelJobsForExcludedReleases = async ( | |
| environmentId: string, | |
| deploymentId: string, | |
| excludedReleasesFilter: ReleaseCondition | null, | |
| ) => { | |
| if (excludedReleasesFilter == null) return; | |
| return db.transaction(async (tx) => { | |
| const jobsToCancel = await db | |
| .select() | |
| .from(SCHEMA.job) | |
| .innerJoin( | |
| SCHEMA.releaseJobTrigger, | |
| eq(SCHEMA.releaseJobTrigger.jobId, SCHEMA.job.id), | |
| ) | |
| .innerJoin( | |
| SCHEMA.release, | |
| eq(SCHEMA.releaseJobTrigger.releaseId, SCHEMA.release.id), | |
| ) | |
| .where( | |
| and( | |
| eq(SCHEMA.release.deploymentId, deploymentId), | |
| eq(SCHEMA.releaseJobTrigger.environmentId, environmentId), | |
| eq(SCHEMA.job.status, JobStatus.Pending), | |
| SCHEMA.releaseMatchesCondition(db, excludedReleasesFilter), | |
| ), | |
| ) | |
| .then((rows) => rows.map((r) => r.job.id)); | |
| if (jobsToCancel.length === 0) return; | |
| await tx | |
| .update(SCHEMA.job) | |
| .set({ status: JobStatus.Cancelled }) | |
| .where(inArray(SCHEMA.job.id, jobsToCancel)); | |
| }); | |
| }; |
| export const handleEnvironmentPolicyReleaseChannelUpdate = async ( | ||
| policyId: string, | ||
| prevReleaseChannels: Record<string, string>, | ||
| newReleaseChannels: Record<string, string>, | ||
| ) => { | ||
| const environments = await db | ||
| .select() | ||
| .from(SCHEMA.environmentPolicy) | ||
| .innerJoin( | ||
| SCHEMA.environment, | ||
| eq(SCHEMA.environmentPolicy.id, SCHEMA.environment.policyId), | ||
| ) | ||
| .leftJoin( | ||
| SCHEMA.environmentReleaseChannel, | ||
| eq(SCHEMA.environmentReleaseChannel.environmentId, SCHEMA.environment.id), | ||
| ) | ||
| .where(eq(SCHEMA.environmentPolicy.id, policyId)) | ||
| .then((rows) => | ||
| _.chain(rows) | ||
| .groupBy((r) => r.environment.id) | ||
| .map((groupedRows) => ({ | ||
| environmentId: groupedRows[0]!.environment.id, | ||
| releaseChannels: groupedRows | ||
| .filter((row) => isPresent(row.environment_release_channel)) | ||
| .map((row) => row.environment_release_channel!), | ||
| })) | ||
| .value(), | ||
| ); | ||
|
|
||
| const deploymentIds = await db | ||
| .select() | ||
| .from(SCHEMA.environmentPolicy) | ||
| .innerJoin( | ||
| SCHEMA.deployment, | ||
| eq(SCHEMA.deployment.systemId, SCHEMA.environmentPolicy.systemId), | ||
| ) | ||
| .where(eq(SCHEMA.environmentPolicy.id, policyId)) | ||
| .then((rows) => rows.map((r) => r.deployment.id)); | ||
|
|
||
| const environmentReleaseChannelUpdatePromises = environments.flatMap( | ||
| ({ environmentId, releaseChannels }) => | ||
| deploymentIds.map((deploymentId) => { | ||
| const environmentLevelReleaseChannel = releaseChannels.find( | ||
| (channel) => channel.deploymentId === deploymentId, | ||
| ); | ||
| if (environmentLevelReleaseChannel != null) return; | ||
|
|
||
| const oldChannelId = prevReleaseChannels[deploymentId] ?? null; | ||
| const newChannelId = newReleaseChannels[deploymentId] ?? null; | ||
| return handleReleaseChannelUpdate( | ||
| environmentId, | ||
| deploymentId, | ||
| oldChannelId, | ||
| newChannelId, | ||
| ); | ||
| }), | ||
| ); | ||
|
|
||
| return Promise.all(environmentReleaseChannelUpdatePromises); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for empty environments list.
Consider adding a check for empty environments to prevent unnecessary processing.
export const handleEnvironmentPolicyReleaseChannelUpdate = async (
policyId: string,
prevReleaseChannels: Record<string, string>,
newReleaseChannels: Record<string, string>,
) => {
const environments = await db
// ... query code ...
.then((rows) =>
_.chain(rows)
.groupBy((r) => r.environment.id)
.map((groupedRows) => ({
environmentId: groupedRows[0]!.environment.id,
releaseChannels: groupedRows
.filter((row) => isPresent(row.environment_release_channel))
.map((row) => row.environment_release_channel!),
}))
.value(),
);
+ if (environments.length === 0) {
+ console.log(`No environments found for policy ${policyId}`);
+ return;
+ }
+
const deploymentIds = await db
// ... rest of the functionCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/job-dispatch/src/environment-release-channel-update.ts (2)
31-59: Consider simplifying the filter creation logic.The nested if conditions in
createFilterForExcludedReleasescould be simplified for better readability and maintainability.Consider this refactored version:
const createFilterForExcludedReleases = ( oldReleaseFilter: ReleaseCondition | null, newReleaseFilter: ReleaseCondition | null, ): ReleaseCondition | null => { - if (oldReleaseFilter == null && newReleaseFilter == null) return null; - if (oldReleaseFilter == null && newReleaseFilter != null) - return { - type: FilterType.Comparison, - not: true, - operator: ComparisonOperator.And, - conditions: [newReleaseFilter], - }; - if (oldReleaseFilter != null && newReleaseFilter == null) return null; - if (oldReleaseFilter != null && newReleaseFilter != null) - return { + if (!newReleaseFilter) return null; + + const excludeNewFilter = { + type: FilterType.Comparison, + not: true, + operator: ComparisonOperator.And, + conditions: [newReleaseFilter], + }; + + if (!oldReleaseFilter) return excludeNewFilter; + + return { type: FilterType.Comparison, operator: ComparisonOperator.And, conditions: [ - { - type: FilterType.Comparison, - not: true, - operator: ComparisonOperator.And, - conditions: [newReleaseFilter], - }, + excludeNewFilter, oldReleaseFilter, ], }; - return null; };
204-244: Add logging for better debugging and monitoring.Consider adding logging to track the progress and outcome of environment release channel updates.
Add logging statements at key points:
export const handleEnvironmentReleaseChannelUpdate = async ( environmentId: string, prevReleaseChannels: Record<string, string>, newReleaseChannels: Record<string, string>, ) => { + console.log(`Updating release channels for environment ${environmentId}`); const deploymentIds = await db // ... existing code ... .then((rows) => rows.map((r) => r.deployment.id)); + console.log(`Found ${deploymentIds.length} deployments to update`); // ... rest of the function + console.log(`Completed release channel updates for environment ${environmentId}`); return Promise.all(releaseChannelUpdatePromises); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/job-dispatch/src/environment-release-channel-update.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/job-dispatch/src/environment-release-channel-update.ts (1)
Pattern **/*.{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.
🔇 Additional comments (2)
packages/job-dispatch/src/environment-release-channel-update.ts (2)
61-95: Consider wrapping job cancellation in a transaction.
The job cancellation operation should be atomic to prevent partial updates.
246-305: Add error handling for empty environments list.
The function should handle the case where no environments are found for the policy.
Run this script to check if this edge case occurs in production:
Consider adding a guard clause:
export const handleEnvironmentPolicyReleaseChannelUpdate = async (
policyId: string,
prevReleaseChannels: Record<string, string>,
newReleaseChannels: Record<string, string>,
) => {
const environments = await db
// ... existing query ...
.then((rows) => // ... existing transformation ...);
+ if (environments.length === 0) {
+ console.log(`No environments found for policy ${policyId}`);
+ return;
+ }✅ Verification successful
Empty environments list is a valid case that should be handled gracefully
Based on the codebase analysis, I recommend adding error handling for empty environments, but with a slight modification to the suggested approach:
export const handleEnvironmentPolicyReleaseChannelUpdate = async (
policyId: string,
prevReleaseChannels: Record<string, string>,
newReleaseChannels: Record<string, string>,
) => {
const environments = await db
// ... existing query ...
.then((rows) => // ... existing transformation ...);
+ if (environments.length === 0) {
+ return Promise.resolve([]); // Return empty array to maintain consistent return type
+ }
const deploymentIds = await db
// ... rest of the function- The function returns
Promise<void[]>(fromPromise.all()of empty promises), so we should maintain that contract by returning an empty array when no environments are found - This is a valid case that can occur when a policy exists but has no environments associated with it yet
- Similar patterns of handling empty results are seen in other parts of the codebase, particularly in API routes and job dispatch functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for policies without environments
# Expected: No results indicate this is not a common case
ast-grep --pattern 'eq(SCHEMA.environmentPolicy.id, policyId)' | \
xargs -I {} rg -A 5 -B 5 {} | \
grep -B 5 'rows.length === 0'
Length of output: 130
Script:
#!/bin/bash
# Let's check how environments are handled in the codebase, particularly error cases
rg -B 5 -A 5 "environments\.length === 0|environments\.length == 0|!environments\.length"
# Also check for any error handling patterns in similar functions
ast-grep --pattern 'if ($rows.length === 0) {
$$$
}'
# Let's also check for any test cases that might handle empty environments
rg -l "test|spec|describe.*environment.*policy"
Length of output: 8828
Script:
#!/bin/bash
# Let's check the test files for this specific function and related error cases
rg -l "handleEnvironmentPolicyReleaseChannelUpdate" packages/job-dispatch/src/__test__/
# Also check if there are any error logs or error handling in similar functions in the same file
rg "console\.(error|warn|log)" packages/job-dispatch/src/environment-release-channel-update.ts
# Check for any error handling patterns in similar database queries in the codebase
ast-grep --pattern 'const $_ = await db.select().from($_).where($_).then($_)'
Length of output: 9792
| const triggerJobsForRelease = async ( | ||
| environmentId: string, | ||
| releaseId: string, | ||
| ) => { | ||
| const releaseJobTriggers = await createReleaseJobTriggers( | ||
| db, | ||
| "new_environment", | ||
| ) | ||
| .environments([environmentId]) | ||
| .releases([releaseId]) | ||
| .filter(isPassingReleaseStringCheckPolicy) | ||
| .then(createJobApprovals) | ||
| .insert(); | ||
|
|
||
| if (releaseJobTriggers.length === 0) return; | ||
|
|
||
| await dispatchReleaseJobTriggers(db) | ||
| .releaseTriggers(releaseJobTriggers) | ||
| .filter(isPassingAllPoliciesExceptNewerThanLastActive) | ||
| .then(cancelOldReleaseJobTriggersOnJobDispatch) | ||
| .dispatch(); | ||
| }; |
There was a problem hiding this comment.
Potential race condition in job trigger creation and dispatch.
There's a potential race condition between creating job triggers and dispatching them. If another process modifies the triggers between creation and dispatch, it could lead to inconsistent behavior.
Consider wrapping the entire operation in a transaction:
const triggerJobsForRelease = async (
environmentId: string,
releaseId: string,
) => {
+ return db.transaction(async (tx) => {
const releaseJobTriggers = await createReleaseJobTriggers(
- db,
+ tx,
"new_environment",
)
.environments([environmentId])
.releases([releaseId])
.filter(isPassingReleaseStringCheckPolicy)
.then(createJobApprovals)
.insert();
if (releaseJobTriggers.length === 0) return;
- await dispatchReleaseJobTriggers(db)
+ await dispatchReleaseJobTriggers(tx)
.releaseTriggers(releaseJobTriggers)
.filter(isPassingAllPoliciesExceptNewerThanLastActive)
.then(cancelOldReleaseJobTriggersOnJobDispatch)
.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.
| const triggerJobsForRelease = async ( | |
| environmentId: string, | |
| releaseId: string, | |
| ) => { | |
| const releaseJobTriggers = await createReleaseJobTriggers( | |
| db, | |
| "new_environment", | |
| ) | |
| .environments([environmentId]) | |
| .releases([releaseId]) | |
| .filter(isPassingReleaseStringCheckPolicy) | |
| .then(createJobApprovals) | |
| .insert(); | |
| if (releaseJobTriggers.length === 0) return; | |
| await dispatchReleaseJobTriggers(db) | |
| .releaseTriggers(releaseJobTriggers) | |
| .filter(isPassingAllPoliciesExceptNewerThanLastActive) | |
| .then(cancelOldReleaseJobTriggersOnJobDispatch) | |
| .dispatch(); | |
| }; | |
| const triggerJobsForRelease = async ( | |
| environmentId: string, | |
| releaseId: string, | |
| ) => { | |
| return db.transaction(async (tx) => { | |
| const releaseJobTriggers = await createReleaseJobTriggers( | |
| tx, | |
| "new_environment", | |
| ) | |
| .environments([environmentId]) | |
| .releases([releaseId]) | |
| .filter(isPassingReleaseStringCheckPolicy) | |
| .then(createJobApprovals) | |
| .insert(); | |
| if (releaseJobTriggers.length === 0) return; | |
| await dispatchReleaseJobTriggers(tx) | |
| .releaseTriggers(releaseJobTriggers) | |
| .filter(isPassingAllPoliciesExceptNewerThanLastActive) | |
| .then(cancelOldReleaseJobTriggersOnJobDispatch) | |
| .dispatch(); | |
| }); | |
| }; |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation