From c0e28046f91daa5dd3a8d454c7ecf60f4c23e239 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 30 May 2023 10:32:36 -0400 Subject: [PATCH 01/13] WIP unit test for workspace statuses --- coderd/workspaces_test.go | 52 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 36227d411acc4..038aa63abfb95 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -8,6 +8,8 @@ import ( "testing" "time" + "github.com/coder/coder/coderd/database/dbgen" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -555,6 +557,56 @@ func TestWorkspaceByOwnerAndName(t *testing.T) { }) } +func TestWorkspaceFilterAllStatus(t *testing.T) { + client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{}) + + owner := coderdtest.CreateFirstUser(t, client) + + file := dbgen.File(t, api.Database, database.File{ + CreatedBy: owner.UserID, + }) + versionJob := dbgen.ProvisionerJob(t, api.Database, database.ProvisionerJob{ + OrganizationID: owner.OrganizationID, + InitiatorID: owner.UserID, + WorkerID: uuid.NullUUID{}, + FileID: file.ID, + }) + version := dbgen.TemplateVersion(t, api.Database, database.TemplateVersion{ + OrganizationID: owner.OrganizationID, + JobID: versionJob.ID, + CreatedBy: owner.UserID, + }) + template := dbgen.Template(t, api.Database, database.Template{ + OrganizationID: owner.OrganizationID, + ActiveVersionID: version.ID, + CreatedBy: owner.UserID, + }) + + makeWorkspace := func(workspace database.Workspace) database.Workspace { + workspace.OwnerID = owner.UserID + workspace.OrganizationID = owner.OrganizationID + workspace.TemplateID = template.ID + workspace = dbgen.Workspace(t, api.Database, workspace) + + return workspace + } + var _ = makeWorkspace + //makeWorkspace(database.Workspace{ + //}) + + // pending + // starting + // running + // stopping + // stopped + // failed + // canceling + // canceled + // deleted + // deleting + +} + // TestWorkspaceFilter creates a set of workspaces, users, and organizations // to run various filters against for testing. func TestWorkspaceFilter(t *testing.T) { From 50b28738d6e8ce1b6289b857842ed12650a2ca6f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 30 May 2023 10:47:26 -0400 Subject: [PATCH 02/13] Fix permission issues of dbgen --- coderd/database/dbgen/generator.go | 69 +++++++++++++++++++----------- 1 file changed, 43 insertions(+), 26 deletions(-) diff --git a/coderd/database/dbgen/generator.go b/coderd/database/dbgen/generator.go index b53a749aa0f49..dfedbcdd0a0ab 100644 --- a/coderd/database/dbgen/generator.go +++ b/coderd/database/dbgen/generator.go @@ -11,6 +11,9 @@ import ( "testing" "time" + "github.com/coder/coder/coderd/database/dbauthz" + "github.com/coder/coder/coderd/rbac" + "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" "github.com/stretchr/testify/require" @@ -23,8 +26,15 @@ import ( // All methods take in a 'seed' object. Any provided fields in the seed will be // maintained. Any fields omitted will have sensible defaults generated. +var genCtx = dbauthz.As(context.Background(), rbac.Subject{ + ID: "owner", + Roles: rbac.Roles(must(rbac.RoleNames{rbac.RoleOwner()}.Expand())), + Groups: []string{}, + Scope: rbac.ExpandableScope(rbac.ScopeAll), +}) + func AuditLog(t testing.TB, db database.Store, seed database.AuditLog) database.AuditLog { - log, err := db.InsertAuditLog(context.Background(), database.InsertAuditLogParams{ + log, err := db.InsertAuditLog(genCtx, database.InsertAuditLogParams{ ID: takeFirst(seed.ID, uuid.New()), Time: takeFirst(seed.Time, database.Now()), UserID: takeFirst(seed.UserID, uuid.New()), @@ -52,7 +62,7 @@ func AuditLog(t testing.TB, db database.Store, seed database.AuditLog) database. } func Template(t testing.TB, db database.Store, seed database.Template) database.Template { - template, err := db.InsertTemplate(context.Background(), database.InsertTemplateParams{ + template, err := db.InsertTemplate(genCtx, database.InsertTemplateParams{ ID: takeFirst(seed.ID, uuid.New()), CreatedAt: takeFirst(seed.CreatedAt, database.Now()), UpdatedAt: takeFirst(seed.UpdatedAt, database.Now()), @@ -88,7 +98,7 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database } } - key, err := db.InsertAPIKey(context.Background(), database.InsertAPIKeyParams{ + key, err := db.InsertAPIKey(genCtx, database.InsertAPIKeyParams{ ID: takeFirst(seed.ID, id), // 0 defaults to 86400 at the db layer LifetimeSeconds: takeFirst(seed.LifetimeSeconds, 0), @@ -108,7 +118,7 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database } func WorkspaceAgent(t testing.TB, db database.Store, orig database.WorkspaceAgent) database.WorkspaceAgent { - workspace, err := db.InsertWorkspaceAgent(context.Background(), database.InsertWorkspaceAgentParams{ + workspace, err := db.InsertWorkspaceAgent(genCtx, database.InsertWorkspaceAgentParams{ ID: takeFirst(orig.ID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), UpdatedAt: takeFirst(orig.UpdatedAt, database.Now()), @@ -149,7 +159,7 @@ func WorkspaceAgent(t testing.TB, db database.Store, orig database.WorkspaceAgen } func Workspace(t testing.TB, db database.Store, orig database.Workspace) database.Workspace { - workspace, err := db.InsertWorkspace(context.Background(), database.InsertWorkspaceParams{ + workspace, err := db.InsertWorkspace(genCtx, database.InsertWorkspaceParams{ ID: takeFirst(orig.ID, uuid.New()), OwnerID: takeFirst(orig.OwnerID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), @@ -166,7 +176,7 @@ func Workspace(t testing.TB, db database.Store, orig database.Workspace) databas } func WorkspaceBuild(t testing.TB, db database.Store, orig database.WorkspaceBuild) database.WorkspaceBuild { - build, err := db.InsertWorkspaceBuild(context.Background(), database.InsertWorkspaceBuildParams{ + build, err := db.InsertWorkspaceBuild(genCtx, database.InsertWorkspaceBuildParams{ ID: takeFirst(orig.ID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), UpdatedAt: takeFirst(orig.UpdatedAt, database.Now()), @@ -185,7 +195,7 @@ func WorkspaceBuild(t testing.TB, db database.Store, orig database.WorkspaceBuil } func User(t testing.TB, db database.Store, orig database.User) database.User { - user, err := db.InsertUser(context.Background(), database.InsertUserParams{ + user, err := db.InsertUser(genCtx, database.InsertUserParams{ ID: takeFirst(orig.ID, uuid.New()), Email: takeFirst(orig.Email, namesgenerator.GetRandomName(1)), Username: takeFirst(orig.Username, namesgenerator.GetRandomName(1)), @@ -200,7 +210,7 @@ func User(t testing.TB, db database.Store, orig database.User) database.User { } func GitSSHKey(t testing.TB, db database.Store, orig database.GitSSHKey) database.GitSSHKey { - key, err := db.InsertGitSSHKey(context.Background(), database.InsertGitSSHKeyParams{ + key, err := db.InsertGitSSHKey(genCtx, database.InsertGitSSHKeyParams{ UserID: takeFirst(orig.UserID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), UpdatedAt: takeFirst(orig.UpdatedAt, database.Now()), @@ -212,7 +222,7 @@ func GitSSHKey(t testing.TB, db database.Store, orig database.GitSSHKey) databas } func Organization(t testing.TB, db database.Store, orig database.Organization) database.Organization { - org, err := db.InsertOrganization(context.Background(), database.InsertOrganizationParams{ + org, err := db.InsertOrganization(genCtx, database.InsertOrganizationParams{ ID: takeFirst(orig.ID, uuid.New()), Name: takeFirst(orig.Name, namesgenerator.GetRandomName(1)), Description: takeFirst(orig.Description, namesgenerator.GetRandomName(1)), @@ -224,7 +234,7 @@ func Organization(t testing.TB, db database.Store, orig database.Organization) d } func OrganizationMember(t testing.TB, db database.Store, orig database.OrganizationMember) database.OrganizationMember { - mem, err := db.InsertOrganizationMember(context.Background(), database.InsertOrganizationMemberParams{ + mem, err := db.InsertOrganizationMember(genCtx, database.InsertOrganizationMemberParams{ OrganizationID: takeFirst(orig.OrganizationID, uuid.New()), UserID: takeFirst(orig.UserID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), @@ -236,7 +246,7 @@ func OrganizationMember(t testing.TB, db database.Store, orig database.Organizat } func Group(t testing.TB, db database.Store, orig database.Group) database.Group { - group, err := db.InsertGroup(context.Background(), database.InsertGroupParams{ + group, err := db.InsertGroup(genCtx, database.InsertGroupParams{ ID: takeFirst(orig.ID, uuid.New()), Name: takeFirst(orig.Name, namesgenerator.GetRandomName(1)), OrganizationID: takeFirst(orig.OrganizationID, uuid.New()), @@ -253,7 +263,7 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) dat GroupID: takeFirst(orig.GroupID, uuid.New()), } //nolint:gosimple - err := db.InsertGroupMember(context.Background(), database.InsertGroupMemberParams{ + err := db.InsertGroupMember(genCtx, database.InsertGroupMemberParams{ UserID: member.UserID, GroupID: member.GroupID, }) @@ -262,7 +272,7 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) dat } func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJob) database.ProvisionerJob { - job, err := db.InsertProvisionerJob(context.Background(), database.InsertProvisionerJobParams{ + job, err := db.InsertProvisionerJob(genCtx, database.InsertProvisionerJobParams{ ID: takeFirst(orig.ID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), UpdatedAt: takeFirst(orig.UpdatedAt, database.Now()), @@ -280,7 +290,7 @@ func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJo } func WorkspaceApp(t testing.TB, db database.Store, orig database.WorkspaceApp) database.WorkspaceApp { - resource, err := db.InsertWorkspaceApp(context.Background(), database.InsertWorkspaceAppParams{ + resource, err := db.InsertWorkspaceApp(genCtx, database.InsertWorkspaceAppParams{ ID: takeFirst(orig.ID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), AgentID: takeFirst(orig.AgentID, uuid.New()), @@ -308,7 +318,7 @@ func WorkspaceApp(t testing.TB, db database.Store, orig database.WorkspaceApp) d } func WorkspaceResource(t testing.TB, db database.Store, orig database.WorkspaceResource) database.WorkspaceResource { - resource, err := db.InsertWorkspaceResource(context.Background(), database.InsertWorkspaceResourceParams{ + resource, err := db.InsertWorkspaceResource(genCtx, database.InsertWorkspaceResourceParams{ ID: takeFirst(orig.ID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), JobID: takeFirst(orig.JobID, uuid.New()), @@ -328,7 +338,7 @@ func WorkspaceResource(t testing.TB, db database.Store, orig database.WorkspaceR } func WorkspaceResourceMetadatums(t testing.TB, db database.Store, seed database.WorkspaceResourceMetadatum) []database.WorkspaceResourceMetadatum { - meta, err := db.InsertWorkspaceResourceMetadata(context.Background(), database.InsertWorkspaceResourceMetadataParams{ + meta, err := db.InsertWorkspaceResourceMetadata(genCtx, database.InsertWorkspaceResourceMetadataParams{ WorkspaceResourceID: takeFirst(seed.WorkspaceResourceID, uuid.New()), Key: []string{takeFirst(seed.Key, namesgenerator.GetRandomName(1))}, Value: []string{takeFirst(seed.Value.String, namesgenerator.GetRandomName(1))}, @@ -343,7 +353,7 @@ func WorkspaceProxy(t testing.TB, db database.Store, orig database.WorkspaceProx require.NoError(t, err, "generate secret") hashedSecret := sha256.Sum256([]byte(secret)) - proxy, err := db.InsertWorkspaceProxy(context.Background(), database.InsertWorkspaceProxyParams{ + proxy, err := db.InsertWorkspaceProxy(genCtx, database.InsertWorkspaceProxyParams{ ID: takeFirst(orig.ID, uuid.New()), Name: takeFirst(orig.Name, namesgenerator.GetRandomName(1)), DisplayName: takeFirst(orig.DisplayName, namesgenerator.GetRandomName(1)), @@ -356,7 +366,7 @@ func WorkspaceProxy(t testing.TB, db database.Store, orig database.WorkspaceProx // Also set these fields if the caller wants them. if orig.Url != "" || orig.WildcardHostname != "" { - proxy, err = db.RegisterWorkspaceProxy(context.Background(), database.RegisterWorkspaceProxyParams{ + proxy, err = db.RegisterWorkspaceProxy(genCtx, database.RegisterWorkspaceProxyParams{ Url: orig.Url, WildcardHostname: orig.WildcardHostname, ID: proxy.ID, @@ -367,7 +377,7 @@ func WorkspaceProxy(t testing.TB, db database.Store, orig database.WorkspaceProx } func File(t testing.TB, db database.Store, orig database.File) database.File { - file, err := db.InsertFile(context.Background(), database.InsertFileParams{ + file, err := db.InsertFile(genCtx, database.InsertFileParams{ ID: takeFirst(orig.ID, uuid.New()), Hash: takeFirst(orig.Hash, hex.EncodeToString(make([]byte, 32))), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), @@ -380,7 +390,7 @@ func File(t testing.TB, db database.Store, orig database.File) database.File { } func UserLink(t testing.TB, db database.Store, orig database.UserLink) database.UserLink { - link, err := db.InsertUserLink(context.Background(), database.InsertUserLinkParams{ + link, err := db.InsertUserLink(genCtx, database.InsertUserLinkParams{ UserID: takeFirst(orig.UserID, uuid.New()), LoginType: takeFirst(orig.LoginType, database.LoginTypeGithub), LinkedID: takeFirst(orig.LinkedID), @@ -394,7 +404,7 @@ func UserLink(t testing.TB, db database.Store, orig database.UserLink) database. } func GitAuthLink(t testing.TB, db database.Store, orig database.GitAuthLink) database.GitAuthLink { - link, err := db.InsertGitAuthLink(context.Background(), database.InsertGitAuthLinkParams{ + link, err := db.InsertGitAuthLink(genCtx, database.InsertGitAuthLinkParams{ ProviderID: takeFirst(orig.ProviderID, uuid.New().String()), UserID: takeFirst(orig.UserID, uuid.New()), OAuthAccessToken: takeFirst(orig.OAuthAccessToken, uuid.NewString()), @@ -409,7 +419,7 @@ func GitAuthLink(t testing.TB, db database.Store, orig database.GitAuthLink) dat } func TemplateVersion(t testing.TB, db database.Store, orig database.TemplateVersion) database.TemplateVersion { - version, err := db.InsertTemplateVersion(context.Background(), database.InsertTemplateVersionParams{ + version, err := db.InsertTemplateVersion(genCtx, database.InsertTemplateVersionParams{ ID: takeFirst(orig.ID, uuid.New()), TemplateID: orig.TemplateID, OrganizationID: takeFirst(orig.OrganizationID, uuid.New()), @@ -425,7 +435,7 @@ func TemplateVersion(t testing.TB, db database.Store, orig database.TemplateVers } func TemplateVersionVariable(t testing.TB, db database.Store, orig database.TemplateVersionVariable) database.TemplateVersionVariable { - version, err := db.InsertTemplateVersionVariable(context.Background(), database.InsertTemplateVersionVariableParams{ + version, err := db.InsertTemplateVersionVariable(genCtx, database.InsertTemplateVersionVariableParams{ TemplateVersionID: takeFirst(orig.TemplateVersionID, uuid.New()), Name: takeFirst(orig.Name, namesgenerator.GetRandomName(1)), Description: takeFirst(orig.Description, namesgenerator.GetRandomName(1)), @@ -440,7 +450,7 @@ func TemplateVersionVariable(t testing.TB, db database.Store, orig database.Temp } func ParameterSchema(t testing.TB, db database.Store, seed database.ParameterSchema) database.ParameterSchema { - scheme, err := db.InsertParameterSchema(context.Background(), database.InsertParameterSchemaParams{ + scheme, err := db.InsertParameterSchema(genCtx, database.InsertParameterSchemaParams{ ID: takeFirst(seed.ID, uuid.New()), JobID: takeFirst(seed.JobID, uuid.New()), CreatedAt: takeFirst(seed.CreatedAt, database.Now()), @@ -464,7 +474,7 @@ func ParameterSchema(t testing.TB, db database.Store, seed database.ParameterSch } func ParameterValue(t testing.TB, db database.Store, seed database.ParameterValue) database.ParameterValue { - scheme, err := db.InsertParameterValue(context.Background(), database.InsertParameterValueParams{ + scheme, err := db.InsertParameterValue(genCtx, database.InsertParameterValueParams{ ID: takeFirst(seed.ID, uuid.New()), Name: takeFirst(seed.Name, namesgenerator.GetRandomName(1)), CreatedAt: takeFirst(seed.CreatedAt, database.Now()), @@ -483,7 +493,7 @@ func WorkspaceAgentStat(t testing.TB, db database.Store, orig database.Workspace if orig.ConnectionsByProto == nil { orig.ConnectionsByProto = json.RawMessage([]byte("{}")) } - scheme, err := db.InsertWorkspaceAgentStat(context.Background(), database.InsertWorkspaceAgentStatParams{ + scheme, err := db.InsertWorkspaceAgentStat(genCtx, database.InsertWorkspaceAgentStatParams{ ID: takeFirst(orig.ID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), UserID: takeFirst(orig.UserID, uuid.New()), @@ -505,3 +515,10 @@ func WorkspaceAgentStat(t testing.TB, db database.Store, orig database.Workspace require.NoError(t, err, "insert workspace agent stat") return scheme } + +func must[V any](v V, err error) V { + if err != nil { + panic(err) + } + return v +} From 2cb3ece5a0de5162de238eb2a1b85a49a988f314 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 30 May 2023 13:29:59 -0400 Subject: [PATCH 03/13] Write unit test for workspace status --- coderd/database/dbgen/generator.go | 41 ++++++++++++++++++++++++++ coderd/workspaces_test.go | 47 ++++++++++++++++++++++++++---- 2 files changed, 82 insertions(+), 6 deletions(-) diff --git a/coderd/database/dbgen/generator.go b/coderd/database/dbgen/generator.go index dfedbcdd0a0ab..7ffea3a45aff1 100644 --- a/coderd/database/dbgen/generator.go +++ b/coderd/database/dbgen/generator.go @@ -11,6 +11,8 @@ import ( "testing" "time" + "github.com/coder/coder/coderd/database/dbtype" + "github.com/coder/coder/coderd/database/dbauthz" "github.com/coder/coder/coderd/rbac" @@ -26,6 +28,7 @@ import ( // All methods take in a 'seed' object. Any provided fields in the seed will be // maintained. Any fields omitted will have sensible defaults generated. +// genCtx is to give all generator functions permission if the db is a dbauthz db. var genCtx = dbauthz.As(context.Background(), rbac.Subject{ ID: "owner", Roles: rbac.Roles(must(rbac.RoleNames{rbac.RoleOwner()}.Expand())), @@ -271,7 +274,17 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) dat return member } +// ProvisionerJob might not have all the correct values like CompletedAt and CancelledAt. This is because +// the workspaceBuild is required to fetch those, func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJob) database.ProvisionerJob { + id := takeFirst(orig.ID, uuid.New()) + if !orig.StartedAt.Time.IsZero() { + if orig.Tags == nil { + orig.Tags = make(dbtype.StringMap) + } + // Make sure when we acquire the job, we only get this one. + orig.Tags[id.String()] = "true" + } job, err := db.InsertProvisionerJob(genCtx, database.InsertProvisionerJobParams{ ID: takeFirst(orig.ID, uuid.New()), CreatedAt: takeFirst(orig.CreatedAt, database.Now()), @@ -286,6 +299,34 @@ func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJo Tags: orig.Tags, }) require.NoError(t, err, "insert job") + + if !orig.CompletedAt.Time.IsZero() || orig.Error.String != "" { + err := db.UpdateProvisionerJobWithCompleteByID(genCtx, database.UpdateProvisionerJobWithCompleteByIDParams{ + ID: job.ID, + UpdatedAt: orig.UpdatedAt, + CompletedAt: orig.CompletedAt, + Error: orig.Error, + ErrorCode: orig.ErrorCode, + }) + require.NoError(t, err) + } + if !orig.CanceledAt.Time.IsZero() { + err := db.UpdateProvisionerJobWithCancelByID(genCtx, database.UpdateProvisionerJobWithCancelByIDParams{ + ID: job.ID, + CanceledAt: orig.CanceledAt, + CompletedAt: orig.CompletedAt, + }) + require.NoError(t, err) + } + if !orig.StartedAt.Time.IsZero() { + job, err = db.AcquireProvisionerJob(genCtx, database.AcquireProvisionerJobParams{ + StartedAt: orig.StartedAt, + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + Tags: must(json.Marshal(orig.Tags)), + }) + require.NoError(t, err) + } + return job } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 038aa63abfb95..c516a3f5a3a3d 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -2,12 +2,15 @@ package coderd_test import ( "context" + "database/sql" "fmt" "net/http" "strings" "testing" "time" + "github.com/coder/coder/coderd/database/dbauthz" + "github.com/coder/coder/coderd/database/dbgen" "github.com/google/uuid" @@ -558,6 +561,7 @@ func TestWorkspaceByOwnerAndName(t *testing.T) { } func TestWorkspaceFilterAllStatus(t *testing.T) { + ctx := dbauthz.AsSystemRestricted(context.Background()) client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{}) owner := coderdtest.CreateFirstUser(t, client) @@ -582,17 +586,48 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { CreatedBy: owner.UserID, }) - makeWorkspace := func(workspace database.Workspace) database.Workspace { + makeWorkspace := func(workspace database.Workspace, job database.ProvisionerJob, transition database.WorkspaceTransition) (database.Workspace, database.WorkspaceBuild, database.ProvisionerJob) { + db := api.Database + workspace.OwnerID = owner.UserID workspace.OrganizationID = owner.OrganizationID workspace.TemplateID = template.ID - workspace = dbgen.Workspace(t, api.Database, workspace) + workspace = dbgen.Workspace(t, db, workspace) + + job.Type = database.ProvisionerJobTypeWorkspaceBuild + job.OrganizationID = owner.OrganizationID + job = dbgen.ProvisionerJob(t, db, job) + + build := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + WorkspaceID: workspace.ID, + TemplateVersionID: version.ID, + BuildNumber: 1, + Transition: transition, + InitiatorID: owner.UserID, + JobID: job.ID, + }) + + var err error + job, err = db.GetProvisionerJobByID(ctx, job.IDtus + ) + require.NoError(t, err) + + return workspace, build, job + } + + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusRunning), + }, database.ProvisionerJob{ + CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + }, database.WorkspaceTransitionStart) + + workspaces, err := client.Workspaces(context.Background(), codersdk.WorkspaceFilter{}) + require.NoError(t, err) - return workspace + for _, apiWorkspace := range workspaces.Workspaces { + require.Equal(t, apiWorkspace.Name, string(apiWorkspace.LatestBuild.Status)) } - var _ = makeWorkspace - //makeWorkspace(database.Workspace{ - //}) // pending // starting From 7d4b7eaec975306fc627d77ec251377e715b0e2e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 30 May 2023 14:06:35 -0400 Subject: [PATCH 04/13] Add unit test for statuses --- coderd/database/dbauthz/dbauthz.go | 11 ++++ coderd/database/dbauthz/system_test.go | 3 +- coderd/database/dbgen/generator.go | 29 ++++++--- coderd/workspaces_test.go | 84 ++++++++++++++++++++++---- 4 files changed, 103 insertions(+), 24 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index b823f7bd7067f..bacf0afbd9341 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -15,6 +15,11 @@ import ( "github.com/coder/coder/coderd/rbac" ) +// IsDBAuthzStore is provided for unit testing only. +type IsDBAuthzStore interface { + UnderlyingDatabase() database.Store +} + var _ database.Store = (*querier)(nil) // NoActorError wraps ErrNoRows for the api to return a 404. This is the correct @@ -99,6 +104,12 @@ func New(db database.Store, authorizer rbac.Authorizer, logger slog.Logger) data } } +// UnderlyingDatabase is dangerous and should not be used. This is only provided to accommodate some +// unit testing that has to manipulate the database directly. +func (q *querier) UnderlyingDatabase() database.Store { + return q.db +} + // authorizeContext is a helper function to authorize an action on an object. func (q *querier) authorizeContext(ctx context.Context, action rbac.Action, object rbac.Objecter) error { act, ok := ActorFromContext(ctx) diff --git a/coderd/database/dbauthz/system_test.go b/coderd/database/dbauthz/system_test.go index 8400b27407c05..8ab78b216a11f 100644 --- a/coderd/database/dbauthz/system_test.go +++ b/coderd/database/dbauthz/system_test.go @@ -3,6 +3,7 @@ package dbauthz_test import ( "context" "database/sql" + "encoding/json" "time" "github.com/google/uuid" @@ -244,7 +245,7 @@ func (s *MethodTestSuite) TestSystemFunctions() { j := dbgen.ProvisionerJob(s.T(), db, database.ProvisionerJob{ StartedAt: sql.NullTime{Valid: false}, }) - check.Args(database.AcquireProvisionerJobParams{Types: []database.ProvisionerType{j.Provisioner}}). + check.Args(database.AcquireProvisionerJobParams{Types: []database.ProvisionerType{j.Provisioner}, Tags: must(json.Marshal(j.Tags))}). Asserts( /*rbac.ResourceSystem, rbac.ActionUpdate*/ ) })) s.Run("UpdateProvisionerJobWithCompleteByID", s.Subtest(func(db database.Store, check *expects) { diff --git a/coderd/database/dbgen/generator.go b/coderd/database/dbgen/generator.go index 7ffea3a45aff1..1096a2f08eb5a 100644 --- a/coderd/database/dbgen/generator.go +++ b/coderd/database/dbgen/generator.go @@ -277,8 +277,13 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) dat // ProvisionerJob might not have all the correct values like CompletedAt and CancelledAt. This is because // the workspaceBuild is required to fetch those, func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJob) database.ProvisionerJob { + if dba, ok := db.(dbauthz.IsDBAuthzStore); ok { + db = dba.UnderlyingDatabase() + } + id := takeFirst(orig.ID, uuid.New()) - if !orig.StartedAt.Time.IsZero() { + // Always set some tags to prevent Acquire from grabbing jobs it should not. + if !orig.StartedAt.Time.IsZero() || orig.Tags == nil { if orig.Tags == nil { orig.Tags = make(dbtype.StringMap) } @@ -300,10 +305,19 @@ func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJo }) require.NoError(t, err, "insert job") + if !orig.StartedAt.Time.IsZero() { + job, err = db.AcquireProvisionerJob(genCtx, database.AcquireProvisionerJobParams{ + StartedAt: orig.StartedAt, + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + Tags: must(json.Marshal(orig.Tags)), + }) + require.NoError(t, err) + } + if !orig.CompletedAt.Time.IsZero() || orig.Error.String != "" { err := db.UpdateProvisionerJobWithCompleteByID(genCtx, database.UpdateProvisionerJobWithCompleteByIDParams{ ID: job.ID, - UpdatedAt: orig.UpdatedAt, + UpdatedAt: job.UpdatedAt, CompletedAt: orig.CompletedAt, Error: orig.Error, ErrorCode: orig.ErrorCode, @@ -318,14 +332,9 @@ func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJo }) require.NoError(t, err) } - if !orig.StartedAt.Time.IsZero() { - job, err = db.AcquireProvisionerJob(genCtx, database.AcquireProvisionerJobParams{ - StartedAt: orig.StartedAt, - Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, - Tags: must(json.Marshal(orig.Tags)), - }) - require.NoError(t, err) - } + + job, err = db.GetProvisionerJobByID(genCtx, job.ID) + require.NoError(t, err) return job } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index c516a3f5a3a3d..c8dd1be6c23e9 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -608,13 +608,27 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { }) var err error - job, err = db.GetProvisionerJobByID(ctx, job.IDtus - ) + job, err = db.GetProvisionerJobByID(ctx, job.ID) require.NoError(t, err) return workspace, build, job } + // pending + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusPending), + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Valid: false}, + }, database.WorkspaceTransitionStart) + + // starting + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusStarting), + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + }, database.WorkspaceTransitionStart) + + // running _, _, _ = makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusRunning), }, database.ProvisionerJob{ @@ -622,24 +636,68 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, }, database.WorkspaceTransitionStart) - workspaces, err := client.Workspaces(context.Background(), codersdk.WorkspaceFilter{}) - require.NoError(t, err) - - for _, apiWorkspace := range workspaces.Workspaces { - require.Equal(t, apiWorkspace.Name, string(apiWorkspace.LatestBuild.Status)) - } - - // pending - // starting - // running // stopping + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusStopping), + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + }, database.WorkspaceTransitionStop) + // stopped + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusStopped), + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, + }, database.WorkspaceTransitionStop) + // failed + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusFailed), + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, + Error: sql.NullString{String: "Some error", Valid: true}, + }, database.WorkspaceTransitionStart) + // canceling + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusCanceling), + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + CanceledAt: sql.NullTime{Time: time.Now(), Valid: true}, + }, database.WorkspaceTransitionStart) + // canceled - // deleted + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusCanceled), + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + CanceledAt: sql.NullTime{Time: time.Now(), Valid: true}, + CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, + }, database.WorkspaceTransitionStart) + // deleting + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusDeleting), + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + }, database.WorkspaceTransitionDelete) + // deleted + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusDeleted), + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, + }, database.WorkspaceTransitionDelete) + + workspaces, err := client.Workspaces(context.Background(), codersdk.WorkspaceFilter{}) + require.NoError(t, err) + + for _, apiWorkspace := range workspaces.Workspaces { + require.Equal(t, apiWorkspace.Name, string(apiWorkspace.LatestBuild.Status)) + } } // TestWorkspaceFilter creates a set of workspaces, users, and organizations From 93d8bf918c49f28905a267f06978f91a08346f2e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 30 May 2023 14:40:24 -0400 Subject: [PATCH 05/13] Import order --- coderd/database/dbauthz/dbauthz.go | 11 ----------- coderd/database/dbgen/generator.go | 12 +++--------- coderd/workspaces_test.go | 24 +++++++++++++----------- 3 files changed, 16 insertions(+), 31 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index bacf0afbd9341..b823f7bd7067f 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -15,11 +15,6 @@ import ( "github.com/coder/coder/coderd/rbac" ) -// IsDBAuthzStore is provided for unit testing only. -type IsDBAuthzStore interface { - UnderlyingDatabase() database.Store -} - var _ database.Store = (*querier)(nil) // NoActorError wraps ErrNoRows for the api to return a 404. This is the correct @@ -104,12 +99,6 @@ func New(db database.Store, authorizer rbac.Authorizer, logger slog.Logger) data } } -// UnderlyingDatabase is dangerous and should not be used. This is only provided to accommodate some -// unit testing that has to manipulate the database directly. -func (q *querier) UnderlyingDatabase() database.Store { - return q.db -} - // authorizeContext is a helper function to authorize an action on an object. func (q *querier) authorizeContext(ctx context.Context, action rbac.Action, object rbac.Objecter) error { act, ok := ActorFromContext(ctx) diff --git a/coderd/database/dbgen/generator.go b/coderd/database/dbgen/generator.go index 1096a2f08eb5a..a55ed4d5a8b2d 100644 --- a/coderd/database/dbgen/generator.go +++ b/coderd/database/dbgen/generator.go @@ -11,17 +11,15 @@ import ( "testing" "time" - "github.com/coder/coder/coderd/database/dbtype" - - "github.com/coder/coder/coderd/database/dbauthz" - "github.com/coder/coder/coderd/rbac" - "github.com/google/uuid" "github.com/moby/moby/pkg/namesgenerator" "github.com/stretchr/testify/require" "github.com/tabbed/pqtype" "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/database/dbauthz" + "github.com/coder/coder/coderd/database/dbtype" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/cryptorand" ) @@ -277,10 +275,6 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) dat // ProvisionerJob might not have all the correct values like CompletedAt and CancelledAt. This is because // the workspaceBuild is required to fetch those, func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJob) database.ProvisionerJob { - if dba, ok := db.(dbauthz.IsDBAuthzStore); ok { - db = dba.UnderlyingDatabase() - } - id := takeFirst(orig.ID, uuid.New()) // Always set some tags to prevent Acquire from grabbing jobs it should not. if !orig.StartedAt.Time.IsZero() || orig.Tags == nil { diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index c8dd1be6c23e9..db64eb47afcd0 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -9,21 +9,19 @@ import ( "testing" "time" - "github.com/coder/coder/coderd/database/dbauthz" - - "github.com/coder/coder/coderd/database/dbgen" - "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/agent" "github.com/coder/coder/coderd/audit" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/database" + "github.com/coder/coder/coderd/database/dbauthz" + "github.com/coder/coder/coderd/database/dbgen" + "github.com/coder/coder/coderd/database/dbtestutil" "github.com/coder/coder/coderd/parameter" "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/coderd/schedule" @@ -562,32 +560,36 @@ func TestWorkspaceByOwnerAndName(t *testing.T) { func TestWorkspaceFilterAllStatus(t *testing.T) { ctx := dbauthz.AsSystemRestricted(context.Background()) - client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{}) + db, pubsub := dbtestutil.NewDB(t) + client := coderdtest.New(t, &coderdtest.Options{ + Database: db, + Pubsub: pubsub, + }) owner := coderdtest.CreateFirstUser(t, client) - file := dbgen.File(t, api.Database, database.File{ + file := dbgen.File(t, db, database.File{ CreatedBy: owner.UserID, }) - versionJob := dbgen.ProvisionerJob(t, api.Database, database.ProvisionerJob{ + versionJob := dbgen.ProvisionerJob(t, db, database.ProvisionerJob{ OrganizationID: owner.OrganizationID, InitiatorID: owner.UserID, WorkerID: uuid.NullUUID{}, FileID: file.ID, }) - version := dbgen.TemplateVersion(t, api.Database, database.TemplateVersion{ + version := dbgen.TemplateVersion(t, db, database.TemplateVersion{ OrganizationID: owner.OrganizationID, JobID: versionJob.ID, CreatedBy: owner.UserID, }) - template := dbgen.Template(t, api.Database, database.Template{ + template := dbgen.Template(t, db, database.Template{ OrganizationID: owner.OrganizationID, ActiveVersionID: version.ID, CreatedBy: owner.UserID, }) makeWorkspace := func(workspace database.Workspace, job database.ProvisionerJob, transition database.WorkspaceTransition) (database.Workspace, database.WorkspaceBuild, database.ProvisionerJob) { - db := api.Database + db := db workspace.OwnerID = owner.UserID workspace.OrganizationID = owner.OrganizationID From 6fd2b21eb3f8b2538833148cf0b845f0a0034092 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 30 May 2023 15:52:23 -0400 Subject: [PATCH 06/13] Fix test for slow cases --- coderd/workspaces_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index db64eb47afcd0..52fb91eddcc9a 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -5,6 +5,7 @@ import ( "database/sql" "fmt" "net/http" + "os" "strings" "testing" "time" @@ -558,7 +559,13 @@ func TestWorkspaceByOwnerAndName(t *testing.T) { }) } +// TestWorkspaceFilterAllStatus tests workspace status is correctly set given a set of conditions. func TestWorkspaceFilterAllStatus(t *testing.T) { + t.Parallel() + if os.Getenv("DB") != "" { + t.Skip(`This test takes too long with an actual database`) + } + ctx := dbauthz.AsSystemRestricted(context.Background()) db, pubsub := dbtestutil.NewDB(t) client := coderdtest.New(t, &coderdtest.Options{ From a4ba58d3e22b71295f6d11543dacc5769825f4d0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 31 May 2023 09:25:45 -0400 Subject: [PATCH 07/13] Add unit test to check for filter status --- coderd/database/dbgen/generator.go | 2 +- coderd/workspaces_test.go | 27 +++++++++++++++++++++++++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/coderd/database/dbgen/generator.go b/coderd/database/dbgen/generator.go index a55ed4d5a8b2d..a1db183dc554c 100644 --- a/coderd/database/dbgen/generator.go +++ b/coderd/database/dbgen/generator.go @@ -277,7 +277,7 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) dat func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJob) database.ProvisionerJob { id := takeFirst(orig.ID, uuid.New()) // Always set some tags to prevent Acquire from grabbing jobs it should not. - if !orig.StartedAt.Time.IsZero() || orig.Tags == nil { + if !orig.StartedAt.Time.IsZero() { if orig.Tags == nil { orig.Tags = make(dbtype.StringMap) } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 52fb91eddcc9a..7438b38e93acd 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -23,6 +23,7 @@ import ( "github.com/coder/coder/coderd/database/dbauthz" "github.com/coder/coder/coderd/database/dbgen" "github.com/coder/coder/coderd/database/dbtestutil" + "github.com/coder/coder/coderd/database/dbtype" "github.com/coder/coder/coderd/parameter" "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/coderd/schedule" @@ -583,6 +584,9 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { InitiatorID: owner.UserID, WorkerID: uuid.NullUUID{}, FileID: file.ID, + Tags: dbtype.StringMap{ + "custom": "true", + }, }) version := dbgen.TemplateVersion(t, db, database.TemplateVersion{ OrganizationID: owner.OrganizationID, @@ -701,11 +705,30 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, }, database.WorkspaceTransitionDelete) - workspaces, err := client.Workspaces(context.Background(), codersdk.WorkspaceFilter{}) + apiCtx, cancel := context.WithTimeout(ctx, testutil.WaitShort) + defer cancel() + workspaces, err := client.Workspaces(apiCtx, codersdk.WorkspaceFilter{}) require.NoError(t, err) + // Make sure all workspaces have the correct status + var statuses []codersdk.WorkspaceStatus for _, apiWorkspace := range workspaces.Workspaces { - require.Equal(t, apiWorkspace.Name, string(apiWorkspace.LatestBuild.Status)) + assert.Equal(t, apiWorkspace.Name, string(apiWorkspace.LatestBuild.Status), "workspace has incorrect status") + statuses = append(statuses, apiWorkspace.LatestBuild.Status) + } + + // Now test the filter + for _, status := range statuses { + ctx, cancel := context.WithTimeout(ctx, testutil.WaitShort) + defer cancel() + + workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{ + Status: string(status), + }) + require.NoErrorf(t, err, "fetch with status: %s", status) + for _, workspace := range workspaces.Workspaces { + assert.Equal(t, status, workspace.LatestBuild.Status, "expect matching status to filter") + } } } From 8704afdb24a0a383be1fe6c8fd526cf9e70e0f80 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 31 May 2023 09:55:53 -0400 Subject: [PATCH 08/13] Match dbfake to postgres --- coderd/database/dbfake/databasefake.go | 89 +++++++++++++++++--------- coderd/database/queries.sql.go | 6 +- coderd/database/queries/workspaces.sql | 6 +- 3 files changed, 65 insertions(+), 36 deletions(-) diff --git a/coderd/database/dbfake/databasefake.go b/coderd/database/dbfake/databasefake.go index c2e42f5c89fc7..4db06de9128cd 100644 --- a/coderd/database/dbfake/databasefake.go +++ b/coderd/database/dbfake/databasefake.go @@ -1194,76 +1194,91 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database. return nil, xerrors.Errorf("get provisioner job: %w", err) } + // How this works is you need to 'continue' if the status does + // not match. I do 'if/else' so I can just match the postgres logic. switch database.WorkspaceStatus(arg.Status) { case database.WorkspaceStatusPending: - if !job.StartedAt.Valid { + if isNull(job.StartedAt) { + } else { continue } case database.WorkspaceStatusStarting: - if !job.StartedAt.Valid && - !job.CanceledAt.Valid && - job.CompletedAt.Valid && - time.Since(job.UpdatedAt) > 30*time.Second || - build.Transition != database.WorkspaceTransitionStart { + if isNotNull(job.StartedAt) && + isNull(job.CanceledAt) && + isNull(job.CompletedAt) && + time.Since(job.UpdatedAt) < 30*time.Second && + build.Transition == database.WorkspaceTransitionStart { + } else { continue } case database.WorkspaceStatusRunning: - if !job.CompletedAt.Valid && - job.CanceledAt.Valid && - job.Error.Valid || - build.Transition != database.WorkspaceTransitionStart { + if isNotNull(job.CompletedAt) && + isNull(job.CanceledAt) && + isNull(job.Error) && + build.Transition == database.WorkspaceTransitionStart { + } else { continue } case database.WorkspaceStatusStopping: - if !job.StartedAt.Valid && - !job.CanceledAt.Valid && - job.CompletedAt.Valid && - time.Since(job.UpdatedAt) > 30*time.Second || - build.Transition != database.WorkspaceTransitionStop { + if isNotNull(job.StartedAt) && + isNull(job.CanceledAt) && + isNull(job.CompletedAt) && + time.Since(job.UpdatedAt) < 30*time.Second && + build.Transition == database.WorkspaceTransitionStop { + } else { continue } case database.WorkspaceStatusStopped: - if !job.CompletedAt.Valid && - job.CanceledAt.Valid && - job.Error.Valid || - build.Transition != database.WorkspaceTransitionStop { + if isNotNull(job.CompletedAt) && + isNull(job.CanceledAt) && + isNull(job.Error) && + build.Transition == database.WorkspaceTransitionStop { + } else { continue } case database.WorkspaceStatusFailed: - if (!job.CanceledAt.Valid && !job.Error.Valid) || - (!job.CompletedAt.Valid && !job.Error.Valid) { + if (isNotNull(job.CanceledAt) && isNotNull(job.Error)) || + (isNotNull(job.CompletedAt) && isNotNull(job.Error)) { + } else { continue } case database.WorkspaceStatusCanceling: - if !job.CanceledAt.Valid && job.CompletedAt.Valid { + if isNotNull(job.CanceledAt) && + isNull(job.CompletedAt) { + } else { continue } case database.WorkspaceStatusCanceled: - if !job.CanceledAt.Valid && !job.CompletedAt.Valid { + if isNotNull(job.CanceledAt) && + isNotNull(job.CompletedAt) { + } else { continue } case database.WorkspaceStatusDeleted: - if !job.StartedAt.Valid && - job.CanceledAt.Valid && - !job.CompletedAt.Valid && - time.Since(job.UpdatedAt) > 30*time.Second || - build.Transition != database.WorkspaceTransitionDelete { + if isNotNull(job.StartedAt) && + isNull(job.CanceledAt) && + isNotNull(job.CompletedAt) && + time.Since(job.UpdatedAt) < 30*time.Second && + build.Transition == database.WorkspaceTransitionDelete && + isNull(job.Error) { + } else { continue } case database.WorkspaceStatusDeleting: - if !job.CompletedAt.Valid && - job.CanceledAt.Valid && - job.Error.Valid && - build.Transition != database.WorkspaceTransitionDelete { + if isNull(job.CompletedAt) && + isNull(job.CanceledAt) && + isNull(job.Error) && + build.Transition == database.WorkspaceTransitionDelete { + } else { continue } @@ -5331,3 +5346,13 @@ func (q *fakeQuerier) UpdateWorkspaceProxyDeleted(_ context.Context, arg databas } return sql.ErrNoRows } + +// isNull is only used in dbfake, so reflect is ok. Use this to make the logic +// look more similar to the postgres. +func isNull(v interface{}) bool { + return !isNotNull(v) +} + +func isNotNull(v interface{}) bool { + return reflect.ValueOf(v).FieldByName("Valid").Bool() +} diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 842dbd87ba75a..df1fe552bfcff 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -8252,10 +8252,12 @@ WHERE latest_build.canceled_at IS NULL AND latest_build.completed_at IS NOT NULL AND latest_build.updated_at - INTERVAL '30 seconds' < NOW() AND - latest_build.transition = 'delete'::workspace_transition + latest_build.transition = 'delete'::workspace_transition AND + -- If the error field is null, the status is 'failed' + latest_build.error IS NULL WHEN $2 = 'deleting' THEN - latest_build.completed_at IS NOT NULL AND + latest_build.completed_at IS NULL AND latest_build.canceled_at IS NULL AND latest_build.error IS NULL AND latest_build.transition = 'delete'::workspace_transition diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index fda15b2a0f9dc..1cbae793e60cd 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -157,10 +157,12 @@ WHERE latest_build.canceled_at IS NULL AND latest_build.completed_at IS NOT NULL AND latest_build.updated_at - INTERVAL '30 seconds' < NOW() AND - latest_build.transition = 'delete'::workspace_transition + latest_build.transition = 'delete'::workspace_transition AND + -- If the error field is null, the status is 'failed' + latest_build.error IS NULL WHEN @status = 'deleting' THEN - latest_build.completed_at IS NOT NULL AND + latest_build.completed_at IS NULL AND latest_build.canceled_at IS NULL AND latest_build.error IS NULL AND latest_build.transition = 'delete'::workspace_transition From 01e9fb0e66e62d4bc4dbf73ef3b3e1a8eb14e9f9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 31 May 2023 09:59:31 -0400 Subject: [PATCH 09/13] Fix test --- coderd/workspaces_test.go | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 7438b38e93acd..b6ca47a8a15ac 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -1,8 +1,10 @@ package coderd_test import ( + "bytes" "context" "database/sql" + "encoding/json" "fmt" "net/http" "os" @@ -564,7 +566,7 @@ func TestWorkspaceByOwnerAndName(t *testing.T) { func TestWorkspaceFilterAllStatus(t *testing.T) { t.Parallel() if os.Getenv("DB") != "" { - t.Skip(`This test takes too long with an actual database`) + t.Skip(`This test takes too long with an actual database. Takes 10s on local machine`) } ctx := dbauthz.AsSystemRestricted(context.Background()) @@ -607,8 +609,14 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { workspace.TemplateID = template.ID workspace = dbgen.Workspace(t, db, workspace) + jobID := uuid.New() + job.ID = jobID job.Type = database.ProvisionerJobTypeWorkspaceBuild job.OrganizationID = owner.OrganizationID + // Need to prevent acquire from getting this job. + job.Tags = dbtype.StringMap{ + jobID.String(): "true", + } job = dbgen.ProvisionerJob(t, db, job) build := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ @@ -664,14 +672,23 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, }, database.WorkspaceTransitionStop) - // failed + // failed -- delete _, _, _ = makeWorkspace(database.Workspace{ - Name: string(database.WorkspaceStatusFailed), + Name: string(database.WorkspaceStatusFailed) + "-deleted", }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, Error: sql.NullString{String: "Some error", Valid: true}, - }, database.WorkspaceTransitionStart) + }, database.WorkspaceTransitionDelete) + + // failed -- stop + _, _, _ = makeWorkspace(database.Workspace{ + Name: string(database.WorkspaceStatusFailed) + "-stopped", + }, database.ProvisionerJob{ + StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, + CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, + Error: sql.NullString{String: "Some error", Valid: true}, + }, database.WorkspaceTransitionStop) // canceling _, _, _ = makeWorkspace(database.Workspace{ @@ -713,7 +730,13 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { // Make sure all workspaces have the correct status var statuses []codersdk.WorkspaceStatus for _, apiWorkspace := range workspaces.Workspaces { - assert.Equal(t, apiWorkspace.Name, string(apiWorkspace.LatestBuild.Status), "workspace has incorrect status") + expStatus := strings.Split(apiWorkspace.Name, "-") + if !assert.Equal(t, expStatus[0], string(apiWorkspace.LatestBuild.Status), "workspace has incorrect status") { + d, _ := json.Marshal(apiWorkspace) + var buf bytes.Buffer + _ = json.Indent(&buf, d, "", "\t") + t.Logf("Incorrect workspace: %s", buf.String()) + } statuses = append(statuses, apiWorkspace.LatestBuild.Status) } From befd8eb5a85a225f3b4b7510f54422fabeea11e4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 31 May 2023 10:17:15 -0400 Subject: [PATCH 10/13] Linting --- coderd/database/dbfake/databasefake.go | 77 ++++++++------------------ coderd/database/dbgen/generator.go | 3 +- coderd/workspaces_test.go | 22 ++++---- 3 files changed, 36 insertions(+), 66 deletions(-) diff --git a/coderd/database/dbfake/databasefake.go b/coderd/database/dbfake/databasefake.go index 4db06de9128cd..d4c2160003740 100644 --- a/coderd/database/dbfake/databasefake.go +++ b/coderd/database/dbfake/databasefake.go @@ -1194,97 +1194,68 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database. return nil, xerrors.Errorf("get provisioner job: %w", err) } - // How this works is you need to 'continue' if the status does - // not match. I do 'if/else' so I can just match the postgres logic. + // This logic should match the logic in the workspace.sql file. + var statusMatch bool switch database.WorkspaceStatus(arg.Status) { case database.WorkspaceStatusPending: - if isNull(job.StartedAt) { - } else { - continue - } - + statusMatch = isNull(job.StartedAt) case database.WorkspaceStatusStarting: - if isNotNull(job.StartedAt) && + statusMatch = isNotNull(job.StartedAt) && isNull(job.CanceledAt) && isNull(job.CompletedAt) && time.Since(job.UpdatedAt) < 30*time.Second && - build.Transition == database.WorkspaceTransitionStart { - } else { - continue - } + build.Transition == database.WorkspaceTransitionStart case database.WorkspaceStatusRunning: - if isNotNull(job.CompletedAt) && + statusMatch = isNotNull(job.CompletedAt) && isNull(job.CanceledAt) && isNull(job.Error) && - build.Transition == database.WorkspaceTransitionStart { - } else { - continue - } + build.Transition == database.WorkspaceTransitionStart case database.WorkspaceStatusStopping: - if isNotNull(job.StartedAt) && + statusMatch = isNotNull(job.StartedAt) && isNull(job.CanceledAt) && isNull(job.CompletedAt) && time.Since(job.UpdatedAt) < 30*time.Second && - build.Transition == database.WorkspaceTransitionStop { - } else { - continue - } + build.Transition == database.WorkspaceTransitionStop case database.WorkspaceStatusStopped: - if isNotNull(job.CompletedAt) && + statusMatch = isNotNull(job.CompletedAt) && isNull(job.CanceledAt) && isNull(job.Error) && - build.Transition == database.WorkspaceTransitionStop { - } else { - continue - } - + build.Transition == database.WorkspaceTransitionStop case database.WorkspaceStatusFailed: - if (isNotNull(job.CanceledAt) && isNotNull(job.Error)) || - (isNotNull(job.CompletedAt) && isNotNull(job.Error)) { - } else { - continue - } + statusMatch = (isNotNull(job.CanceledAt) && isNotNull(job.Error)) || + (isNotNull(job.CompletedAt) && isNotNull(job.Error)) case database.WorkspaceStatusCanceling: - if isNotNull(job.CanceledAt) && - isNull(job.CompletedAt) { - } else { - continue - } + statusMatch = isNotNull(job.CanceledAt) && + isNull(job.CompletedAt) case database.WorkspaceStatusCanceled: - if isNotNull(job.CanceledAt) && - isNotNull(job.CompletedAt) { - } else { - continue - } + statusMatch = isNotNull(job.CanceledAt) && + isNotNull(job.CompletedAt) case database.WorkspaceStatusDeleted: - if isNotNull(job.StartedAt) && + statusMatch = isNotNull(job.StartedAt) && isNull(job.CanceledAt) && isNotNull(job.CompletedAt) && time.Since(job.UpdatedAt) < 30*time.Second && build.Transition == database.WorkspaceTransitionDelete && - isNull(job.Error) { - } else { - continue - } + isNull(job.Error) case database.WorkspaceStatusDeleting: - if isNull(job.CompletedAt) && + statusMatch = isNull(job.CompletedAt) && isNull(job.CanceledAt) && isNull(job.Error) && - build.Transition == database.WorkspaceTransitionDelete { - } else { - continue - } + build.Transition == database.WorkspaceTransitionDelete default: return nil, xerrors.Errorf("unknown workspace status in filter: %q", arg.Status) } + if !statusMatch { + continue + } } if arg.HasAgent != "" { diff --git a/coderd/database/dbgen/generator.go b/coderd/database/dbgen/generator.go index a1db183dc554c..74adba2db42c2 100644 --- a/coderd/database/dbgen/generator.go +++ b/coderd/database/dbgen/generator.go @@ -272,8 +272,7 @@ func GroupMember(t testing.TB, db database.Store, orig database.GroupMember) dat return member } -// ProvisionerJob might not have all the correct values like CompletedAt and CancelledAt. This is because -// the workspaceBuild is required to fetch those, +// ProvisionerJob is a bit more involved to get the values such as "completedAt", "startedAt", "cancelledAt" set. func ProvisionerJob(t testing.TB, db database.Store, orig database.ProvisionerJob) database.ProvisionerJob { id := takeFirst(orig.ID, uuid.New()) // Always set some tags to prevent Acquire from grabbing jobs it should not. diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index b6ca47a8a15ac..02d874c3b6806 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -636,21 +636,21 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { } // pending - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusPending), }, database.ProvisionerJob{ StartedAt: sql.NullTime{Valid: false}, }, database.WorkspaceTransitionStart) // starting - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusStarting), }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, }, database.WorkspaceTransitionStart) // running - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusRunning), }, database.ProvisionerJob{ CompletedAt: sql.NullTime{Time: time.Now(), Valid: true}, @@ -658,14 +658,14 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { }, database.WorkspaceTransitionStart) // stopping - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusStopping), }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, }, database.WorkspaceTransitionStop) // stopped - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusStopped), }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, @@ -673,7 +673,7 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { }, database.WorkspaceTransitionStop) // failed -- delete - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusFailed) + "-deleted", }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, @@ -682,7 +682,7 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { }, database.WorkspaceTransitionDelete) // failed -- stop - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusFailed) + "-stopped", }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, @@ -691,7 +691,7 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { }, database.WorkspaceTransitionStop) // canceling - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusCanceling), }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, @@ -699,7 +699,7 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { }, database.WorkspaceTransitionStart) // canceled - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusCanceled), }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, @@ -708,14 +708,14 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { }, database.WorkspaceTransitionStart) // deleting - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusDeleting), }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, }, database.WorkspaceTransitionDelete) // deleted - _, _, _ = makeWorkspace(database.Workspace{ + makeWorkspace(database.Workspace{ Name: string(database.WorkspaceStatusDeleted), }, database.ProvisionerJob{ StartedAt: sql.NullTime{Time: time.Now().Add(time.Second * -2), Valid: true}, From 558ef48d97f870abba9d64ae96de70f649efa0db Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 31 May 2023 10:48:10 -0400 Subject: [PATCH 11/13] Linting --- coderd/workspaces_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 02d874c3b6806..222cf95ce0515 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -569,6 +569,7 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { t.Skip(`This test takes too long with an actual database. Takes 10s on local machine`) } + // nolint:gocritic -- unit testing ctx := dbauthz.AsSystemRestricted(context.Background()) db, pubsub := dbtestutil.NewDB(t) client := coderdtest.New(t, &coderdtest.Options{ @@ -743,7 +744,6 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { // Now test the filter for _, status := range statuses { ctx, cancel := context.WithTimeout(ctx, testutil.WaitShort) - defer cancel() workspaces, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{ Status: string(status), @@ -752,6 +752,7 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { for _, workspace := range workspaces.Workspaces { assert.Equal(t, status, workspace.LatestBuild.Status, "expect matching status to filter") } + cancel() } } From aee97f8677b0bb684b983f42104ca86f8ea64756 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 31 May 2023 11:01:49 -0400 Subject: [PATCH 12/13] Add dbauthz system comment --- coderd/workspaces_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 222cf95ce0515..1e2cc41835d59 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -569,7 +569,8 @@ func TestWorkspaceFilterAllStatus(t *testing.T) { t.Skip(`This test takes too long with an actual database. Takes 10s on local machine`) } - // nolint:gocritic -- unit testing + // For this test, we do not care about permissions. + // nolint:gocritic // unit testing ctx := dbauthz.AsSystemRestricted(context.Background()) db, pubsub := dbtestutil.NewDB(t) client := coderdtest.New(t, &coderdtest.Options{ From f6857d5a4f363b78e6b85098d45fdb47eafba669 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 1 Jun 2023 08:28:13 -0400 Subject: [PATCH 13/13] Fix comment --- coderd/database/queries.sql.go | 2 +- coderd/database/queries/workspaces.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index df1fe552bfcff..1f727f7832e8a 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -8253,7 +8253,7 @@ WHERE latest_build.completed_at IS NOT NULL AND latest_build.updated_at - INTERVAL '30 seconds' < NOW() AND latest_build.transition = 'delete'::workspace_transition AND - -- If the error field is null, the status is 'failed' + -- If the error field is not null, the status is 'failed' latest_build.error IS NULL WHEN $2 = 'deleting' THEN diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index 1cbae793e60cd..f5d7c35cfd929 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -158,7 +158,7 @@ WHERE latest_build.completed_at IS NOT NULL AND latest_build.updated_at - INTERVAL '30 seconds' < NOW() AND latest_build.transition = 'delete'::workspace_transition AND - -- If the error field is null, the status is 'failed' + -- If the error field is not null, the status is 'failed' latest_build.error IS NULL WHEN @status = 'deleting' THEN