Skip to content

Commit f001a57

Browse files
aslilacaslilac
authored
fix: only allow promoting successful template versions (#9998)
1 parent 48ee80a commit f001a57

File tree

9 files changed

+201
-39
lines changed

9 files changed

+201
-39
lines changed

‎coderd/coderdtest/coderdtest.go

+36-4
Original file line numberDiff line numberDiff line change
@@ -760,21 +760,53 @@ func UpdateTemplateVersion(t *testing.T, client *codersdk.Client, organizationID
760760
return templateVersion
761761
}
762762

763-
// AwaitTemplateVersionJobCompleted awaits for an import job to reach completed status.
763+
// AwaitTemplateVersionJobRunning waits for the build to be picked up by a provisioner.
764+
func AwaitTemplateVersionJobRunning(t *testing.T, client *codersdk.Client, version uuid.UUID) codersdk.TemplateVersion {
765+
t.Helper()
766+
767+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
768+
defer cancel()
769+
770+
t.Logf("waiting for template version %s build job to start", version)
771+
var templateVersion codersdk.TemplateVersion
772+
require.Eventually(t, func() bool {
773+
var err error
774+
templateVersion, err = client.TemplateVersion(ctx, version)
775+
if err != nil {
776+
return false
777+
}
778+
t.Logf("template version job status: %s", templateVersion.Job.Status)
779+
switch templateVersion.Job.Status {
780+
case codersdk.ProvisionerJobPending:
781+
return false
782+
case codersdk.ProvisionerJobRunning:
783+
return true
784+
default:
785+
t.FailNow()
786+
return false
787+
}
788+
}, testutil.WaitShort, testutil.IntervalFast, "make sure you set `IncludeProvisionerDaemon`!")
789+
t.Logf("template version %s job has started", version)
790+
return templateVersion
791+
}
792+
793+
// AwaitTemplateVersionJobCompleted waits for the build to be completed. This may result
794+
// from cancelation, an error, or from completing successfully.
764795
func AwaitTemplateVersionJobCompleted(t *testing.T, client *codersdk.Client, version uuid.UUID) codersdk.TemplateVersion {
765796
t.Helper()
766797

767798
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
768799
defer cancel()
769800

770-
t.Logf("waiting for template version job %s", version)
801+
t.Logf("waiting for template version %s build job to complete", version)
771802
var templateVersion codersdk.TemplateVersion
772803
require.Eventually(t, func() bool {
773804
var err error
774805
templateVersion, err = client.TemplateVersion(ctx, version)
806+
t.Logf("template version job status: %s", templateVersion.Job.Status)
775807
return assert.NoError(t, err) && templateVersion.Job.CompletedAt != nil
776-
}, testutil.WaitLong, testutil.IntervalMedium)
777-
t.Logf("got template version job %s", version)
808+
}, testutil.WaitLong, testutil.IntervalMedium, "make sure you set `IncludeProvisionerDaemon`!")
809+
t.Logf("template version %s job has completed", version)
778810
return templateVersion
779811
}
780812

‎coderd/insights_test.go

+6-6
Original file line numberDiff line numberDiff line change
@@ -2110,7 +2110,7 @@ func TestTemplateInsights_RBAC(t *testing.T) {
21102110
t.Run("AsOwner", func(t *testing.T) {
21112111
t.Parallel()
21122112

2113-
client := coderdtest.New(t, &coderdtest.Options{})
2113+
client := coderdtest.New(t, nil)
21142114
admin := coderdtest.CreateFirstUser(t, client)
21152115

21162116
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
@@ -2134,7 +2134,7 @@ func TestTemplateInsights_RBAC(t *testing.T) {
21342134
t.Run("AsTemplateAdmin", func(t *testing.T) {
21352135
t.Parallel()
21362136

2137-
client := coderdtest.New(t, &coderdtest.Options{})
2137+
client := coderdtest.New(t, nil)
21382138
admin := coderdtest.CreateFirstUser(t, client)
21392139

21402140
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin())
@@ -2160,7 +2160,7 @@ func TestTemplateInsights_RBAC(t *testing.T) {
21602160
t.Run("AsRegularUser", func(t *testing.T) {
21612161
t.Parallel()
21622162

2163-
client := coderdtest.New(t, &coderdtest.Options{})
2163+
client := coderdtest.New(t, nil)
21642164
admin := coderdtest.CreateFirstUser(t, client)
21652165

21662166
regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID)
@@ -2239,7 +2239,7 @@ func TestGenericInsights_RBAC(t *testing.T) {
22392239
t.Run("AsOwner", func(t *testing.T) {
22402240
t.Parallel()
22412241

2242-
client := coderdtest.New(t, &coderdtest.Options{})
2242+
client := coderdtest.New(t, nil)
22432243
admin := coderdtest.CreateFirstUser(t, client)
22442244

22452245
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
@@ -2261,7 +2261,7 @@ func TestGenericInsights_RBAC(t *testing.T) {
22612261
t.Run("AsTemplateAdmin", func(t *testing.T) {
22622262
t.Parallel()
22632263

2264-
client := coderdtest.New(t, &coderdtest.Options{})
2264+
client := coderdtest.New(t, nil)
22652265
admin := coderdtest.CreateFirstUser(t, client)
22662266

22672267
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID, rbac.RoleTemplateAdmin())
@@ -2285,7 +2285,7 @@ func TestGenericInsights_RBAC(t *testing.T) {
22852285
t.Run("AsRegularUser", func(t *testing.T) {
22862286
t.Parallel()
22872287

2288-
client := coderdtest.New(t, &coderdtest.Options{})
2288+
client := coderdtest.New(t, nil)
22892289
admin := coderdtest.CreateFirstUser(t, client)
22902290

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

‎coderd/templateversions.go

+19-2
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"
@@ -248,7 +249,7 @@ func (api *API) templateVersionRichParameters(rw http.ResponseWriter, r *http.Re
248249
return
249250
}
250251
if !job.CompletedAt.Valid {
251-
httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{
252+
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
252253
Message: "Job hasn't completed!",
253254
})
254255
return
@@ -383,7 +384,7 @@ func (api *API) templateVersionVariables(rw http.ResponseWriter, r *http.Request
383384
return
384385
}
385386
if !job.CompletedAt.Valid {
386-
httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{
387+
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
387388
Message: "Job hasn't completed!",
388389
})
389390
return
@@ -1040,6 +1041,22 @@ func (api *API) patchActiveTemplateVersion(rw http.ResponseWriter, r *http.Reque
10401041
})
10411042
return
10421043
}
1044+
job, err := api.Database.GetProvisionerJobByID(ctx, version.JobID)
1045+
if err != nil {
1046+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
1047+
Message: "Internal error fetching template version job status.",
1048+
Detail: err.Error(),
1049+
})
1050+
return
1051+
}
1052+
jobStatus := db2sdk.ProvisionerJobStatus(job)
1053+
if jobStatus != codersdk.ProvisionerJobSucceeded {
1054+
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
1055+
Message: "Only versions that have been built successfully can be promoted.",
1056+
Detail: fmt.Sprintf("Attempted to promote a version with a %s build", jobStatus),
1057+
})
1058+
return
1059+
}
10431060

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

‎coderd/templateversions_test.go

+64-16
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) {
561557
t.Parallel()
562558
client := coderdtest.New(t, nil)
563559
user := coderdtest.CreateFirstUser(t, client)
564560
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
565561
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
562+
version = coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, nil, template.ID)
563+
564+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
565+
defer cancel()
566+
567+
err := client.CancelTemplateVersion(ctx, version.ID)
568+
require.NoError(t, err)
569+
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
570+
err = client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{
571+
ID: version.ID,
572+
})
573+
var apiErr *codersdk.Error
574+
require.ErrorAs(t, err, &apiErr)
575+
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
576+
require.Contains(t, apiErr.Detail, "canceled")
577+
})
578+
579+
t.Run("PendingBuild", func(t *testing.T) {
580+
t.Parallel()
581+
client := coderdtest.New(t, nil)
582+
user := coderdtest.CreateFirstUser(t, client)
583+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
584+
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
585+
version = coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, nil, template.ID)
586+
587+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
588+
defer cancel()
589+
590+
err := client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{
591+
ID: version.ID,
592+
})
593+
var apiErr *codersdk.Error
594+
require.ErrorAs(t, err, &apiErr)
595+
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
596+
require.Contains(t, apiErr.Detail, "pending")
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

+1-1
Original file line numberDiff line numberDiff line change
@@ -255,11 +255,11 @@ func TestWorkspaceAgent(t *testing.T) {
255255
req.TemplateID = template.ID
256256
})
257257

258+
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
258259
err = client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{
259260
ID: version.ID,
260261
})
261262
require.NoError(t, err)
262-
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
263263
// Creating another workspace is just easier.
264264
workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
265265
build := coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)

‎coderd/workspaces_test.go

-1
Original file line numberDiff line numberDiff line change
@@ -428,7 +428,6 @@ func TestPostWorkspacesByOrganization(t *testing.T) {
428428
t.Parallel()
429429
client := coderdtest.New(t, nil)
430430
first := coderdtest.CreateFirstUser(t, client)
431-
432431
other, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleMember(), rbac.RoleOwner())
433432

434433
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)

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

+13-1
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ export const VersionRow: React.FC<VersionRowProps> = ({
3232
onClick: () => navigate(version.name),
3333
});
3434

35+
const jobStatus = version.job.status;
36+
3537
return (
3638
<TimelineEntry
3739
data-testid={`version-${version.id}`}
@@ -78,10 +80,20 @@ export const VersionRow: React.FC<VersionRowProps> = ({
7880
<Stack direction="row" alignItems="center" spacing={2}>
7981
{isActive && <Pill text="Active" type="success" />}
8082
{isLatest && <Pill text="Newest" type="info" />}
83+
{jobStatus === "pending" && (
84+
<Pill text={<>Pending&hellip;</>} type="warning" lightBorder />
85+
)}
86+
{jobStatus === "running" && (
87+
<Pill text={<>Building&hellip;</>} type="warning" lightBorder />
88+
)}
89+
{(jobStatus === "canceling" || jobStatus === "canceled") && (
90+
<Pill text="Canceled" type="neutral" lightBorder />
91+
)}
92+
{jobStatus === "failed" && <Pill text="Failed" type="error" />}
8193
{onPromoteClick && (
8294
<Button
8395
className={styles.promoteButton}
84-
disabled={isActive}
96+
disabled={isActive || jobStatus !== "succeeded"}
8597
onClick={(e) => {
8698
e.preventDefault();
8799
e.stopPropagation();

0 commit comments

Comments
 (0)