-
Notifications
You must be signed in to change notification settings - Fork 903
feat: notify owner about failed autobuild #13891
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM! A few relatively minor points.
coderd/database/migrations/000226_notifications_autobuild_failed.up.sql
Outdated
Show resolved
Hide resolved
ignoreLogErrors := true | ||
srv, db, ps, pd := setup(t, ignoreLogErrors, &overrides{ | ||
notificationEnqueuer: notifEnq, | ||
}) | ||
|
||
user := dbgen.User(t, db, database.User{}) | ||
initiator := user | ||
|
||
template := dbgen.Template(t, db, database.Template{ | ||
Name: "template", | ||
Provisioner: database.ProvisionerTypeEcho, | ||
OrganizationID: pd.OrganizationID, | ||
}) | ||
template, err := db.GetTemplateByID(ctx, template.ID) | ||
require.NoError(t, err) | ||
file := dbgen.File(t, db, database.File{CreatedBy: user.ID}) | ||
workspace := dbgen.Workspace(t, db, database.Workspace{ | ||
TemplateID: template.ID, | ||
OwnerID: user.ID, | ||
OrganizationID: pd.OrganizationID, | ||
}) | ||
version := dbgen.TemplateVersion(t, db, database.TemplateVersion{ | ||
OrganizationID: pd.OrganizationID, | ||
TemplateID: uuid.NullUUID{ | ||
UUID: template.ID, | ||
Valid: true, | ||
}, | ||
JobID: uuid.New(), | ||
}) | ||
build := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ | ||
WorkspaceID: workspace.ID, | ||
TemplateVersionID: version.ID, | ||
InitiatorID: initiator.ID, | ||
Transition: database.WorkspaceTransitionDelete, | ||
Reason: tc.buildReason, | ||
}) | ||
job := dbgen.ProvisionerJob(t, db, ps, database.ProvisionerJob{ | ||
FileID: file.ID, | ||
Type: database.ProvisionerJobTypeWorkspaceBuild, | ||
Input: must(json.Marshal(provisionerdserver.WorkspaceProvisionJob{ | ||
WorkspaceBuildID: build.ID, | ||
})), | ||
OrganizationID: pd.OrganizationID, | ||
}) | ||
_, err = db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{ | ||
OrganizationID: pd.OrganizationID, | ||
WorkerID: uuid.NullUUID{ | ||
UUID: pd.ID, | ||
Valid: true, | ||
}, | ||
Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, | ||
}) | ||
require.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: all the common bits here could be moved out to a helper func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my problem with this idea is that we already have setup()
function, so we will add another, let's say prepare()
, but prepare()
would be used only by 2/10 tests in the unit.
I think I will pass on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some missing/confusing aspects of the tests that I think we need to fix but after that this is ready to go.
Related: coder/internal#7
This PR implements a new notification: workspace autobuild failure.
Changes:
Workspace Autobuild Failed
initiatedBy
withinitiator
initiator
when autobuild deletes a workspace