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 all 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
44 changes: 12 additions & 32 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
@@ -1,9 +1,10 @@
import type { WorkflowRunEvent } from "@octokit/webhooks-types";

import { and, eq, takeFirstOrNull } from "@ctrlplane/db";
import { 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 { updateJob } from "@ctrlplane/job-dispatch";
import { ReservedMetadataKey } from "@ctrlplane/validators/conditions";
import { JobStatus } from "@ctrlplane/validators/jobs";

type Conclusion = Exclude<WorkflowRunEvent["workflow_run"]["conclusion"], null>;
Expand Down Expand Up @@ -65,34 +66,13 @@ export const handleWorkflowWebhookEvent = async (event: WorkflowRunEvent) => {
: convertStatus(externalStatus);

const externalId = id.toString();
await db
.update(schema.job)
.set({ status, externalId })
.where(eq(schema.job.id, job.id));

const existingUrlMetadata = await db
.select()
.from(schema.jobMetadata)
.where(
and(
eq(schema.jobMetadata.jobId, job.id),
eq(schema.jobMetadata.key, "ctrlplane/links"),
),
)
.then(takeFirstOrNull);

const links = JSON.stringify({
...JSON.parse(existingUrlMetadata?.value ?? "{}"),
GitHub: `https://github.com/${repository.owner.login}/${repository.name}/actions/runs/${id}`,
});

await db
.insert(schema.jobMetadata)
.values([{ jobId: job.id, key: "ctrlplane/links", value: links }])
.onConflictDoUpdate({
target: [schema.jobMetadata.jobId, schema.jobMetadata.key],
set: { value: links },
});

if (job.status === JobStatus.Completed) return onJobCompletion(job);
await updateJob(
job.id,
{ status, externalId },
{
[String(ReservedMetadataKey.Links)]: {
GitHub: `https://github.com/${repository.owner.login}/${repository.name}/actions/runs/${id}`,
},
},
);
};
102 changes: 48 additions & 54 deletions 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 @@ -3,43 +3,31 @@ import { NextResponse } from "next/server";

import { and, eq, isNull, takeFirstOrNull } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import {
deployment,
environment,
environmentPolicyApproval,
job,
jobVariable,
release,
releaseJobTrigger,
resource,
resourceMetadata,
runbook,
runbookJobTrigger,
updateJob,
user,
} from "@ctrlplane/db/schema";
import { onJobCompletion } from "@ctrlplane/job-dispatch";
import * as schema from "@ctrlplane/db/schema";
import { updateJob } from "@ctrlplane/job-dispatch";
import { variablesAES256 } from "@ctrlplane/secrets";
import { Permission } from "@ctrlplane/validators/auth";
import { JobStatus } from "@ctrlplane/validators/jobs";

import { authn, authz } from "~/app/api/v1/auth";
import { request } from "~/app/api/v1/middleware";

type ApprovalJoinResult = {
environment_policy_approval: typeof environmentPolicyApproval.$inferSelect;
user: typeof user.$inferSelect | null;
environment_policy_approval: typeof schema.environmentPolicyApproval.$inferSelect;
user: typeof schema.user.$inferSelect | null;
};

const getApprovalDetails = async (releaseId: string, policyId: string) =>
db
.select()
.from(environmentPolicyApproval)
.leftJoin(user, eq(environmentPolicyApproval.userId, user.id))
.from(schema.environmentPolicyApproval)
.leftJoin(
schema.user,
eq(schema.environmentPolicyApproval.userId, schema.user.id),
)
.where(
and(
eq(environmentPolicyApproval.releaseId, releaseId),
eq(environmentPolicyApproval.policyId, policyId),
eq(schema.environmentPolicyApproval.releaseId, releaseId),
eq(schema.environmentPolicyApproval.policyId, policyId),
),
)
.then(takeFirstOrNull)
Expand Down Expand Up @@ -72,18 +60,38 @@ export const GET = request()
.handle<object, { params: { jobId: string } }>(async ({ db }, { params }) => {
const row = await db
.select()
.from(job)
.leftJoin(runbookJobTrigger, eq(runbookJobTrigger.jobId, job.id))
.leftJoin(runbook, eq(runbookJobTrigger.runbookId, runbook.id))
.leftJoin(releaseJobTrigger, eq(releaseJobTrigger.jobId, job.id))
.from(schema.job)
.leftJoin(
schema.runbookJobTrigger,
eq(schema.runbookJobTrigger.jobId, schema.job.id),
)
.leftJoin(
schema.runbook,
eq(schema.runbookJobTrigger.runbookId, schema.runbook.id),
)
.leftJoin(
schema.releaseJobTrigger,
eq(schema.releaseJobTrigger.jobId, schema.job.id),
)
.leftJoin(
schema.environment,
eq(schema.releaseJobTrigger.environmentId, schema.environment.id),
)
.leftJoin(
schema.resource,
eq(schema.releaseJobTrigger.resourceId, schema.resource.id),
)
.leftJoin(
schema.release,
eq(schema.releaseJobTrigger.releaseId, schema.release.id),
)
.leftJoin(
environment,
eq(environment.id, releaseJobTrigger.environmentId),
schema.deployment,
eq(schema.release.deploymentId, schema.deployment.id),
)
.where(
and(eq(schema.job.id, params.jobId), isNull(schema.resource.deletedAt)),
)
.leftJoin(resource, eq(resource.id, releaseJobTrigger.resourceId))
.leftJoin(release, eq(release.id, releaseJobTrigger.releaseId))
.leftJoin(deployment, eq(deployment.id, release.deploymentId))
.where(and(eq(job.id, params.jobId), isNull(resource.deletedAt)))
.then(takeFirstOrNull);

if (row == null)
Expand All @@ -110,8 +118,8 @@ export const GET = request()

const jobVariableRows = await db
.select()
.from(jobVariable)
.where(eq(jobVariable.jobId, params.jobId));
.from(schema.jobVariable)
.where(eq(schema.jobVariable.jobId, params.jobId));

const variables = Object.fromEntries(
jobVariableRows.map((v) => {
Expand All @@ -126,8 +134,8 @@ export const GET = request()

const metadata = await db
.select()
.from(resourceMetadata)
.where(eq(resourceMetadata.resourceId, je.resource.id))
.from(schema.resourceMetadata)
.where(eq(schema.resourceMetadata.resourceId, je.resource.id))
.then((rows) => Object.fromEntries(rows.map((m) => [m.key, m.value])));

return NextResponse.json({
Expand All @@ -137,7 +145,7 @@ export const GET = request()
});
});

const bodySchema = updateJob;
const bodySchema = schema.updateJob;

export const PATCH = async (
req: NextRequest,
Expand All @@ -146,21 +154,7 @@ export const PATCH = async (
const response = await req.json();
const body = bodySchema.parse(response);

const je = await db
.update(job)
.set(body)
.where(and(eq(job.id, params.jobId)))
.returning()
.then(takeFirstOrNull);

if (je == null)
return NextResponse.json(
{ error: "Job execution not found" },
{ status: 404 },
);

if (je.status === JobStatus.Completed)
onJobCompletion(je).catch(console.error);
const job = await updateJob(params.jobId, body);

return NextResponse.json(je);
return NextResponse.json(job);
Comment on lines +157 to +159
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

Add error handling for updateJob operation

The PATCH handler should include proper error handling for the updateJob operation, including handling of the "Job not found" error.

-  const job = await updateJob(params.jobId, body);
-
-  return NextResponse.json(job);
+  try {
+    const job = await updateJob(params.jobId, body);
+    return NextResponse.json(job);
+  } catch (error) {
+    if (error instanceof Error && error.message === "Job not found") {
+      return NextResponse.json(
+        { error: "Job not found" },
+        { status: 404 }
+      );
+    }
+    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.

Suggested change
const job = await updateJob(params.jobId, body);
return NextResponse.json(je);
return NextResponse.json(job);
try {
const job = await updateJob(params.jobId, body);
return NextResponse.json(job);
} catch (error) {
if (error instanceof Error && error.message === "Job not found") {
return NextResponse.json(
{ error: "Job not found" },
{ status: 404 }
);
}
throw error;
}

};
Loading
Loading