fix: Target redeploy #210
fix: Target redeploy #210
Conversation
WalkthroughThe pull request introduces a new dialog component, 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: 5
🧹 Outside diff range and nitpick comments (4)
packages/job-dispatch/src/index.ts (1)
25-25: Consider grouping related exports together.The pending-job-checker export appears to be job-related functionality. Consider moving it near other job-related exports (around line 4) to maintain logical grouping in the file.
export * from "./config.js"; export * from "./release-job-trigger.js"; export * from "./job-creation.js"; export * from "./job-dispatch.js"; +export * from "./pending-job-checker.js"; export * from "./policy-checker.js"; export * from "./policy-create.js"; export * from "./release-sequencing.js"; export * from "./gradual-rollout.js"; export * from "./new-target.js"; export * from "./target.js"; export * from "./lock-checker.js"; export * from "./queue.js"; export { isDateInTimeWindow } from "./utils.js"; export * from "./policies/gradual-rollout.js"; export * from "./policies/manual-approval.js"; export * from "./policies/release-sequencing.js"; export * from "./policies/success-rate-criteria-passing.js"; export * from "./policies/release-dependency.js"; export * from "./policies/release-string-check.js"; export * from "./policies/concurrency-policy.js"; export * from "./policies/release-window.js"; export * from "./environment-creation.js"; -export * from "./pending-job-checker.js";apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetActionsDropdown.tsx (2)
121-127: Enhance loading state feedback.The button shows a disabled state but could provide better visual feedback during redeployment.
Consider adding a loading spinner:
<AlertDialogAction onClick={onRedeploy} className={buttonVariants({ variant: "default" })} disabled={redeployTarget.isPending} > - Redeploy + {redeployTarget.isPending ? ( + <> + <IconLoader className="mr-2 h-4 w-4 animate-spin" /> + Redeploying... + </> + ) : ( + 'Redeploy' + )} </AlertDialogAction>
152-158: Enhance accessibility for the redeploy menu item.While the implementation is functionally correct, consider adding ARIA attributes for better accessibility.
Add appropriate ARIA attributes:
<DropdownMenuItem className="flex cursor-pointer items-center gap-2" onSelect={(e) => e.preventDefault()} + aria-label="Redeploy target" + role="menuitem" > <IconRefresh className="h-4 w-4" /> Redeploy </DropdownMenuItem>packages/api/src/router/target.ts (1)
541-564: Refactorredeployprocedure usingasync/awaitfor better readabilityThe current implementation uses nested promise chains with multiple
.then()calls, which can make the code harder to read and maintain. Refactoring to useasync/awaitsyntax will enhance readability and simplify error handling.Here is the refactored code:
redeploy: protectedProcedure .input(z.string().uuid()) .meta({ authorizationCheck: ({ canUser, input }) => canUser .perform(Permission.TargetUpdate) .on({ type: "target", id: input }), }) - .mutation(({ ctx, input }) => - createReleaseJobTriggers(ctx.db, "redeploy") - .causedById(ctx.session.user.id) - .targets([input]) - .filter(isPassingReleaseStringCheckPolicy) - .filter(isPassingNoPendingJobsPolicy) - .then(createJobApprovals) - .insert() - .then((triggers) => - dispatchReleaseJobTriggers(ctx.db) - .releaseTriggers(triggers) - .filter(isPassingAllPoliciesExceptNewerThanLastActive) - .then(cancelOldReleaseJobTriggersOnJobDispatch) - .dispatch(), - ), - ), + .mutation(async ({ ctx, input }) => { + const triggers = await createReleaseJobTriggers(ctx.db, "redeploy") + .causedById(ctx.session.user.id) + .targets([input]) + .filter(isPassingReleaseStringCheckPolicy) + .filter(isPassingNoPendingJobsPolicy) + .then(createJobApprovals) + .insert(); + + await dispatchReleaseJobTriggers(ctx.db) + .releaseTriggers(triggers) + .filter(isPassingAllPoliciesExceptNewerThanLastActive) + .then(cancelOldReleaseJobTriggersOnJobDispatch) + .dispatch(); + }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetActionsDropdown.tsx(3 hunks)packages/api/src/router/target.ts(2 hunks)packages/job-dispatch/src/index.ts(1 hunks)packages/job-dispatch/src/pending-job-checker.ts(1 hunks)
🔇 Additional comments (3)
packages/job-dispatch/src/index.ts (1)
25-25: LGTM! Export statement follows established patterns.
The new export follows the consistent pattern of using .js extension and matches the barrel file's purpose of re-exporting functionality.
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetActionsDropdown.tsx (1)
96-96: Verify backend validation for target redeployment.
Since redeployment affects all systems and environments, we should ensure proper validation is in place.
✅ Verification successful
Target redeployment implementation has proper validation and security measures
The implementation in packages/api/src/router/target.ts shows that the redeployment mutation is properly secured with:
- Authorization check requiring
TargetUpdatepermission on the target - Multiple validation policies:
isPassingReleaseStringCheckPolicyisPassingNoPendingJobsPolicyisPassingAllPoliciesExceptNewerThanLastActive
- Proper job trigger management with cancellation of old triggers
The implementation follows a robust pattern of creating, validating, and dispatching release job triggers with appropriate security controls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validation checks in the target router implementation
ast-grep --pattern 'redeploy = protectedProcedure
.input($_)
.mutation(({ ctx, input }) => {
$$$
})'
# Look for any existing validation middleware
rg -A 5 'protectedProcedure.*redeploy'
Length of output: 153
Script:
#!/bin/bash
# Search for target router implementation files
fd "target.*router" --type f
# Search for redeploy mutation in any router file
rg -A 10 "redeploy.*=.*procedure" --type ts
# Look for any validation schemas related to target redeployment
rg -A 5 "z.object.*redeploy" --type ts
Length of output: 117
Script:
#!/bin/bash
# Search for any router files in the server/api directory
fd "router.ts" --type f
# Search for any files containing 'redeploy' in their content
rg -l "redeploy"
# Search for trpc procedure definitions containing redeploy
rg -A 10 "\.procedure.*redeploy" --type ts
Length of output: 2012
Script:
#!/bin/bash
# Check the target router implementation specifically
cat "packages/api/src/router/target.ts"
# Look for any validation schemas in the API package
rg "z.object" "packages/api/src"
Length of output: 21054
packages/api/src/router/target.ts (1)
18-26: Imports are correctly added
The imported functions from @ctrlplane/job-dispatch are appropriate and are utilized in the new redeploy procedure.
| children, | ||
| }) => { | ||
| const [open, setOpen] = useState(false); | ||
| const redeployTarget = api.target.redeploy.useMutation(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Invalidate relevant queries after redeployment.
After a successful redeployment, related queries should be invalidated to ensure UI consistency.
Add cache invalidation:
+ const utils = api.useUtils();
const redeployTarget = api.target.redeploy.useMutation({
+ onSuccess: () => {
+ utils.target.byWorkspaceId.list.invalidate();
+ utils.target.jobs.list.invalidate();
+ }
});📝 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 redeployTarget = api.target.redeploy.useMutation(); | |
| const utils = api.useUtils(); | |
| const redeployTarget = api.target.redeploy.useMutation({ | |
| onSuccess: () => { | |
| utils.target.byWorkspaceId.list.invalidate(); | |
| utils.target.jobs.list.invalidate(); | |
| } | |
| }); |
| const onRedeploy = () => | ||
| redeployTarget.mutateAsync(targetId).then(() => setOpen(false)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and success feedback for redeployment.
The onRedeploy function should handle errors and provide feedback to users about the operation's status.
Consider this implementation:
const onRedeploy = () =>
- redeployTarget.mutateAsync(targetId).then(() => setOpen(false));
+ redeployTarget.mutateAsync(targetId)
+ .then(() => {
+ toast.success('Target redeployment initiated successfully');
+ setOpen(false);
+ })
+ .catch((error) => {
+ toast.error(`Failed to redeploy target: ${error.message}`);
+ });📝 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 onRedeploy = () => | |
| redeployTarget.mutateAsync(targetId).then(() => setOpen(false)); | |
| const onRedeploy = () => | |
| redeployTarget.mutateAsync(targetId) | |
| .then(() => { | |
| toast.success('Target redeployment initiated successfully'); | |
| setOpen(false); | |
| }) | |
| .catch((error) => { | |
| toast.error(`Failed to redeploy target: ${error.message}`); | |
| }); |
| !rows.some( | ||
| (r) => | ||
| r.release_job_trigger.releaseId === w.releaseId && | ||
| r.release_job_trigger.targetId === w.targetId && | ||
| r.release_job_trigger.environmentId === w.environmentId, | ||
| ), |
There was a problem hiding this comment.
Correct property access in filter logic
In the .then clause, the properties are accessed using r.release_job_trigger.releaseId, but the query result may not nest properties under release_job_trigger. This could lead to undefined property errors.
Ensure that you access the properties directly from r if they are not nested. Adjust the property access as follows:
!rows.some(
(r) =>
- r.release_job_trigger.releaseId === w.releaseId &&
- r.release_job_trigger.targetId === w.targetId &&
- r.release_job_trigger.environmentId === w.environmentId,
+ r.releaseId === w.releaseId &&
+ r.targetId === w.targetId &&
+ r.environmentId === w.environmentId,
),Committable suggestion skipped: line range outside the PR's diff.
| or( | ||
| ...wf.map((w) => | ||
| and( | ||
| eq(SCHEMA.releaseJobTrigger.environmentId, w.environmentId), | ||
| eq(SCHEMA.releaseJobTrigger.releaseId, w.releaseId), | ||
| eq(SCHEMA.releaseJobTrigger.targetId, w.targetId), | ||
| or( | ||
| eq(SCHEMA.job.status, JobStatus.Pending), | ||
| eq(SCHEMA.job.status, JobStatus.InProgress), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Handle empty workflow arrays to prevent query errors
If the wf array is empty, the or(...wf.map(...)) condition will result in an empty or() call, which may produce an invalid SQL query or unintended behavior.
Add a check to handle the case when wf is empty to prevent potential errors. You can return early or adjust the query accordingly.
Apply this diff to handle empty workflows:
.where(
- or(
- ...wf.map((w) =>
- and(
- eq(SCHEMA.releaseJobTrigger.environmentId, w.environmentId),
- eq(SCHEMA.releaseJobTrigger.releaseId, w.releaseId),
- eq(SCHEMA.releaseJobTrigger.targetId, w.targetId),
- or(
- eq(SCHEMA.job.status, JobStatus.Pending),
- eq(SCHEMA.job.status, JobStatus.InProgress),
- ),
- ),
- ),
- ),
+ wf.length > 0
+ ? and(
+ inArray(SCHEMA.releaseJobTrigger.environmentId, wf.map((w) => w.environmentId)),
+ inArray(SCHEMA.releaseJobTrigger.releaseId, wf.map((w) => w.releaseId)),
+ inArray(SCHEMA.releaseJobTrigger.targetId, wf.map((w) => w.targetId)),
+ or(
+ eq(SCHEMA.job.status, JobStatus.Pending),
+ eq(SCHEMA.job.status, JobStatus.InProgress),
+ ),
+ )
+ : false,
)Alternatively, you can return the original wf array if it's empty:
+ if (wf.length === 0) {
+ return wf;
+ }📝 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.
| or( | |
| ...wf.map((w) => | |
| and( | |
| eq(SCHEMA.releaseJobTrigger.environmentId, w.environmentId), | |
| eq(SCHEMA.releaseJobTrigger.releaseId, w.releaseId), | |
| eq(SCHEMA.releaseJobTrigger.targetId, w.targetId), | |
| or( | |
| eq(SCHEMA.job.status, JobStatus.Pending), | |
| eq(SCHEMA.job.status, JobStatus.InProgress), | |
| ), | |
| ), | |
| ), | |
| ), | |
| ) | |
| wf.length > 0 | |
| ? and( | |
| inArray(SCHEMA.releaseJobTrigger.environmentId, wf.map((w) => w.environmentId)), | |
| inArray(SCHEMA.releaseJobTrigger.releaseId, wf.map((w) => w.releaseId)), | |
| inArray(SCHEMA.releaseJobTrigger.targetId, wf.map((w) => w.targetId)), | |
| or( | |
| eq(SCHEMA.job.status, JobStatus.Pending), | |
| eq(SCHEMA.job.status, JobStatus.InProgress), | |
| ), | |
| ) | |
| : false, |
| export const isPassingNoPendingJobsPolicy: ReleasePolicyChecker = async ( | ||
| db, | ||
| wf, | ||
| ) => | ||
| db | ||
| .selectDistinctOn([ | ||
| SCHEMA.releaseJobTrigger.releaseId, | ||
| SCHEMA.releaseJobTrigger.targetId, | ||
| SCHEMA.releaseJobTrigger.environmentId, | ||
| ]) | ||
| .from(SCHEMA.job) | ||
| .innerJoin( | ||
| SCHEMA.releaseJobTrigger, | ||
| eq(SCHEMA.releaseJobTrigger.jobId, SCHEMA.job.id), | ||
| ) | ||
| .where( | ||
| or( | ||
| ...wf.map((w) => | ||
| and( | ||
| eq(SCHEMA.releaseJobTrigger.environmentId, w.environmentId), | ||
| eq(SCHEMA.releaseJobTrigger.releaseId, w.releaseId), | ||
| eq(SCHEMA.releaseJobTrigger.targetId, w.targetId), | ||
| or( | ||
| eq(SCHEMA.job.status, JobStatus.Pending), | ||
| eq(SCHEMA.job.status, JobStatus.InProgress), | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| ) | ||
| .then((rows) => | ||
| wf.filter( | ||
| (w) => | ||
| !rows.some( | ||
| (r) => | ||
| r.release_job_trigger.releaseId === w.releaseId && | ||
| r.release_job_trigger.targetId === w.targetId && | ||
| r.release_job_trigger.environmentId === w.environmentId, | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize query performance for large workflow lists
The current implementation constructs an OR condition for each item in the wf array, which can lead to performance issues with large datasets due to the generation of a lengthy SQL query.
Consider refactoring the query to use IN clauses, which are more efficient for handling multiple values. This approach reduces the complexity of the SQL query and improves performance.
Apply this diff to optimize the query:
export const isPassingNoPendingJobsPolicy: ReleasePolicyChecker = async (
db,
wf,
) =>
+ {
+ const environmentIds = wf.map((w) => w.environmentId);
+ const releaseIds = wf.map((w) => w.releaseId);
+ const targetIds = wf.map((w) => w.targetId);
+
return db
.selectDistinctOn([
SCHEMA.releaseJobTrigger.releaseId,
SCHEMA.releaseJobTrigger.targetId,
SCHEMA.releaseJobTrigger.environmentId,
])
.from(SCHEMA.job)
.innerJoin(
SCHEMA.releaseJobTrigger,
eq(SCHEMA.releaseJobTrigger.jobId, SCHEMA.job.id),
)
.where(
- or(
- ...wf.map((w) =>
- and(
- eq(SCHEMA.releaseJobTrigger.environmentId, w.environmentId),
- eq(SCHEMA.releaseJobTrigger.releaseId, w.releaseId),
- eq(SCHEMA.releaseJobTrigger.targetId, w.targetId),
- or(
- eq(SCHEMA.job.status, JobStatus.Pending),
- eq(SCHEMA.job.status, JobStatus.InProgress),
- ),
- ),
- ),
- ),
+ and(
+ inArray(SCHEMA.releaseJobTrigger.environmentId, environmentIds),
+ inArray(SCHEMA.releaseJobTrigger.releaseId, releaseIds),
+ inArray(SCHEMA.releaseJobTrigger.targetId, targetIds),
+ or(
+ eq(SCHEMA.job.status, JobStatus.Pending),
+ eq(SCHEMA.job.status, JobStatus.InProgress),
+ ),
+ ),
)
.then((rows) =>
wf.filter(
(w) =>
!rows.some(
(r) =>
r.release_job_trigger.releaseId === w.releaseId &&
r.release_job_trigger.targetId === w.targetId &&
r.release_job_trigger.environmentId === w.environmentId,
),
),
);
+ }📝 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 isPassingNoPendingJobsPolicy: ReleasePolicyChecker = async ( | |
| db, | |
| wf, | |
| ) => | |
| db | |
| .selectDistinctOn([ | |
| SCHEMA.releaseJobTrigger.releaseId, | |
| SCHEMA.releaseJobTrigger.targetId, | |
| SCHEMA.releaseJobTrigger.environmentId, | |
| ]) | |
| .from(SCHEMA.job) | |
| .innerJoin( | |
| SCHEMA.releaseJobTrigger, | |
| eq(SCHEMA.releaseJobTrigger.jobId, SCHEMA.job.id), | |
| ) | |
| .where( | |
| or( | |
| ...wf.map((w) => | |
| and( | |
| eq(SCHEMA.releaseJobTrigger.environmentId, w.environmentId), | |
| eq(SCHEMA.releaseJobTrigger.releaseId, w.releaseId), | |
| eq(SCHEMA.releaseJobTrigger.targetId, w.targetId), | |
| or( | |
| eq(SCHEMA.job.status, JobStatus.Pending), | |
| eq(SCHEMA.job.status, JobStatus.InProgress), | |
| ), | |
| ), | |
| ), | |
| ), | |
| ) | |
| .then((rows) => | |
| wf.filter( | |
| (w) => | |
| !rows.some( | |
| (r) => | |
| r.release_job_trigger.releaseId === w.releaseId && | |
| r.release_job_trigger.targetId === w.targetId && | |
| r.release_job_trigger.environmentId === w.environmentId, | |
| ), | |
| ), | |
| ); | |
| export const isPassingNoPendingJobsPolicy: ReleasePolicyChecker = async ( | |
| db, | |
| wf, | |
| ) => | |
| { | |
| const environmentIds = wf.map((w) => w.environmentId); | |
| const releaseIds = wf.map((w) => w.releaseId); | |
| const targetIds = wf.map((w) => w.targetId); | |
| return db | |
| .selectDistinctOn([ | |
| SCHEMA.releaseJobTrigger.releaseId, | |
| SCHEMA.releaseJobTrigger.targetId, | |
| SCHEMA.releaseJobTrigger.environmentId, | |
| ]) | |
| .from(SCHEMA.job) | |
| .innerJoin( | |
| SCHEMA.releaseJobTrigger, | |
| eq(SCHEMA.releaseJobTrigger.jobId, SCHEMA.job.id), | |
| ) | |
| .where( | |
| and( | |
| inArray(SCHEMA.releaseJobTrigger.environmentId, environmentIds), | |
| inArray(SCHEMA.releaseJobTrigger.releaseId, releaseIds), | |
| inArray(SCHEMA.releaseJobTrigger.targetId, targetIds), | |
| or( | |
| eq(SCHEMA.job.status, JobStatus.Pending), | |
| eq(SCHEMA.job.status, JobStatus.InProgress), | |
| ), | |
| ), | |
| ) | |
| .then((rows) => | |
| wf.filter( | |
| (w) => | |
| !rows.some( | |
| (r) => | |
| r.release_job_trigger.releaseId === w.releaseId && | |
| r.release_job_trigger.targetId === w.targetId && | |
| r.release_job_trigger.environmentId === w.environmentId, | |
| ), | |
| ), | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/job-dispatch/src/pending-job-checker.ts(1 hunks)
🔇 Additional comments (2)
packages/job-dispatch/src/pending-job-checker.ts (2)
1-6: LGTM! Well-organized imports.
The imports are properly organized with clear separation between external and internal dependencies.
11-42: LGTM! Well-optimized query implementation.
The implementation shows good practices:
- Proper handling of empty workflow arrays
- Efficient use of
selectDistinctOn - Optimized query using
inArrayinstead of multiple OR conditions - Clear and maintainable query structure
| .then((rows) => | ||
| wf.filter( | ||
| (w) => | ||
| !rows.some( | ||
| (r) => | ||
| r.release_job_trigger.releaseId === w.releaseId && | ||
| r.release_job_trigger.targetId === w.targetId && | ||
| r.release_job_trigger.environmentId === w.environmentId, | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Property access in filter logic needs type safety improvements.
The code still uses nested property access r.release_job_trigger.releaseId which could lead to runtime errors if the structure is not as expected.
Apply this diff to make the property access more type-safe:
.then((rows) =>
wf.filter(
(w) =>
!rows.some(
(r) =>
- r.release_job_trigger.releaseId === w.releaseId &&
- r.release_job_trigger.targetId === w.targetId &&
- r.release_job_trigger.environmentId === w.environmentId,
+ r.releaseId === w.releaseId &&
+ r.targetId === w.targetId &&
+ r.environmentId === w.environmentId,
),
),
)Additionally, consider adding type assertions or runtime checks:
interface JobTriggerRow {
releaseId: string;
targetId: string;
environmentId: string;
}
// Add type guard
function isValidJobTrigger(row: unknown): row is JobTriggerRow {
return (
typeof row === 'object' &&
row !== null &&
'releaseId' in row &&
'targetId' in row &&
'environmentId' in row
);
}
// Use in the filter
!rows.some((r) => {
if (!isValidJobTrigger(r)) {
console.warn('Invalid job trigger row structure:', r);
return false;
}
return (
r.releaseId === w.releaseId &&
r.targetId === w.targetId &&
r.environmentId === w.environmentId
);
})
Summary by CodeRabbit
New Features
RedeployTargetDialogfor redeploying targets, accessible from the Target Actions Dropdown.IconRefresh.redeployprocedure in the target router.Bug Fixes
isPassingNoPendingJobsPolicyfunction.Documentation
pending-job-checker.jsmodule.