Skip to content

Commit 6dc8fc3

Browse files
aslilacaslilac
committed
fix: only allow promoting successful template versions
1 parent e8b84eb commit 6dc8fc3

File tree

11 files changed

+212
-36
lines changed

11 files changed

+212
-36
lines changed

‎coderd/coderdtest/coderdtest.go

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -760,21 +760,51 @@ func UpdateTemplateVersion(t *testing.T, client *codersdk.Client, organizationID
760760
return templateVersion
761761
}
762762

763+
func AwaitTemplateVersionJobRunning(t *testing.T, client *codersdk.Client, version uuid.UUID) codersdk.TemplateVersion {
764+
t.Helper()
765+
766+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
767+
defer cancel()
768+
769+
t.Logf("waiting for template version %s build job to start", version)
770+
var templateVersion codersdk.TemplateVersion
771+
require.Eventually(t, func() bool {
772+
var err error
773+
templateVersion, err = client.TemplateVersion(ctx, version)
774+
if err != nil {
775+
return false
776+
}
777+
t.Logf("template version job status: %s", templateVersion.Job.Status)
778+
switch templateVersion.Job.Status {
779+
case codersdk.ProvisionerJobPending:
780+
return false
781+
case codersdk.ProvisionerJobRunning:
782+
return true
783+
default:
784+
t.FailNow()
785+
return false
786+
}
787+
}, testutil.WaitShort, testutil.IntervalFast, "make sure you set `IncludeProvisionerDaemon`!")
788+
t.Logf("template version %s job has started", version)
789+
return templateVersion
790+
}
791+
763792
// AwaitTemplateImportJobCompleted awaits for an import job to reach completed status.
764793
func AwaitTemplateVersionJobCompleted(t *testing.T, client *codersdk.Client, version uuid.UUID) codersdk.TemplateVersion {
765794
t.Helper()
766795

767796
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
768797
defer cancel()
769798

770-
t.Logf("waiting for template version job %s", version)
799+
t.Logf("waiting for template version %s build job to complete", version)
771800
var templateVersion codersdk.TemplateVersion
772801
require.Eventually(t, func() bool {
773802
var err error
774803
templateVersion, err = client.TemplateVersion(ctx, version)
804+
t.Logf("template version job status: %s", templateVersion.Job.Status)
775805
return assert.NoError(t, err) && templateVersion.Job.CompletedAt != nil
776-
}, testutil.WaitLong, testutil.IntervalMedium)
777-
t.Logf("got template version job %s", version)
806+
}, testutil.WaitLong, testutil.IntervalMedium, "make sure you set `IncludeProvisionerDaemon`!")
807+
t.Logf("template version %s job has completed", version)
778808
return templateVersion
779809
}
780810

‎coderd/insights_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2078,7 +2078,7 @@ func TestTemplateInsights_RBAC(t *testing.T) {
20782078
t.Run("AsOwner", func(t *testing.T) {
20792079
t.Parallel()
20802080

2081-
client := coderdtest.New(t, &coderdtest.Options{})
2081+
client := coderdtest.New(t, nil)
20822082
admin := coderdtest.CreateFirstUser(t, client)
20832083

20842084
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
@@ -2102,7 +2102,7 @@ func TestTemplateInsights_RBAC(t *testing.T) {
21022102
t.Run("AsTemplateAdmin", func(t *testing.T) {
21032103
t.Parallel()
21042104

2105-
client := coderdtest.New(t, &coderdtest.Options{})
2105+
client := coderdtest.New(t, nil)
21062106
admin := coderdtest.CreateFirstUser(t, client)
21072107

21082108
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin())
@@ -2128,7 +2128,7 @@ func TestTemplateInsights_RBAC(t *testing.T) {
21282128
t.Run("AsRegularUser", func(t *testing.T) {
21292129
t.Parallel()
21302130

2131-
client := coderdtest.New(t, &coderdtest.Options{})
2131+
client := coderdtest.New(t, nil)
21322132
admin := coderdtest.CreateFirstUser(t, client)
21332133

21342134
regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID)
@@ -2207,7 +2207,7 @@ func TestGenericInsights_RBAC(t *testing.T) {
22072207
t.Run("AsOwner", func(t *testing.T) {
22082208
t.Parallel()
22092209

2210-
client := coderdtest.New(t, &coderdtest.Options{})
2210+
client := coderdtest.New(t, nil)
22112211
admin := coderdtest.CreateFirstUser(t, client)
22122212

22132213
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
@@ -2229,7 +2229,7 @@ func TestGenericInsights_RBAC(t *testing.T) {
22292229
t.Run("AsTemplateAdmin", func(t *testing.T) {
22302230
t.Parallel()
22312231

2232-
client := coderdtest.New(t, &coderdtest.Options{})
2232+
client := coderdtest.New(t, nil)
22332233
admin := coderdtest.CreateFirstUser(t, client)
22342234

22352235
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin())
@@ -2253,7 +2253,7 @@ func TestGenericInsights_RBAC(t *testing.T) {
22532253
t.Run("AsRegularUser", func(t *testing.T) {
22542254
t.Parallel()
22552255

2256-
client := coderdtest.New(t, &coderdtest.Options{})
2256+
client := coderdtest.New(t, nil)
22572257
admin := coderdtest.CreateFirstUser(t, client)
22582258

22592259
regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID)

‎coderd/templateversions.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020

2121
"github.com/coder/coder/v2/coderd/audit"
2222
"github.com/coder/coder/v2/coderd/database"
23+
"github.com/coder/coder/v2/coderd/database/db2sdk"
2324
"github.com/coder/coder/v2/coderd/database/dbtime"
2425
"github.com/coder/coder/v2/coderd/database/provisionerjobs"
2526
"github.com/coder/coder/v2/coderd/externalauth"
@@ -1038,6 +1039,22 @@ func (api *API) patchActiveTemplateVersion(rw http.ResponseWriter, r *http.Reque
10381039
})
10391040
return
10401041
}
1042+
job, err := api.Database.GetProvisionerJobByID(ctx, version.JobID)
1043+
if err != nil {
1044+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1045+
Message: "Internal error fetching template version job status.",
1046+
Detail: err.Error(),
1047+
})
1048+
return
1049+
}
1050+
jobStatus := db2sdk.ProvisionerJobStatus(job)
1051+
if jobStatus != codersdk.ProvisionerJobSucceeded {
1052+
httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{
1053+
Message: "Only versions that have been built successfully can be promoted.",
1054+
Detail: fmt.Sprintf("Attempted to promote a version with a %s build", jobStatus),
1055+
})
1056+
return
1057+
}
10411058

10421059
err = api.Database.InTx(func(store database.Store) error {
10431060
err = store.UpdateTemplateActiveVersionByID(ctx, database.UpdateTemplateActiveVersionByIDParams{

‎coderd/templateversions_test.go

Lines changed: 64 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,9 @@ func TestPatchCancelTemplateVersion(t *testing.T) {
241241
})
242242
t.Run("AlreadyCanceled", func(t *testing.T) {
243243
t.Parallel()
244-
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
244+
client := coderdtest.New(t, &coderdtest.Options{
245+
IncludeProvisionerDaemon: true,
246+
})
245247
user := coderdtest.CreateFirstUser(t, client)
246248
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{
247249
Parse: echo.ParseComplete,
@@ -255,15 +257,7 @@ func TestPatchCancelTemplateVersion(t *testing.T) {
255257
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
256258
defer cancel()
257259

258-
require.Eventually(t, func() bool {
259-
var err error
260-
version, err = client.TemplateVersion(ctx, version.ID)
261-
if !assert.NoError(t, err) {
262-
return false
263-
}
264-
t.Logf("Status: %s", version.Job.Status)
265-
return version.Job.Status == codersdk.ProvisionerJobRunning
266-
}, testutil.WaitShort, testutil.IntervalFast)
260+
coderdtest.AwaitTemplateVersionJobRunning(t, client, version.ID)
267261
err := client.CancelTemplateVersion(ctx, version.ID)
268262
require.NoError(t, err)
269263
err = client.CancelTemplateVersion(ctx, version.ID)
@@ -280,7 +274,9 @@ func TestPatchCancelTemplateVersion(t *testing.T) {
280274
// Running -> Canceling is the best we can do for now.
281275
t.Run("Canceling", func(t *testing.T) {
282276
t.Parallel()
283-
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
277+
client := coderdtest.New(t, &coderdtest.Options{
278+
IncludeProvisionerDaemon: true,
279+
})
284280
user := coderdtest.CreateFirstUser(t, client)
285281
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{
286282
Parse: echo.ParseComplete,
@@ -557,13 +553,60 @@ func TestPatchActiveTemplateVersion(t *testing.T) {
557553
require.Equal(t, http.StatusNotFound, apiErr.StatusCode())
558554
})
559555

560-
t.Run("DoesNotBelong", func(t *testing.T) {
556+
t.Run("CanceledBuild", func(t *testing.T) {
557+
t.Parallel()
558+
client := coderdtest.New(t, &coderdtest.Options{
559+
IncludeProvisionerDaemon: true,
560+
})
561+
user := coderdtest.CreateFirstUser(t, client)
562+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
563+
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
564+
version = coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, nil, template.ID)
565+
566+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
567+
defer cancel()
568+
569+
err := client.CancelTemplateVersion(ctx, version.ID)
570+
require.NoError(t, err)
571+
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
572+
err = client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{
573+
ID: version.ID,
574+
})
575+
var apiErr *codersdk.Error
576+
require.ErrorAs(t, err, &apiErr)
577+
require.Equal(t, http.StatusPreconditionFailed, apiErr.StatusCode())
578+
})
579+
580+
t.Run("PendingBuild", func(t *testing.T) {
561581
t.Parallel()
562582
client := coderdtest.New(t, nil)
563583
user := coderdtest.CreateFirstUser(t, client)
564584
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
565585
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
586+
version = coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, nil, template.ID)
587+
588+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
589+
defer cancel()
590+
591+
err := client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{
592+
ID: version.ID,
593+
})
594+
var apiErr *codersdk.Error
595+
require.ErrorAs(t, err, &apiErr)
596+
require.Equal(t, http.StatusPreconditionFailed, apiErr.StatusCode())
597+
})
598+
599+
t.Run("DoesNotBelong", func(t *testing.T) {
600+
t.Parallel()
601+
client := coderdtest.New(t, &coderdtest.Options{
602+
IncludeProvisionerDaemon: true,
603+
})
604+
user := coderdtest.CreateFirstUser(t, client)
605+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
606+
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
566607
version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
608+
_ = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
609+
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
567610

568611
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
569612
defer cancel()
@@ -576,13 +619,18 @@ func TestPatchActiveTemplateVersion(t *testing.T) {
576619
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
577620
})
578621

579-
t.Run("Found", func(t *testing.T) {
622+
t.Run("SuccessfulBuild", func(t *testing.T) {
580623
t.Parallel()
581624
auditor := audit.NewMock()
582-
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true, Auditor: auditor})
625+
client := coderdtest.New(t, &coderdtest.Options{
626+
IncludeProvisionerDaemon: true,
627+
Auditor: auditor,
628+
})
583629
user := coderdtest.CreateFirstUser(t, client)
584630
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
585631
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
632+
version = coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, nil, template.ID)
633+
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
586634

587635
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
588636
defer cancel()
@@ -592,8 +640,8 @@ func TestPatchActiveTemplateVersion(t *testing.T) {
592640
})
593641
require.NoError(t, err)
594642

595-
require.Len(t, auditor.AuditLogs(), 5)
596-
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs()[4].Action)
643+
require.Len(t, auditor.AuditLogs(), 6)
644+
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs()[5].Action)
597645
})
598646
}
599647

‎coderd/workspaceagents_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,11 +253,11 @@ func TestWorkspaceAgent(t *testing.T) {
253253
req.TemplateID = template.ID
254254
})
255255

256+
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
256257
err = client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{
257258
ID: version.ID,
258259
})
259260
require.NoError(t, err)
260-
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
261261
// Creating another workspace is just easier.
262262
workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
263263
build := coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)

‎coderd/workspaces_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -426,9 +426,10 @@ func TestPostWorkspacesByOrganization(t *testing.T) {
426426

427427
t.Run("NoTemplateAccess", func(t *testing.T) {
428428
t.Parallel()
429-
client := coderdtest.New(t, nil)
429+
client := coderdtest.New(t, &coderdtest.Options{
430+
IncludeProvisionerDaemon: true,
431+
})
430432
first := coderdtest.CreateFirstUser(t, client)
431-
432433
other, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleMember(), rbac.RoleOwner())
433434

434435
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
@@ -440,6 +441,7 @@ func TestPostWorkspacesByOrganization(t *testing.T) {
440441
require.NoError(t, err)
441442
version := coderdtest.CreateTemplateVersion(t, other, org.ID, nil)
442443
template := coderdtest.CreateTemplate(t, other, org.ID, version.ID)
444+
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
443445

444446
_, err = client.CreateWorkspace(ctx, first.OrganizationID, codersdk.Me, codersdk.CreateWorkspaceRequest{
445447
TemplateID: template.ID,

‎site/src/components/Pill/Pill.stories.tsx

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,11 @@ export const WarningLight: Story = {
6464
lightBorder: true,
6565
},
6666
};
67+
68+
export const Neutral: Story = {
69+
args: {
70+
text: "Neutral",
71+
type: "neutral",
72+
lightBorder: true,
73+
},
74+
};

‎site/src/components/Pill/Pill.tsx

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
1-
import { PaletteColor, Theme } from "@mui/material/styles";
1+
import { type PaletteColor, type Theme } from "@mui/material/styles";
22
import { makeStyles } from "@mui/styles";
3-
import { FC } from "react";
4-
import { PaletteIndex } from "theme/theme";
3+
import { type FC, type ReactNode } from "react";
4+
import { type PaletteIndex } from "theme/theme";
55
import { combineClasses } from "utils/combineClasses";
66

77
export interface PillProps {
88
className?: string;
9-
icon?: React.ReactNode;
10-
text: string;
9+
icon?: ReactNode;
10+
text: ReactNode;
1111
type?: PaletteIndex;
1212
lightBorder?: boolean;
1313
title?: string;

‎site/src/pages/TemplatePage/TemplateVersionsPage/VersionRow.tsx

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ export const VersionRow: React.FC<VersionRowProps> = ({
3232
onClick: () => navigate(version.name),
3333
});
3434

35+
const jobStatus = version.job.status;
36+
if (jobStatus === "canceled") {
37+
return null;
38+
}
39+
3540
return (
3641
<TimelineEntry
3742
data-testid={`version-${version.id}`}
@@ -78,10 +83,20 @@ export const VersionRow: React.FC<VersionRowProps> = ({
7883
<Stack direction="row" alignItems="center" spacing={2}>
7984
{isActive && <Pill text="Active" type="success" />}
8085
{isLatest && <Pill text="Newest" type="info" />}
86+
{jobStatus === "pending" && (
87+
<Pill text={<>Pending&hellip;</>} type="warning" lightBorder />
88+
)}
89+
{jobStatus === "running" && (
90+
<Pill text={<>Building&hellip;</>} type="warning" lightBorder />
91+
)}
92+
{jobStatus === "canceling" && (
93+
<Pill text={<>Canceling&hellip;</>} type="neutral" lightBorder />
94+
)}
95+
{jobStatus === "failed" && <Pill text="Failed" type="error" />}
8196
{onPromoteClick && (
8297
<Button
8398
className={styles.promoteButton}
84-
disabled={isActive}
99+
disabled={isActive || jobStatus !== "succeeded"}
85100
onClick={(e) => {
86101
e.preventDefault();
87102
e.stopPropagation();

0 commit comments

Comments
 (0)