Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: notify template owner on manual build failures#14262

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
BrunoQuaresma wants to merge8 commits intomainfrombq/notify-manual-build-failure

Conversation

BrunoQuaresma
Copy link
Collaborator

Preview:

Screenshot 2024-08-13 at 15 22 02

Related tocoder/internal#16 and it depends on#14250

@@ -1020,7 +1020,23 @@ func (s *server) FailJob(ctx context.Context, failJob *proto.FailedJob) (*proto.
return nil, err
}

s.notifyWorkspaceBuildFailed(ctx, workspace, build)
// Only notify auto build failures
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I would rather that this comment explain what classifies as an auto build (i.e. not initiated by a user)

s.Logger.Warn(ctx, "failed to find template admins for template build failed notification", slog.Error(err))
} else {
for _, admin := range admins {
if admin.ID != build.InitiatorID && !autoBuild {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
ifadmin.ID!=build.InitiatorID&&!autoBuild {
// Only notify users who did not initiate the action.
ifadmin.ID!=build.InitiatorID&&!autoBuild {

reason = string(build.Reason)

if _, err := s.NotificationsEnqueuer.Enqueue(ctx, workspace.OwnerID, notifications.TemplateWorkspaceAutobuildFailed,
func (s *server) notifyWorkspaceBuildFailed(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild, receiverID uuid.UUID) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't understand why you're passing inreceiverID here. Can you elaborate pls?

func (s *server) notifyTemplateBuildFailed(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild, receiverID uuid.UUID) {
template, err := s.Database.GetTemplateByID(ctx, workspace.TemplateID)
if err != nil {
s.Logger.Warn(ctx, "failed to get template", slog.Error(err))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Think about how an operator might debug with these logs; how would they know that this is related to sending notifications?

Try think of log lines as communication. You're communicating with an operator who is a) possibly not familiar with the product or less experienced, and b) trying to debug an issue, possibly under stress. Vague log lines are almost as bad as no log lines at all, sometimes worse because they can mislead or distract.

for _, c := range tc {
t.Run(c.name, func(t *testing.T) {
// Given: a template and a workspace build
isManualBuild := c.reason == database.BuildReasonInitiator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Since you have this logic in multiple places, it might be worth creating a helper method.

for _, c := range tc {
t.Run(c.name, func(t *testing.T) {
// Given: a template and a workspace build
isManualBuild := c.reason == database.BuildReasonInitiator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Try and group your code into logical blocks, and use whitespace for visual assistance. Think of your code as having the same visual hierarchy that you'd design in your frontends.

For example,isManualBuild is only needed lower down: group them logically.

You've got a whole lot of code below which is doing several things; add a bit of spaces and/or comments to give the reader's "eye" some space to "breathe", and group related concepts/lines together to infer relationships.

// Then: send the template build failed notifications
var buildFailedNotifications []*testutil.Notification
for _, n := range notifEnq.Sent {
if n.TemplateID == notifications.TemplateTemplateBuildFailed {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What if they're not the template you're expecting?

Your tests should not only prove that something works; it should also prove that itonly works inprecisely the way you're expecting, and any relevant deviation should fail the test.

@mtojek
Copy link
Member

@BrunoQuaresma I will take over this PR, not sure if I create a one or modify yours, just FYI 👍

BrunoQuaresma reacted with thumbs up emoji

@BrunoQuaresma
Copy link
CollaboratorAuthor

@BrunoQuaresma I will take over this PR, not sure if I create a one or modify yours, just FYI 👍

Sure, whatever you think is best. Just tag me on your PR so I can take a look and learn.

@mtojek
Copy link
Member

Close in favor of#14419

@mtojekmtojek closed thisAug 26, 2024
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsAug 26, 2024
@github-actionsgithub-actionsbot deleted the bq/notify-manual-build-failure branchFebruary 20, 2025 00:06
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@dannykoppingdannykoppingdannykopping left review comments

@stirbystirbyAwaiting requested review from stirby

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@BrunoQuaresma@mtojek@dannykopping@stirby

[8]ページ先頭

©2009-2025 Movatter.jp