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.

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: show latest release by deployment and environment #169

Merged
merged 9 commits into from
Oct 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,13 @@ const ReleaseSelector: React.FC<{

const DeploymentNode: React.FC<
NodeProps<
Deployment & { latestRelease: { id: string; version: string } | null }
Deployment & { latestActiveRelease: { id: string; version: string } | null }
>
> = ({ data }) => {
const { getEdges, setEdges } = useReactFlow();

const [selectedRelease, setSelectedRelease] = useState(
data.latestRelease?.id,
data.latestActiveRelease?.id,
);
const releases = api.release.list.useQuery({ deploymentId: data.id });
const release = releases.data?.items.find((r) => r.id === selectedRelease);
Expand All @@ -163,7 +163,7 @@ const DeploymentNode: React.FC<
useEffect(() => {
if (release == null) return;
const deps = release.releaseDependencies.filter(isPresent);
if (data.latestRelease == null) return;
if (data.latestActiveRelease == null) return;

const edges = getEdges().filter((e) => e.source !== data.id);

Expand Down Expand Up @@ -193,7 +193,7 @@ const DeploymentNode: React.FC<
<div>
{releases.data != null && (
<ReleaseSelector
value={selectedRelease ?? data.latestRelease?.id ?? ""}
value={selectedRelease ?? data.latestActiveRelease?.id ?? ""}
onChange={setSelectedRelease}
releases={releases.data.items}
/>
Expand Down Expand Up @@ -222,7 +222,7 @@ const edgeTypes = { default: DepEdge };
const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms));
const DependencyDiagram: React.FC<{
deployments: Array<
Deployment & { latestRelease: { id: string; version: string } | null }
Deployment & { latestActiveRelease: { id: string; version: string } | null }
>;
}> = ({ deployments }) => {
const [nodes, _, onNodesChange] = useNodesState(
Expand Down Expand Up @@ -262,7 +262,7 @@ const DependencyDiagram: React.FC<{

export const Diagram: React.FC<{
deployments: Array<
Deployment & { latestRelease: { id: string; version: string } | null }
Deployment & { latestActiveRelease: { id: string; version: string } | null }
>;
}> = ({ deployments }) => {
return (
Expand Down
12 changes: 10 additions & 2 deletions apps/webservice/src/app/[workspaceSlug]/dependencies/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,21 @@ export default async function Dependencies({

if (
deployments.length === 0 ||
deployments.some((d) => d.latestRelease != null)
deployments.some((d) => d.latestActiveReleases != 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

Fix incorrect condition logic for showing getting started guide.

The current condition shows the getting started guide when there ARE active releases, which seems backwards. The getting started guide should typically be shown when there are no active releases.

Apply one of these fixes:

- deployments.some((d) => d.latestActiveReleases != null)
+ !deployments.some((d) => d.latestActiveReleases != null)

Or alternatively:

- deployments.some((d) => d.latestActiveReleases != null)
+ deployments.every((d) => d.latestActiveReleases == null)
📝 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
deployments.some((d) => d.latestActiveReleases != null)
!deployments.some((d) => d.latestActiveReleases != null)

)
return <DependenciesGettingStarted />;

const transformedDeployments = deployments.map((deployment) => ({
...deployment,
latestActiveRelease: deployment.latestActiveReleases && {
id: deployment.latestActiveReleases.id,
version: deployment.latestActiveReleases.version,
},
}));

return (
<div className="h-full">
<Diagram deployments={deployments} />
<Diagram deployments={transformedDeployments} />
</div>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,13 @@ const DeploymentTable: React.FC<{
environments: Array<Environment & { targets: Target[] }>;
deployments: Array<
Deployment & {
latestRelease: {
activeReleases: Array<{
id: string;
name: string;
version: string;
createdAt: Date;
} | null;
environmentId: string;
}> | null;
}
>;
workspaceSlug: string;
Expand Down Expand Up @@ -177,6 +178,9 @@ const DeploymentTable: React.FC<{
</td>

{environments.map((env, envIdx) => {
const release =
r.activeReleases?.find((r) => r.environmentId === env.id) ??
null;
return (
<td
key={env.id}
Expand All @@ -188,7 +192,7 @@ const DeploymentTable: React.FC<{
)}
>
<ReleaseCell
release={r.latestRelease}
release={release}
environment={env}
deployment={r}
workspaceSlug={workspaceSlug}
Expand Down
41 changes: 27 additions & 14 deletions packages/api/src/router/deployment.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import type { Tx } from "@ctrlplane/db";
import _ from "lodash";
import { isPresent } from "ts-is-present";
import { z } from "zod";

import {
Expand Down Expand Up @@ -30,7 +32,7 @@ import { JobStatus } from "@ctrlplane/validators/jobs";
import { createTRPCRouter, protectedProcedure } from "../trpc";
import { deploymentVariableRouter } from "./deployment-variable";

const latestReleaseSubQuery = (db: Tx) =>
const latestActiveReleaseSubQuery = (db: Tx) =>
db
.select({
id: release.id,
Expand All @@ -39,13 +41,15 @@ const latestReleaseSubQuery = (db: Tx) =>
createdAt: release.createdAt,
name: release.name,
config: release.config,
environmentId: releaseJobTrigger.environmentId,

rank: sql<number>`ROW_NUMBER() OVER (PARTITION BY deployment_id ORDER BY created_at DESC)`.as(
rank: sql<number>`ROW_NUMBER() OVER (PARTITION BY ${release.deploymentId}, ${releaseJobTrigger.environmentId} ORDER BY ${release.createdAt} DESC)`.as(
"rank",
),
})
.from(release)
.as("release");
.innerJoin(releaseJobTrigger, eq(releaseJobTrigger.releaseId, release.id))
.as("active_releases");

export const deploymentRouter = createTRPCRouter({
variable: deploymentVariableRouter,
Expand Down Expand Up @@ -203,20 +207,26 @@ export const deploymentRouter = createTRPCRouter({
.on({ type: "system", id: input }),
})
.query(async ({ ctx, input }) => {
const latestRelease = latestReleaseSubQuery(ctx.db);
const activeRelease = latestActiveReleaseSubQuery(ctx.db);
return ctx.db
.select()
.from(deployment)
.leftJoin(
latestRelease,
activeRelease,
and(
eq(latestRelease.deploymentId, deployment.id),
eq(latestRelease.rank, 1),
eq(activeRelease.deploymentId, deployment.id),
eq(activeRelease.rank, 1),
),
)
.where(eq(deployment.systemId, input))
.then((r) =>
r.map((row) => ({ ...row.deployment, latestRelease: row.release })),
.then((ts) =>
_.chain(ts)
.groupBy((t) => t.deployment.id)
.map((t) => ({
...t[0]!.deployment,
activeReleases: t.map((a) => a.active_releases).filter(isPresent),
}))
.value(),
Comment on lines +222 to +229
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

Handle potential empty groups to prevent runtime errors

The use of t[0]! assumes that each group has at least one element. This could lead to runtime errors if t is empty.

Apply this diff to add a null check:

            .groupBy((t) => t.deployment.id)
            .map((t) => ({
-              ...t[0]!.deployment,
+              ...(t[0]?.deployment ?? {}),
               activeReleases: t.map((a) => a.active_releases).filter(isPresent),
            }))
📝 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
.then((ts) =>
_.chain(ts)
.groupBy((t) => t.deployment.id)
.map((t) => ({
...t[0]!.deployment,
activeReleases: t.map((a) => a.active_releases).filter(isPresent),
}))
.value(),
.then((ts) =>
_.chain(ts)
.groupBy((t) => t.deployment.id)
.map((t) => ({
...(t[0]?.deployment ?? {}),
activeReleases: t.map((a) => a.active_releases).filter(isPresent),
}))
.value(),

);
}),

Expand Down Expand Up @@ -304,21 +314,24 @@ export const deploymentRouter = createTRPCRouter({
})
.input(z.string().uuid())
.query(async ({ ctx, input }) => {
const latestRelease = latestReleaseSubQuery(ctx.db);
const activeRelease = latestActiveReleaseSubQuery(ctx.db);
return ctx.db
.select()
.from(deployment)
.innerJoin(system, eq(system.id, deployment.systemId))
.leftJoin(
latestRelease,
activeRelease,
and(
eq(latestRelease.deploymentId, deployment.id),
eq(latestRelease.rank, 1),
eq(activeRelease.deploymentId, deployment.id),
eq(activeRelease.rank, 1),
),
)
.where(eq(system.workspaceId, input))
.then((r) =>
r.map((row) => ({ ...row.deployment, latestRelease: row.release })),
r.map((row) => ({
...row.deployment,
latestActiveReleases: row.active_releases,
})),
Comment on lines +331 to +334
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

Fix inconsistent property naming

The property name latestActiveReleases is inconsistent with activeReleases used in the bySystemId procedure. This could cause confusion and maintenance issues.

Apply this diff to maintain consistency:

          r.map((row) => ({
            ...row.deployment,
-           latestActiveReleases: row.active_releases,
+           activeReleases: row.active_releases,
          })),
📝 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
r.map((row) => ({
...row.deployment,
latestActiveReleases: row.active_releases,
})),
r.map((row) => ({
...row.deployment,
activeReleases: row.active_releases,
})),

);
}),
});
8 changes: 4 additions & 4 deletions packages/job-dispatch/src/release-job-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,17 +114,17 @@ class ReleaseJobTriggerBuilder {
}

async _values() {
const latestReleaseSubQuery = this._releaseSubQuery();
const latestActiveReleaseSubQuery = this._releaseSubQuery();
const releaseJobTriggers = this.releaseIds
? this._baseQuery().innerJoin(
release,
eq(release.deploymentId, deployment.id),
)
: this._baseQuery().innerJoin(
latestReleaseSubQuery,
latestActiveReleaseSubQuery,
and(
eq(latestReleaseSubQuery.deploymentId, deployment.id),
eq(latestReleaseSubQuery.rank, 1),
eq(latestActiveReleaseSubQuery.deploymentId, deployment.id),
eq(latestActiveReleaseSubQuery.rank, 1),
),
);

Expand Down
Loading