Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Deployment retry policy #248

Merged
merged 7 commits into from
Dec 4, 2024
Merged

fix: Deployment retry policy #248

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6a0b379
init'
adityachoudhari26 Dec 3, 2024
c3efad4
more
adityachoudhari26 Dec 3, 2024
5cead0b
style nit
adityachoudhari26 Dec 3, 2024
ec7fc59
nit
adityachoudhari26 Dec 3, 2024
fde49d3
move to dispatch package
adityachoudhari26 Dec 3, 2024
c220e0d
make generic updateJob func
adityachoudhari26 Dec 3, 2024
66067a9
fix links
adityachoudhari26 Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions apps/webservice/src/app/[workspaceSlug]/(app)/_components/CreateDeployment.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export const CreateDeploymentDialog: React.FC<{
name: "",
slug: "",
description: "",
retryCount: 0,
},
mode: "onSubmit",
});
Expand Down Expand Up @@ -183,6 +184,26 @@ export const CreateDeploymentDialog: React.FC<{
</FormItem>
)}
/>
<FormField
control={form.control}
name="retryCount"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Retry Count</FormLabel>
<FormControl>
<Input
type="number"
min={0}
step={1}
value={value}
onChange={(e) => onChange(e.target.valueAsNumber)}
className="w-16"
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
<FormRootError />
<DialogFooter>
<Button type="submit">Create</Button>
Expand Down
19 changes: 19 additions & 0 deletions apps/webservice/src/app/[workspaceSlug]/(app)/_components/EditDeploymentDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,25 @@ export const EditDeploymentDialog: React.FC<{
/>
<FormField
control={form.control}
name="retryCount"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Retry Count</FormLabel>
<FormControl>
<Input
type="number"
value={value}
onChange={(e) => onChange(e.target.valueAsNumber)}
min={0}
step={1}
className="w-16"
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
<FormField
name="id"
render={() => (
<FormItem>
Expand Down
21 changes: 21 additions & 0 deletions ...ceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/EditDeploymentSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,27 @@ export const EditDeploymentSection: React.FC<{
)}
/>

<FormField
control={form.control}
name="retryCount"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Retry Count</FormLabel>
<FormControl>
<Input
type="number"
value={value}
onChange={(e) => onChange(e.target.valueAsNumber)}
min={0}
step={1}
className="w-16"
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
Comment on lines +120 to +139
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Handle potential NaN values from e.target.valueAsNumber

When using e.target.valueAsNumber in the onChange handler, if the input is empty or invalid (e.g., the user deletes the content), it may result in NaN. This could cause issues with form validation and data submission. Consider adding a check to handle NaN values and default to 0 or an empty string.

Apply this diff to handle NaN values:

<FormControl>
  <Input
    type="number"
    value={value}
-   onChange={(e) => onChange(e.target.valueAsNumber)}
+   onChange={(e) => {
+     const newValue = e.target.valueAsNumber;
+     onChange(isNaN(newValue) ? 0 : newValue);
+   }}
    min={0}
    step={1}
    className="w-16"
  />
</FormControl>
📝 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.

Suggested change
<FormField
control={form.control}
name="retryCount"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Retry Count</FormLabel>
<FormControl>
<Input
type="number"
value={value}
onChange={(e) => onChange(e.target.valueAsNumber)}
min={0}
step={1}
className="w-16"
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>
<FormField
control={form.control}
name="retryCount"
render={({ field: { value, onChange } }) => (
<FormItem>
<FormLabel>Retry Count</FormLabel>
<FormControl>
<Input
type="number"
value={value}
onChange={(e) => {
const newValue = e.target.valueAsNumber;
onChange(isNaN(newValue) ? 0 : newValue);
}}
min={0}
step={1}
className="w-16"
/>
</FormControl>
<FormMessage />
</FormItem>
)}
/>


<FormRootError />

<Button
Expand Down
5 changes: 3 additions & 2 deletions apps/webservice/src/app/api/github/webhook/workflow/handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { WorkflowRunEvent } from "@octokit/webhooks-types";
import { and, eq, takeFirstOrNull } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import * as schema from "@ctrlplane/db/schema";
import { onJobCompletion } from "@ctrlplane/job-dispatch";
import { onJobCompletion, onJobFailure } from "@ctrlplane/job-dispatch";
import { JobStatus } from "@ctrlplane/validators/jobs";

type Conclusion = Exclude<WorkflowRunEvent["workflow_run"]["conclusion"], null>;
Expand Down Expand Up @@ -94,5 +94,6 @@ export const handleWorkflowWebhookEvent = async (event: WorkflowRunEvent) => {
set: { value: links },
});

if (job.status === JobStatus.Completed) return onJobCompletion(job);
if (status === JobStatus.Failure) await onJobFailure(job);
if (status === JobStatus.Completed) return onJobCompletion(job);
};
4 changes: 3 additions & 1 deletion apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
updateJob,
user,
} from "@ctrlplane/db/schema";
import { onJobCompletion } from "@ctrlplane/job-dispatch";
import { onJobCompletion, onJobFailure } from "@ctrlplane/job-dispatch";
import { variablesAES256 } from "@ctrlplane/secrets";
import { Permission } from "@ctrlplane/validators/auth";
import { JobStatus } from "@ctrlplane/validators/jobs";
Expand Down Expand Up @@ -162,5 +162,7 @@ export const PATCH = async (
if (je.status === JobStatus.Completed)
onJobCompletion(je).catch(console.error);

if (je.status === JobStatus.Failure) onJobFailure(je).catch(console.error);

return NextResponse.json(je);
};
8 changes: 8 additions & 0 deletions packages/api/src/router/job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
getRolloutDateForReleaseJobTrigger,
isDateInTimeWindow,
onJobCompletion,
onJobFailure,
} from "@ctrlplane/job-dispatch";
import { Permission } from "@ctrlplane/validators/auth";
import { jobCondition, JobStatus } from "@ctrlplane/validators/jobs";
Expand Down Expand Up @@ -548,6 +549,13 @@ export const jobRouter = createTRPCRouter({
job.status === JobStatus.Completed
)
onJobCompletion(job);

if (
input.data.status === JobStatus.Failure &&
job.status === JobStatus.Failure
)
onJobFailure(job);

return job;
}),
),
Expand Down
2 changes: 2 additions & 0 deletions packages/db/drizzle/0044_sturdy_silver_centurion.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TYPE "release_job_trigger_type" ADD VALUE 'retry';--> statement-breakpoint
ALTER TABLE "deployment" ADD COLUMN "retry_count" integer DEFAULT 0 NOT NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider the migration strategy for adding a NOT NULL column with a default value

Adding a new NOT NULL column with a default value to a large table can lead to performance issues due to a full table rewrite and locking. To mitigate this, consider adding the column without the NOT NULL constraint and default, backfilling the data, and then altering the column to set the constraint and default.

Apply this refactored migration strategy:

-- Step 1: Add the column as nullable without a default
ALTER TABLE "deployment" ADD COLUMN "retry_count" integer;

-- Step 2: Backfill existing rows with the default value
UPDATE "deployment" SET "retry_count" = 0 WHERE "retry_count" IS NULL;

-- Step 3: Alter the column to set NOT NULL constraint and default value
ALTER TABLE "deployment" ALTER COLUMN "retry_count" SET NOT NULL;
ALTER TABLE "deployment" ALTER COLUMN "retry_count" SET DEFAULT 0;

Loading
Loading