diff --git a/apps/web/app/lib/plan-sections.ts b/apps/web/app/lib/plan-sections.ts new file mode 100644 index 0000000000..9ea1a459f3 --- /dev/null +++ b/apps/web/app/lib/plan-sections.ts @@ -0,0 +1,63 @@ +import yaml from "js-yaml"; + +export type PlanSection = { name: string; content: string }; + +const isArgoApplication = (doc: unknown): boolean => { + const d = doc as { apiVersion?: unknown; kind?: unknown } | null; + return ( + typeof d?.apiVersion === "string" && + d.apiVersion.startsWith("argoproj.io/") && + d.kind === "Application" + ); +}; + +function tryParseYamlStream(stream: string): unknown[] | null { + try { + return yaml + .loadAll(stream) + .filter( + (d) => + d != null && (typeof d !== "object" || Object.keys(d).length > 0), + ); + } catch { + return null; + } +} + +export function extractPlanSections(stream: string): PlanSection[] { + if (!stream.trim()) return []; + + const docs = tryParseYamlStream(stream); + if (docs == null) return [{ name: "Rendered Manifests", content: stream }]; + + const firstIsCR = docs.length > 0 && isArgoApplication(docs[0]); + const cr = firstIsCR ? [docs[0]] : []; + const manifests = firstIsCR ? docs.slice(1) : docs; + + return [ + ...(cr.length > 0 + ? [ + { + name: "Application CR", + content: cr.map((d) => yaml.dump(d)).join("---\n"), + }, + ] + : []), + ...(manifests.length > 0 + ? [ + { + name: "Rendered Manifests", + content: manifests.map((d) => yaml.dump(d)).join("---\n"), + }, + ] + : []), + ]; +} + +export function unionSectionNames(...streams: string[]): string[] { + const names = new Set(); + for (const s of streams) { + for (const sec of extractPlanSections(s)) names.add(sec.name); + } + return [...names]; +} diff --git a/apps/web/app/routes/ws/deployments/_components/plans/PlanDetailPageHeader.tsx b/apps/web/app/routes/ws/deployments/_components/plans/PlanDetailPageHeader.tsx new file mode 100644 index 0000000000..ca0977e938 --- /dev/null +++ b/apps/web/app/routes/ws/deployments/_components/plans/PlanDetailPageHeader.tsx @@ -0,0 +1,68 @@ +import { Link, useParams } from "react-router"; + +import { trpc } from "~/api/trpc"; +import { + Breadcrumb, + BreadcrumbItem, + BreadcrumbList, + BreadcrumbPage, + BreadcrumbSeparator, +} from "~/components/ui/breadcrumb"; +import { Separator } from "~/components/ui/separator"; +import { SidebarTrigger } from "~/components/ui/sidebar"; +import { useWorkspace } from "~/components/WorkspaceProvider"; +import { useDeployment } from "../DeploymentProvider"; +import { DeploymentsNavbarTabs } from "../DeploymentsNavbarTabs"; + +export function PlanDetailPageHeader() { + const { workspace } = useWorkspace(); + const { deployment } = useDeployment(); + const { planId } = useParams<{ planId: string }>(); + + const resultsQuery = trpc.deployment.plans.results.useQuery( + { deploymentId: deployment.id, planId: planId! }, + { enabled: !!planId }, + ); + const version = resultsQuery.data?.version; + + return ( +
+
+ + + + + + Deployments + + + + + {deployment.name} + + + + + + Plans + + + + + {version?.name ?? version?.tag ?? planId} + + + +
+ +
+ +
+
+ ); +} diff --git a/apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx b/apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx index 6edd3bc5d6..182eda8323 100644 --- a/apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx +++ b/apps/web/app/routes/ws/deployments/_components/plans/PlanDiffDialog.tsx @@ -1,9 +1,7 @@ -import type { RouterOutputs } from "@ctrlplane/trpc"; -import { useEffect, useState } from "react"; -import { DiffEditor } from "@monaco-editor/react"; +import { useState } from "react"; +import { useParams } from "react-router"; import { trpc } from "~/api/trpc"; -import { useTheme } from "~/components/ThemeProvider"; import { Badge } from "~/components/ui/badge"; import { Dialog, @@ -14,174 +12,72 @@ import { import { Label } from "~/components/ui/label"; import { Switch } from "~/components/ui/switch"; import { Tabs, TabsList, TabsTrigger } from "~/components/ui/tabs"; +import { extractPlanSections, unionSectionNames } from "~/lib/plan-sections"; -type PlanDiffDialogProps = { - deploymentId: string; - resultId: string | undefined; - title: string; - open: boolean; - initialTab?: TopTab; - onOpenChange: (open: boolean) => void; -}; +import { usePlanResultParam } from "../../_hooks/usePlanResultParam"; +import { useDeployment } from "../DeploymentProvider"; +import { DiffEditorPane } from "./plan-diff/DiffEditorPane"; +import { SectionSelector } from "./plan-diff/SectionSelector"; -type TopTab = "diff" | "validations"; type DiffView = "split" | "unified"; -type Validation = - RouterOutputs["deployment"]["plans"]["resultValidations"][number]; - -function ValidationsTab({ - deploymentId, - resultId, - open, -}: { - deploymentId: string; - resultId: string; - open: boolean; -}) { - const query = trpc.deployment.plans.resultValidations.useQuery( - { deploymentId, resultId }, - { enabled: open }, - ); - - if (query.isLoading) - return ( -
- Loading validations... -
- ); - - const validations = query.data ?? []; - if (validations.length === 0) - return ( -
- No validations evaluated for this result -
- ); +export function PlanDiffDialog() { + const { deployment } = useDeployment(); + const { planId } = useParams<{ planId: string }>(); + const { resultId, tab, setTab, closeResult } = usePlanResultParam(); + const [view, setView] = useState("unified"); + const [selected, setSelected] = useState(); - return ( -
-
    - {validations.map((v) => ( - - ))} -
-
+ const resultsQuery = trpc.deployment.plans.results.useQuery( + { deploymentId: deployment.id, planId: planId! }, + { enabled: !!planId && resultId != null }, ); -} - -function ValidationItem({ validation }: { validation: Validation }) { - return ( -
  • -
    - - {validation.passed ? "Passed" : "Failed"} - - {validation.ruleName} -
    - {validation.ruleDescription && ( -

    - {validation.ruleDescription} -

    - )} - {validation.violations.length > 0 && ( -
      - {validation.violations.map((violation, i) => ( -
    • - {violation.message} -
      • - ))} -
    - )} -
    • + const activeResult = resultsQuery.data?.items.find( + (r) => r.resultId === resultId, ); -} - -function DiffTab({ - deploymentId, - resultId, - open, - view, -}: { - deploymentId: string; - resultId: string; - open: boolean; - view: DiffView; -}) { - const { theme } = useTheme(); const diffQuery = trpc.deployment.plans.resultDiff.useQuery( - { deploymentId, resultId }, - { enabled: open }, + { deploymentId: deployment.id, resultId: resultId ?? "" }, + { enabled: resultId != null && tab === "diff" }, ); - if (diffQuery.isLoading) - return ( -
    - Loading diff... -
    - ); - - if (diffQuery.data == null) - return ( -
    - No diff available -
    - ); - - return ( - + const sectionNames = unionSectionNames( + diffQuery.data?.current ?? "", + diffQuery.data?.proposed ?? "", ); -} + const activeSection = + selected != null && sectionNames.includes(selected) + ? selected + : sectionNames[0]; -export function PlanDiffDialog({ - deploymentId, - resultId, - title, - open, - initialTab = "diff", - onOpenChange, -}: PlanDiffDialogProps) { - const [tab, setTab] = useState(initialTab); - const [view, setView] = useState("unified"); - - useEffect(() => { - if (open) setTab(initialTab); - }, [open, initialTab]); + const title = activeResult + ? `${activeResult.environment.name} · ${activeResult.resource.name} · ${activeResult.agent.name}` + : ""; return ( - + { + if (!o) closeResult(); + }} + > - {title} +
    + {title} + +
    {tab === "diff" && (
    - setView(checked ? "split" : "unified") - } + onCheckedChange={(c) => setView(c ? "split" : "unified")} />
    )} - setTab(v as TopTab)}> + setTab(v as "diff" | "validations")}> Diff Validations @@ -200,18 +96,18 @@ export function PlanDiffDialog({
    - {resultId == null ? null : tab === "diff" ? ( - - ) : ( - )}
    @@ -219,3 +115,104 @@ export function PlanDiffDialog({     ); } + +function DiffBody({ + isLoading, + data, + activeSection, + view, +}: { + isLoading: boolean; + data: { current: string; proposed: string } | null | undefined; + activeSection: string | undefined; + view: DiffView; +}) { + if (isLoading) + return ( +
    + Loading diff... +
    + ); + if (data == null) + return ( +
    + No diff available +
    + ); + + const currentSection = extractPlanSections(data.current).find( + (s) => s.name === activeSection, + ); + const proposedSection = extractPlanSections(data.proposed).find( + (s) => s.name === activeSection, + ); + return ( + + ); +} + +function ValidationsBody({ + deploymentId, + resultId, +}: { + deploymentId: string; + resultId: string; +}) { + const query = trpc.deployment.plans.resultValidations.useQuery({ + deploymentId, + resultId, + }); + + if (query.isLoading) + return ( +
    + Loading validations... +
    + ); + + const validations = query.data ?? []; + if (validations.length === 0) + return ( +
    + No validations evaluated for this result +
    + ); + + return ( +
    +
      + {validations.map((v) => ( +
    • +
      + + {v.passed ? "Passed" : "Failed"} + + {v.ruleName} +
      + {v.ruleDescription && ( +

      + {v.ruleDescription} +

      + )} + {v.violations.length > 0 && ( +
        + {v.violations.map((violation, i) => ( +
      • + {violation.message} +
        • + ))} +
      + )} +
      • + ))} +
    +
    + ); +} diff --git a/apps/web/app/routes/ws/deployments/_components/plans/plan-diff/DiffEditorPane.tsx b/apps/web/app/routes/ws/deployments/_components/plans/plan-diff/DiffEditorPane.tsx new file mode 100644 index 0000000000..49bed657af --- /dev/null +++ b/apps/web/app/routes/ws/deployments/_components/plans/plan-diff/DiffEditorPane.tsx @@ -0,0 +1,37 @@ +import { DiffEditor } from "@monaco-editor/react"; + +import { useTheme } from "~/components/ThemeProvider"; + +export function DiffEditorPane({ + current, + proposed, + view, +}: { + current: string; + proposed: string; + view: "split" | "unified"; +}) { + const { theme } = useTheme(); + return ( + + ); +} diff --git a/apps/web/app/routes/ws/deployments/_components/plans/plan-diff/SectionSelector.tsx b/apps/web/app/routes/ws/deployments/_components/plans/plan-diff/SectionSelector.tsx new file mode 100644 index 0000000000..c71255c3b2 --- /dev/null +++ b/apps/web/app/routes/ws/deployments/_components/plans/plan-diff/SectionSelector.tsx @@ -0,0 +1,35 @@ +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from "~/components/ui/select"; + +export type SectionSelectorProps = { + sections: string[]; + value: string | undefined; + onChange: (value: string) => void; +}; + +export function SectionSelector({ + sections, + value, + onChange, +}: SectionSelectorProps) { + if (sections.length <= 1) return null; + return ( + + ); +} diff --git a/apps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.ts b/apps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.ts index 57451b23c9..12e03933e3 100644 --- a/apps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.ts +++ b/apps/web/app/routes/ws/deployments/_hooks/usePlanResultParam.ts @@ -1,20 +1,32 @@ import { useSearchParams } from "react-router"; +export type PlanResultTab = "diff" | "validations"; + export function usePlanResultParam() { const [searchParams, setSearchParams] = useSearchParams(); const resultId = searchParams.get("resultId") ?? undefined; + const tab: PlanResultTab = + searchParams.get("tab") === "validations" ? "validations" : "diff"; - const openResult = (id: string) => { + const openResult = (id: string, nextTab: PlanResultTab = "diff") => { const newParams = new URLSearchParams(searchParams); newParams.set("resultId", id); + newParams.set("tab", nextTab); + setSearchParams(newParams); + }; + + const setTab = (nextTab: PlanResultTab) => { + const newParams = new URLSearchParams(searchParams); + newParams.set("tab", nextTab); setSearchParams(newParams); }; const closeResult = () => { const newParams = new URLSearchParams(searchParams); newParams.delete("resultId"); + newParams.delete("tab"); setSearchParams(newParams); }; - return { resultId, openResult, closeResult }; + return { resultId, tab, openResult, setTab, closeResult }; } diff --git a/apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx b/apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx index 929c97dac9..efd77465c7 100644 --- a/apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx +++ b/apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx @@ -1,18 +1,8 @@ import type { RouterOutputs } from "@ctrlplane/trpc"; -import { useState } from "react"; import { FileText } from "lucide-react"; -import { Link, useParams } from "react-router"; +import { useParams } from "react-router"; import { trpc } from "~/api/trpc"; -import { - Breadcrumb, - BreadcrumbItem, - BreadcrumbList, - BreadcrumbPage, - BreadcrumbSeparator, -} from "~/components/ui/breadcrumb"; -import { Separator } from "~/components/ui/separator"; -import { SidebarTrigger } from "~/components/ui/sidebar"; import { Table, TableBody, @@ -21,10 +11,9 @@ import { TableHeader, TableRow, } from "~/components/ui/table"; -import { useWorkspace } from "~/components/WorkspaceProvider"; import { cn } from "~/lib/utils"; import { useDeployment } from "./_components/DeploymentProvider"; -import { DeploymentsNavbarTabs } from "./_components/DeploymentsNavbarTabs"; +import { PlanDetailPageHeader } from "./_components/plans/PlanDetailPageHeader"; import { PlanDiffDialog } from "./_components/plans/PlanDiffDialog"; import { PlanStatusBadge } from "./_components/plans/PlanStatusBadge"; import { usePlanResultParam } from "./_hooks/usePlanResultParam"; @@ -38,10 +27,6 @@ export function meta() { type Result = RouterOutputs["deployment"]["plans"]["results"]["items"][number]; -function resultTitle(result: Result) { - return `${result.environment.name} · ${result.resource.name} · ${result.agent.name}`; -} - function DiffStats({ stats, }: { @@ -183,69 +168,20 @@ function NoResults() { } export default function DeploymentPlanDetail() { - const { workspace } = useWorkspace(); const { deployment } = useDeployment(); const { planId } = useParams<{ planId: string }>(); - const { resultId, openResult, closeResult } = usePlanResultParam(); - const [initialTab, setInitialTab] = useState<"diff" | "validations">("diff"); + const { openResult } = usePlanResultParam(); const resultsQuery = trpc.deployment.plans.results.useQuery( { deploymentId: deployment.id, planId: planId! }, { enabled: !!planId, refetchInterval: 5000 }, ); - const version = resultsQuery.data?.version; const results = resultsQuery.data?.items ?? []; - const activeResult = results.find((r) => r.resultId === resultId); - - const handleOpenResult = ( - id: string, - tab: "diff" | "validations" = "diff", - ) => { - setInitialTab(tab); - openResult(id); - }; return ( <> -
    -
    - - - - - - Deployments - - - - - {deployment.name} - - - - - - Plans - - - - - {version?.name ?? version?.tag ?? planId} - - - -
    - -
    - -
    -
    + {results.length === 0 && !resultsQuery.isLoading ? ( @@ -257,23 +193,14 @@ export default function DeploymentPlanDetail() { ))} )} - { - if (!o) closeResult(); - }} - /> + ); } diff --git a/apps/workspace-engine/pkg/jobagents/argo/application_upserter.go b/apps/workspace-engine/pkg/jobagents/argo/application_upserter.go index 90ed777106..fd052ad8e8 100644 --- a/apps/workspace-engine/pkg/jobagents/argo/application_upserter.go +++ b/apps/workspace-engine/pkg/jobagents/argo/application_upserter.go @@ -151,3 +151,27 @@ func (g *GoManifestGetter) GetManifests( } return resp.Manifests, nil } + +// GoApplicationGetter is the production implementation of +// ApplicationGetter that calls the ArgoCD API. +type GoApplicationGetter struct{} + +func (g *GoApplicationGetter) GetApplication( + ctx context.Context, + serverAddr, apiKey, appName string, +) (*v1alpha1.Application, error) { + client, err := argocdclient.NewClient(&argocdclient.ClientOptions{ + ServerAddr: serverAddr, + AuthToken: apiKey, + }) + if err != nil { + return nil, fmt.Errorf("create ArgoCD client: %w", err) + } + ioCloser, appClient, err := client.NewApplicationClient() + if err != nil { + return nil, fmt.Errorf("create application client: %w", err) + } + defer ioCloser.Close() + + return appClient.Get(ctx, &argocdapplication.ApplicationQuery{Name: &appName}) +} diff --git a/apps/workspace-engine/pkg/jobagents/argo/argoapp_test.go b/apps/workspace-engine/pkg/jobagents/argo/argoapp_test.go index 62eef6e122..6ff6e0ed8f 100644 --- a/apps/workspace-engine/pkg/jobagents/argo/argoapp_test.go +++ b/apps/workspace-engine/pkg/jobagents/argo/argoapp_test.go @@ -106,6 +106,20 @@ func (m *mockManifestGetter) GetManifests( return nil, nil } +type mockApplicationGetter struct { + fn func(ctx context.Context, serverAddr, apiKey, appName string) (*v1alpha1.Application, error) +} + +func (m *mockApplicationGetter) GetApplication( + ctx context.Context, + serverAddr, apiKey, appName string, +) (*v1alpha1.Application, error) { + if m.fn != nil { + return m.fn(ctx, serverAddr, apiKey, appName) + } + return nil, nil +} + // --- helpers --- func testJob() *oapi.Job { @@ -332,11 +346,33 @@ func planManifestGetter(current, proposed []string) *mockManifestGetter { } } +func planAppGetter(dispatchCtx *oapi.DispatchContext) *mockApplicationGetter { + return &mockApplicationGetter{ + fn: func(_ context.Context, _, _, _ string) (*v1alpha1.Application, error) { + _, _, template, err := ParseJobAgentConfig(dispatchCtx.JobAgentConfig) + if err != nil { + return nil, err + } + app, err := TemplateApplication(dispatchCtx, template) + if err != nil { + return nil, err + } + MakeApplicationK8sCompatible(app) + return app, nil + }, + } +} + func TestPlan_HasChanges(t *testing.T) { current := []string{`{"kind":"Deployment","metadata":{"name":"app"},"spec":{"replicas":1}}`} proposed := []string{`{"kind":"Deployment","metadata":{"name":"app"},"spec":{"replicas":3}}`} - p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, planManifestGetter(current, proposed)) + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(current, proposed), + &mockApplicationGetter{}, + ) result, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.NoError(t, err) @@ -349,7 +385,12 @@ func TestPlan_HasChanges(t *testing.T) { func TestPlan_NoChanges(t *testing.T) { manifests := []string{`{"kind":"Deployment","metadata":{"name":"app"},"spec":{"replicas":1}}`} - p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, planManifestGetter(manifests, manifests)) + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(manifests, manifests), + planAppGetter(testDispatchCtx()), + ) result, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.NoError(t, err) @@ -368,7 +409,12 @@ func TestPlan_MultipleManifests(t *testing.T) { `{"kind":"Service","metadata":{"name":"web"}}`, } - p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, planManifestGetter(current, proposed)) + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(current, proposed), + &mockApplicationGetter{}, + ) result, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.NoError(t, err) @@ -377,11 +423,114 @@ func TestPlan_MultipleManifests(t *testing.T) { assert.Contains(t, result.Proposed, "---\n") } +func TestPlan_ProposedAlwaysContainsApplicationCR(t *testing.T) { + manifests := []string{`{"kind":"Deployment","metadata":{"name":"app"}}`} + + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(manifests, manifests), + &mockApplicationGetter{}, + ) + + result, err := p.Plan(context.Background(), testDispatchCtx(), nil) + require.NoError(t, err) + assert.True(t, strings.HasPrefix(result.Proposed, "apiVersion: argoproj.io/v1alpha1\n"), + "proposed should begin with the Application CR") + assert.Contains(t, result.Proposed, "kind: Application") + crEnd := strings.Index(result.Proposed, "\n---\n") + deploymentIdx := strings.Index(result.Proposed, "kind: Deployment") + require.GreaterOrEqual(t, crEnd, 0, "expected `---` separator between CR and manifests") + require.GreaterOrEqual(t, deploymentIdx, 0, "expected manifest content after separator") + assert.Less(t, crEnd, deploymentIdx, "CR should come before manifests in the stream") +} + +func TestPlan_CurrentContainsApplicationCR_WhenLiveAppExists(t *testing.T) { + manifests := []string{`{"kind":"Deployment","metadata":{"name":"app"}}`} + + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(manifests, manifests), + planAppGetter(testDispatchCtx()), + ) + + result, err := p.Plan(context.Background(), testDispatchCtx(), nil) + require.NoError(t, err) + assert.True(t, strings.HasPrefix(result.Current, "apiVersion: argoproj.io/v1alpha1\n"), + "current should begin with the live Application CR") +} + +func TestPlan_CurrentOmitsCR_OnFirstDeploy(t *testing.T) { + manifests := []string{`{"kind":"Deployment","metadata":{"name":"app"}}`} + + // mockApplicationGetter defaults to returning (nil, nil) — represents + // a release target whose Application doesn't exist in ArgoCD yet. + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(manifests, manifests), + &mockApplicationGetter{}, + ) + + result, err := p.Plan(context.Background(), testDispatchCtx(), nil) + require.NoError(t, err) + assert.NotContains(t, result.Current, "apiVersion: argoproj.io/v1alpha1", + "first-time deploys should have no CR on the current side") + assert.True(t, result.HasChanges, + "adding the Application CR should register as a change") +} + +func TestPlan_CurrentApplicationNotFound_IsNotFatal(t *testing.T) { + manifests := []string{`{"kind":"Deployment","metadata":{"name":"app"}}`} + getter := &mockApplicationGetter{ + fn: func(_ context.Context, _, _, _ string) (*v1alpha1.Application, error) { + return nil, status.Error(codes.NotFound, "application not found") + }, + } + + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(manifests, manifests), + getter, + ) + + result, err := p.Plan(context.Background(), testDispatchCtx(), nil) + require.NoError(t, err) + assert.NotContains(t, result.Current, "apiVersion: argoproj.io/v1alpha1") +} + +func TestPlan_CurrentApplicationError_IsFatal(t *testing.T) { + manifests := []string{`{"kind":"Deployment","metadata":{"name":"app"}}`} + getter := &mockApplicationGetter{ + fn: func(_ context.Context, _, _, _ string) (*v1alpha1.Application, error) { + return nil, fmt.Errorf("argocd unavailable") + }, + } + + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(manifests, manifests), + getter, + ) + + _, err := p.Plan(context.Background(), testDispatchCtx(), nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "get current application") +} + func TestPlan_ContentHashDeterministic(t *testing.T) { current := []string{`{"kind":"ConfigMap","metadata":{"name":"cfg"}}`} proposed := []string{`{"kind":"ConfigMap","metadata":{"name":"cfg"},"data":{"k":"v"}}`} - p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, planManifestGetter(current, proposed)) + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(current, proposed), + &mockApplicationGetter{}, + ) r1, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.NoError(t, err) @@ -393,7 +542,12 @@ func TestPlan_ContentHashDeterministic(t *testing.T) { } func TestPlan_BadConfig(t *testing.T) { - p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, &mockManifestGetter{}) + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + &mockManifestGetter{}, + &mockApplicationGetter{}, + ) dctx := testDispatchCtx() dctx.JobAgentConfig = oapi.JobAgentConfig{} @@ -406,6 +560,7 @@ func TestPlan_UpsertFailure(t *testing.T) { &mockUpserter{err: fmt.Errorf("conflict")}, &mockDeleter{}, &mockManifestGetter{}, + &mockApplicationGetter{}, ) _, err := p.Plan(context.Background(), testDispatchCtx(), nil) @@ -422,7 +577,7 @@ func TestPlan_GetProposedManifestsFailure_ReturnsIncomplete(t *testing.T) { return []string{"manifest"}, nil }, } - p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, getter) + p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, getter, &mockApplicationGetter{}) result, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.NoError(t, err) @@ -448,7 +603,7 @@ func TestPlan_GetCurrentManifestsNotFound_FirstVersion(t *testing.T) { }, } deleter := &mockDeleter{} - p := NewArgoCDPlanner(&mockUpserter{}, deleter, getter) + p := NewArgoCDPlanner(&mockUpserter{}, deleter, getter, &mockApplicationGetter{}) result, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.NoError(t, err) @@ -474,7 +629,7 @@ func TestPlan_GetCurrentManifestsFailure(t *testing.T) { return nil, fmt.Errorf("current app not found") }, } - p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, getter) + p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, getter, &mockApplicationGetter{}) _, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.Error(t, err) @@ -487,7 +642,7 @@ func TestPlan_EmptyManifests_ReturnsIncomplete(t *testing.T) { return nil, nil }, } - p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, getter) + p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, getter, &mockApplicationGetter{}) result, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.NoError(t, err) @@ -512,7 +667,7 @@ func TestPlan_ManifestTimeout_Exhausted_WithError(t *testing.T) { }, } deleter := &mockDeleter{} - p := NewArgoCDPlanner(&mockUpserter{}, deleter, getter) + p := NewArgoCDPlanner(&mockUpserter{}, deleter, getter, &mockApplicationGetter{}) expired := time.Now().Add(-manifestTimeout - time.Second) state, _ := json.Marshal(argoPlanState{ @@ -535,7 +690,7 @@ func TestPlan_ManifestTimeout_Exhausted_EmptyManifests(t *testing.T) { }, } deleter := &mockDeleter{} - p := NewArgoCDPlanner(&mockUpserter{}, deleter, getter) + p := NewArgoCDPlanner(&mockUpserter{}, deleter, getter, &mockApplicationGetter{}) expired := time.Now().Add(-manifestTimeout - time.Second) state, _ := json.Marshal(argoPlanState{ @@ -554,7 +709,12 @@ func TestPlan_ManifestTimeout_Exhausted_EmptyManifests(t *testing.T) { func TestPlan_ManifestTimeout_SucceedsBeforeExpiry(t *testing.T) { current := []string{`{"kind":"Deployment"}`} proposed := []string{`{"kind":"Deployment","spec":{"replicas":2}}`} - p := NewArgoCDPlanner(&mockUpserter{}, &mockDeleter{}, planManifestGetter(current, proposed)) + p := NewArgoCDPlanner( + &mockUpserter{}, + &mockDeleter{}, + planManifestGetter(current, proposed), + &mockApplicationGetter{}, + ) recent := time.Now().Add(-30 * time.Second) state, _ := json.Marshal(argoPlanState{ @@ -574,7 +734,12 @@ func TestPlan_DeletesTemporaryAppOnSuccess(t *testing.T) { proposed := []string{`{"kind":"Deployment","spec":{"replicas":2}}`} deleter := &mockDeleter{} - p := NewArgoCDPlanner(&mockUpserter{}, deleter, planManifestGetter(current, proposed)) + p := NewArgoCDPlanner( + &mockUpserter{}, + deleter, + planManifestGetter(current, proposed), + &mockApplicationGetter{}, + ) _, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.NoError(t, err) @@ -595,7 +760,7 @@ func TestPlan_CurrentManifestError_DeletesTmpApp(t *testing.T) { }, } - p := NewArgoCDPlanner(&mockUpserter{}, deleter, getter) + p := NewArgoCDPlanner(&mockUpserter{}, deleter, getter, &mockApplicationGetter{}) _, err := p.Plan(context.Background(), testDispatchCtx(), nil) require.Error(t, err) @@ -612,6 +777,7 @@ func TestPlan_DeleteNotCalledOnUpsertFailure(t *testing.T) { &mockUpserter{err: fmt.Errorf("conflict")}, deleter, &mockManifestGetter{}, + &mockApplicationGetter{}, ) _, err := p.Plan(context.Background(), testDispatchCtx(), nil) diff --git a/apps/workspace-engine/pkg/jobagents/argo/argocd.go b/apps/workspace-engine/pkg/jobagents/argo/argocd.go index 2a5a42c7be..8be9935221 100644 --- a/apps/workspace-engine/pkg/jobagents/argo/argocd.go +++ b/apps/workspace-engine/pkg/jobagents/argo/argocd.go @@ -53,6 +53,14 @@ type ManifestGetter interface { GetManifests(ctx context.Context, serverAddr, apiKey, appName string) ([]string, error) } +// ApplicationGetter fetches an ArgoCD Application by name. +type ApplicationGetter interface { + GetApplication( + ctx context.Context, + serverAddr, apiKey, appName string, + ) (*v1alpha1.Application, error) +} + var ( _ types.Dispatchable = &ArgoApplication{} _ types.Verifiable = &ArgoApplication{} diff --git a/apps/workspace-engine/pkg/jobagents/argo/argocd_plan.go b/apps/workspace-engine/pkg/jobagents/argo/argocd_plan.go index 71a158055b..43c74d1388 100644 --- a/apps/workspace-engine/pkg/jobagents/argo/argocd_plan.go +++ b/apps/workspace-engine/pkg/jobagents/argo/argocd_plan.go @@ -14,11 +14,38 @@ import ( "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" sigsyaml "sigs.k8s.io/yaml" "workspace-engine/pkg/jobagents/types" "workspace-engine/pkg/oapi" ) +// normalizeApplicationForDiff zeroes server-managed and operational fields +// and pins TypeMeta so the live Application from ArgoCD and a freshly-templated +// proposed Application can be diffed cleanly. Without that, the diff is +// dominated by managedFields, status, creationTimestamp, etc. that aren't +// user intent — and the live Application returned by ArgoCD's typed client +// has empty TypeMeta, which would otherwise show as a phantom diff against +// the YAML-templated proposed. +func normalizeApplicationForDiff(app *v1alpha1.Application) *v1alpha1.Application { + if app == nil { + return nil + } + cp := app.DeepCopy() + cp.TypeMeta = metav1.TypeMeta{ + APIVersion: "argoproj.io/v1alpha1", + Kind: "Application", + } + cp.ObjectMeta.ManagedFields = nil + cp.ObjectMeta.CreationTimestamp = metav1.Time{} + cp.ObjectMeta.Generation = 0 + cp.ObjectMeta.ResourceVersion = "" + cp.ObjectMeta.UID = "" + cp.Status = v1alpha1.ApplicationStatus{} + cp.Operation = nil + return cp +} + const ( planLabelKey = "ctrlplane.dev/plan" planCreatedAtKey = "ctrlplane.dev/plan-created-at" @@ -31,9 +58,10 @@ var _ types.Plannable = (*ArgoCDPlanner)(nil) // application, comparing its rendered manifests to the current application, // and cleaning up. It implements [types.Plannable]. type ArgoCDPlanner struct { - upserter ApplicationUpserter - deleter ApplicationDeleter - manifestGetter ManifestGetter + upserter ApplicationUpserter + deleter ApplicationDeleter + manifestGetter ManifestGetter + applicationGetter ApplicationGetter } // NewArgoCDPlanner creates an ArgoCDPlanner with the given dependencies. @@ -41,11 +69,13 @@ func NewArgoCDPlanner( upserter ApplicationUpserter, deleter ApplicationDeleter, manifestGetter ManifestGetter, + applicationGetter ApplicationGetter, ) *ArgoCDPlanner { return &ArgoCDPlanner{ - upserter: upserter, - deleter: deleter, - manifestGetter: manifestGetter, + upserter: upserter, + deleter: deleter, + manifestGetter: manifestGetter, + applicationGetter: applicationGetter, } } @@ -197,6 +227,14 @@ func (p *ArgoCDPlanner) Plan( currentManifests = nil } + currentApp, err := p.applicationGetter.GetApplication(ctx, serverAddr, apiKey, originalName) + if err != nil { + if status.Code(err) != codes.NotFound { + return nil, fmt.Errorf("get current application: %w", err) + } + currentApp = nil + } + for i, m := range proposedManifests { proposedManifests[i] = strings.ReplaceAll(m, s.TmpAppName, originalName) } @@ -204,8 +242,24 @@ func (p *ArgoCDPlanner) Plan( sort.Strings(currentManifests) sort.Strings(proposedManifests) - current := strings.Join(jsonManifestsToYAML(currentManifests), "---\n") - proposed := strings.Join(jsonManifestsToYAML(proposedManifests), "---\n") + proposedCRYAML, err := sigsyaml.Marshal(normalizeApplicationForDiff(proposedApp)) + if err != nil { + return nil, fmt.Errorf("marshal proposed application: %w", err) + } + proposedDocs := append( + []string{string(proposedCRYAML)}, + jsonManifestsToYAML(proposedManifests)...) + proposed := strings.Join(proposedDocs, "---\n") + + currentDocs := jsonManifestsToYAML(currentManifests) + if currentApp != nil { + currentCRYAML, err := sigsyaml.Marshal(normalizeApplicationForDiff(currentApp)) + if err != nil { + return nil, fmt.Errorf("marshal current application: %w", err) + } + currentDocs = append([]string{string(currentCRYAML)}, currentDocs...) + } + current := strings.Join(currentDocs, "---\n") hasChanges := current != proposed contentHash := sha256.Sum256([]byte(current + proposed)) diff --git a/apps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.go b/apps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.go index 1ac210f5c2..29df16f44d 100644 --- a/apps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.go +++ b/apps/workspace-engine/svc/controllers/deploymentplanresult/getters_postgres.go @@ -115,6 +115,7 @@ func newRegistry() *jobagents.Registry { &argo.GoApplicationUpserter{}, &argo.GoApplicationDeleter{}, &argo.GoManifestGetter{}, + &argo.GoApplicationGetter{}, ), ) registry.Register(testrunner.New(nil))