-
Notifications
You must be signed in to change notification settings - Fork 41
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
Unified PR machinery for actions like pull request comment #5223
base: main
Are you sure you want to change the base?
Changes from all commits
055518c
62bf627
4ff339f
7ce24f0
14bd995
e99d9ee
96be386
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,78 @@ | ||||||||||
// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors | ||||||||||
// SPDX-License-Identifier: Apache-2.0 | ||||||||||
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
package pull_request_comment | ||||||||||
|
||||||||||
import ( | ||||||||||
"context" | ||||||||||
"fmt" | ||||||||||
"slices" | ||||||||||
"strings" | ||||||||||
|
||||||||||
"github.com/rs/zerolog" | ||||||||||
|
||||||||||
"github.com/mindersec/minder/internal/entities/properties" | ||||||||||
provifv1 "github.com/mindersec/minder/pkg/providers/v1" | ||||||||||
) | ||||||||||
|
||||||||||
// alertFlusher aggregates a list of comments and flushes them to the PR | ||||||||||
// as a single comment. The idea is that we can aggregate multiple alerts | ||||||||||
// into a single comment without needing to flood the PR with multiple comments. | ||||||||||
// This is only instantiated once; the first creation is the only one that will | ||||||||||
// be used. | ||||||||||
type alertFlusher struct { | ||||||||||
props *properties.Properties | ||||||||||
commitSha string | ||||||||||
commenter provifv1.PullRequestCommenter | ||||||||||
} | ||||||||||
|
||||||||||
func newAlertFlusher(props *properties.Properties, commitSha string, commenter provifv1.PullRequestCommenter) *alertFlusher { | ||||||||||
return &alertFlusher{ | ||||||||||
props: props, | ||||||||||
commitSha: commitSha, | ||||||||||
commenter: commenter, | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
func (a *alertFlusher) Flush(ctx context.Context, items ...any) error { | ||||||||||
title := title1("Minder Alerts") | ||||||||||
|
||||||||||
aggregatedAlerts := getAlerts(ctx, items...) | ||||||||||
|
||||||||||
_, err := a.commenter.CommentOnPullRequest(ctx, a.props, provifv1.PullRequestCommentInfo{ | ||||||||||
Commit: a.commitSha, | ||||||||||
Body: fmt.Sprintf("%s\n\n%s", title, aggregatedAlerts), | ||||||||||
}) | ||||||||||
if err != nil { | ||||||||||
return fmt.Errorf("error creating PR review: %w", err) | ||||||||||
} | ||||||||||
|
||||||||||
return nil | ||||||||||
} | ||||||||||
|
||||||||||
func getAlerts(ctx context.Context, items ...any) string { | ||||||||||
logger := zerolog.Ctx(ctx) | ||||||||||
|
||||||||||
if len(items) == 0 { | ||||||||||
return "Minder found no issues." | ||||||||||
} | ||||||||||
|
||||||||||
var alerts []string | ||||||||||
|
||||||||||
// iterate and aggregate | ||||||||||
for _, item := range items { | ||||||||||
fp, ok := item.(*provifv1.PullRequestCommentInfo) | ||||||||||
if !ok { | ||||||||||
logger.Error().Msgf("expected PullRequestCommentInfo, got %T", item) | ||||||||||
continue | ||||||||||
} | ||||||||||
|
||||||||||
alerts = append(alerts, alert(fp.Header, fp.Body)) | ||||||||||
} | ||||||||||
|
||||||||||
// Ensure predictable ordering | ||||||||||
// TODO: This should be sorted by severity | ||||||||||
slices.Sort(alerts) | ||||||||||
Comment on lines
+73
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: how is stable ordering guaranteed between two runs? If my reasoning is correct, this might warrant a longer comment explaining how we expect this comment to be "stable". |
||||||||||
|
||||||||||
return strings.Join(alerts, spacing()) | ||||||||||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: given this is Go, I'm not really fond of this approach and would rather replace this with templates which are, for better or worse, more idiomatic. No need to change this now, I'm just sharing a thought. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package pull_request_comment | ||
|
||
import "fmt" | ||
|
||
func formatTitle(displayName string) string { | ||
return fmt.Sprintf("Rule '%s' Alert", displayName) | ||
} | ||
|
||
// Formats the comment for a single alert as markdown | ||
func alert(title, body string) string { | ||
return fmt.Sprintf("%s\n\n%s", title2(title), body) | ||
} | ||
|
||
func title1(title string) string { | ||
return fmt.Sprintf("# %s", title) | ||
} | ||
|
||
func title2(title string) string { | ||
return fmt.Sprintf("## %s", title) | ||
} | ||
|
||
func spacing() string { | ||
return "\n\n" | ||
} | ||
|
||
func separator() string { | ||
return "---" | ||
} |
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.
Would you rely on this to be unique now or at some point? Asking because I think we don't conflict on
display_name
clashes but name only.