Skip to content

fix: only allow promoting successful template versions #9998

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

Merged
merged 11 commits into from
Oct 5, 2023
40 changes: 36 additions & 4 deletions coderd/coderdtest/coderdtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -760,21 +760,53 @@ func UpdateTemplateVersion(t *testing.T, client *codersdk.Client, organizationID
return templateVersion
}

// AwaitTemplateVersionJobCompleted awaits for an import job to reach completed status.
// AwaitTemplateVersionJobRunning waits for the build to be picked up by a provisioner.
func AwaitTemplateVersionJobRunning(t *testing.T, client *codersdk.Client, version uuid.UUID) codersdk.TemplateVersion {
t.Helper()

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
defer cancel()

t.Logf("waiting for template version %s build job to start", version)
var templateVersion codersdk.TemplateVersion
require.Eventually(t, func() bool {
var err error
templateVersion, err = client.TemplateVersion(ctx, version)
if err != nil {
return false
}
t.Logf("template version job status: %s", templateVersion.Job.Status)
switch templateVersion.Job.Status {
case codersdk.ProvisionerJobPending:
return false
case codersdk.ProvisionerJobRunning:
return true
default:
t.FailNow()
return false
}
}, testutil.WaitShort, testutil.IntervalFast, "make sure you set `IncludeProvisionerDaemon`!")
t.Logf("template version %s job has started", version)
return templateVersion
}

// AwaitTemplateVersionJobCompleted waits for the build to be completed. This may result
// from cancelation, an error, or from completing successfully.
func AwaitTemplateVersionJobCompleted(t *testing.T, client *codersdk.Client, version uuid.UUID) codersdk.TemplateVersion {
t.Helper()

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

t.Logf("waiting for template version job %s", version)
t.Logf("waiting for template version %s build job to complete", version)
var templateVersion codersdk.TemplateVersion
require.Eventually(t, func() bool {
var err error
templateVersion, err = client.TemplateVersion(ctx, version)
t.Logf("template version job status: %s", templateVersion.Job.Status)
return assert.NoError(t, err) && templateVersion.Job.CompletedAt != nil
}, testutil.WaitLong, testutil.IntervalMedium)
t.Logf("got template version job %s", version)
}, testutil.WaitLong, testutil.IntervalMedium, "make sure you set `IncludeProvisionerDaemon`!")
t.Logf("template version %s job has completed", version)
return templateVersion
}

Expand Down
12 changes: 6 additions & 6 deletions coderd/insights_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2110,7 +2110,7 @@ func TestTemplateInsights_RBAC(t *testing.T) {
t.Run("AsOwner", func(t *testing.T) {
t.Parallel()

client := coderdtest.New(t, &coderdtest.Options{})
client := coderdtest.New(t, nil)
admin := coderdtest.CreateFirstUser(t, client)

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

client := coderdtest.New(t, &coderdtest.Options{})
client := coderdtest.New(t, nil)
admin := coderdtest.CreateFirstUser(t, client)

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

client := coderdtest.New(t, &coderdtest.Options{})
client := coderdtest.New(t, nil)
admin := coderdtest.CreateFirstUser(t, client)

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

client := coderdtest.New(t, &coderdtest.Options{})
client := coderdtest.New(t, nil)
admin := coderdtest.CreateFirstUser(t, client)

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

client := coderdtest.New(t, &coderdtest.Options{})
client := coderdtest.New(t, nil)
admin := coderdtest.CreateFirstUser(t, client)

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

client := coderdtest.New(t, &coderdtest.Options{})
client := coderdtest.New(t, nil)
admin := coderdtest.CreateFirstUser(t, client)

regular, _ := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID)
Expand Down
21 changes: 19 additions & 2 deletions coderd/templateversions.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

"github.com/coder/coder/v2/coderd/audit"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/db2sdk"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/coderd/database/provisionerjobs"
"github.com/coder/coder/v2/coderd/externalauth"
Expand Down Expand Up @@ -248,7 +249,7 @@ func (api *API) templateVersionRichParameters(rw http.ResponseWriter, r *http.Re
return
}
if !job.CompletedAt.Valid {
httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
Message: "Job hasn't completed!",
})
return
Expand Down Expand Up @@ -383,7 +384,7 @@ func (api *API) templateVersionVariables(rw http.ResponseWriter, r *http.Request
return
}
if !job.CompletedAt.Valid {
httpapi.Write(ctx, rw, http.StatusPreconditionFailed, codersdk.Response{
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
Message: "Job hasn't completed!",
})
return
Expand Down Expand Up @@ -1040,6 +1041,22 @@ func (api *API) patchActiveTemplateVersion(rw http.ResponseWriter, r *http.Reque
})
return
}
job, err := api.Database.GetProvisionerJobByID(ctx, version.JobID)
if err != nil {
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
Message: "Internal error fetching template version job status.",
Detail: err.Error(),
})
return
}
jobStatus := db2sdk.ProvisionerJobStatus(job)
if jobStatus != codersdk.ProvisionerJobSucceeded {
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
Message: "Only versions that have been built successfully can be promoted.",
Detail: fmt.Sprintf("Attempted to promote a version with a %s build", jobStatus),
})
return
}

err = api.Database.InTx(func(store database.Store) error {
err = store.UpdateTemplateActiveVersionByID(ctx, database.UpdateTemplateActiveVersionByIDParams{
Expand Down
80 changes: 64 additions & 16 deletions coderd/templateversions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,9 @@ func TestPatchCancelTemplateVersion(t *testing.T) {
})
t.Run("AlreadyCanceled", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
client := coderdtest.New(t, &coderdtest.Options{
IncludeProvisionerDaemon: true,
})
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{
Parse: echo.ParseComplete,
Expand All @@ -255,15 +257,7 @@ func TestPatchCancelTemplateVersion(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
defer cancel()

require.Eventually(t, func() bool {
var err error
version, err = client.TemplateVersion(ctx, version.ID)
if !assert.NoError(t, err) {
return false
}
t.Logf("Status: %s", version.Job.Status)
return version.Job.Status == codersdk.ProvisionerJobRunning
}, testutil.WaitShort, testutil.IntervalFast)
coderdtest.AwaitTemplateVersionJobRunning(t, client, version.ID)
err := client.CancelTemplateVersion(ctx, version.ID)
require.NoError(t, err)
err = client.CancelTemplateVersion(ctx, version.ID)
Expand All @@ -280,7 +274,9 @@ func TestPatchCancelTemplateVersion(t *testing.T) {
// Running -> Canceling is the best we can do for now.
t.Run("Canceling", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
client := coderdtest.New(t, &coderdtest.Options{
IncludeProvisionerDaemon: true,
})
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{
Parse: echo.ParseComplete,
Expand Down Expand Up @@ -557,13 +553,60 @@ func TestPatchActiveTemplateVersion(t *testing.T) {
require.Equal(t, http.StatusNotFound, apiErr.StatusCode())
})

t.Run("DoesNotBelong", func(t *testing.T) {
t.Run("CanceledBuild", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
version = coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, nil, template.ID)

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

err := client.CancelTemplateVersion(ctx, version.ID)
require.NoError(t, err)
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
err = client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{
ID: version.ID,
})
var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
require.Contains(t, apiErr.Detail, "canceled")
})

t.Run("PendingBuild", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
version = coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, nil, template.ID)

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

err := client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{
ID: version.ID,
})
var apiErr *codersdk.Error
require.ErrorAs(t, err, &apiErr)
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
require.Contains(t, apiErr.Detail, "pending")
})

t.Run("DoesNotBelong", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, &coderdtest.Options{
IncludeProvisionerDaemon: true,
})
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
_ = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)

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

t.Run("Found", func(t *testing.T) {
t.Run("SuccessfulBuild", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true, Auditor: auditor})
client := coderdtest.New(t, &coderdtest.Options{
IncludeProvisionerDaemon: true,
Auditor: auditor,
})
user := coderdtest.CreateFirstUser(t, client)
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
version = coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, nil, template.ID)
_ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)

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

require.Len(t, auditor.AuditLogs(), 5)
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs()[4].Action)
require.Len(t, auditor.AuditLogs(), 6)
assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs()[5].Action)
})
}

Expand Down
2 changes: 1 addition & 1 deletion coderd/workspaceagents_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,11 @@ func TestWorkspaceAgent(t *testing.T) {
req.TemplateID = template.ID
})

coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
err = client.UpdateActiveTemplateVersion(ctx, template.ID, codersdk.UpdateActiveTemplateVersion{
ID: version.ID,
})
require.NoError(t, err)
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
// Creating another workspace is just easier.
workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
build := coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)
Expand Down
1 change: 0 additions & 1 deletion coderd/workspaces_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,6 @@ func TestPostWorkspacesByOrganization(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
first := coderdtest.CreateFirstUser(t, client)

other, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleMember(), rbac.RoleOwner())

ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
Expand Down
14 changes: 13 additions & 1 deletion site/src/pages/TemplatePage/TemplateVersionsPage/VersionRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ export const VersionRow: React.FC<VersionRowProps> = ({
onClick: () => navigate(version.name),
});

const jobStatus = version.job.status;

return (
<TimelineEntry
data-testid={`version-${version.id}`}
Expand Down Expand Up @@ -78,10 +80,20 @@ export const VersionRow: React.FC<VersionRowProps> = ({
<Stack direction="row" alignItems="center" spacing={2}>
{isActive && <Pill text="Active" type="success" />}
{isLatest && <Pill text="Newest" type="info" />}
{jobStatus === "pending" && (
<Pill text={<>Pending&hellip;</>} type="warning" lightBorder />
)}
{jobStatus === "running" && (
<Pill text={<>Building&hellip;</>} type="warning" lightBorder />
)}
{(jobStatus === "canceling" || jobStatus === "canceled") && (
<Pill text="Canceled" type="neutral" lightBorder />
)}
{jobStatus === "failed" && <Pill text="Failed" type="error" />}
{onPromoteClick && (
<Button
className={styles.promoteButton}
disabled={isActive}
disabled={isActive || jobStatus !== "succeeded"}
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
Expand Down
Loading