Unified PR machinery for actions like pull request comment #5223

JAORMX · 2024-12-18T10:43:54Z

Summary

This adds all the needed machinery and implementation to create a unified actions result. This is specially useful for aggregating results from different alerts such as pull_request_comment.

I tried doing it in separate PRs... see #5188 & #5182 but this proved challenging. So now it's all here.

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

internal/providers/github/commenter.go

+type minderTemplateData struct {
+	MagicComment string
+	Body         string
+}


internal/providers/github/commenter.go

+
+		ic, err := c.CreateIssueComment(ctx, owner, repoName,
+			// TODO: overflow
+			int(prNumber), body)


internal/providers/github/commenter.go

+
+func (c *GitHub) createReview(
+	ctx context.Context, comment provifv1.PullRequestCommentInfo, mci magicCommentInfo,
+	owner, repoName string, prNumber int64, commitSha string,


internal/providers/github/commenter.go

+	// TODO: overflow
+	r, err := c.CreateReview(ctx, owner, repoName, int(prNumber), review)
+	if err != nil {
+		return mci, fmt.Errorf("could not create review: %w", err)


This allows for providers to comment on pull requests Signed-off-by: Juan Antonio Osorio <[email protected]>

This shared space will allow actions to gather structures or results that will allow it to aggregate and flush. In more solid terms, we'd be able to aggregate PR comments, iterate them, and issue just one big comment. Signed-off-by: Juan Antonio Osorio <[email protected]>

Signed-off-by: Juan Antonio Osorio <[email protected]>

blkt

First few comments while I keep reviewing the code.

blkt · 2025-01-10T09:47:34Z

internal/engine/actions/alert/pull_request_comment/format.go

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.

blkt · 2025-01-10T10:41:53Z

internal/engine/actions/alert/pull_request_comment/pull_request_comment.go

+	sac.ShareAndRegister("pull_request_comment",
+		newAlertFlusher(params.props, params.props.GetProperty(properties.PullRequestCommitSHA).GetString(), alert.commenter),
+		&provifv1.PullRequestCommentInfo{
+			Header: params.Title,
+			Commit: params.props.GetProperty(properties.PullRequestCommitSHA).GetString(),
+			Body:   params.Comment,
+			// TODO: Determine the priority from the rule type severity
+		})


question: ShareAndRegister adds an alertFlusher to its internal list kept under "pull_request_comment" in this case, which is the only key used.

Why does that need to be a map? What other uses can it have?

blkt · 2025-01-10T10:43:54Z

internal/engine/actions/alert/pull_request_comment/flush.go

+	// Ensure predictable ordering
+	// TODO: This should be sorted by severity
+	slices.Sort(alerts)


question: how is stable ordering guaranteed between two runs?
I assume the main ordering key is actually fp.Header, and if two alerts have the same header then sorting goes further into the string to sort by whatever comes next.

If my reasoning is correct, this might warrant a longer comment explaining how we expect this comment to be "stable".

blkt · 2025-01-10T10:52:45Z

internal/providers/github/commenter.go

question: was this code ported from other source files? Are there any tests somewhere else?

blkt · 2025-01-10T10:53:23Z

internal/engine/actions/context/context.go

+// SPDX-FileCopyrightText: Copyright 2023 The Minder Authors
+// SPDX-License-Identifier: Apache-2.0


Suggested change

// SPDX-FileCopyrightText: Copyright 2023 The Minder Authors

// SPDX-License-Identifier: Apache-2.0

// SPDX-FileCopyrightText: Copyright 2025 The Minder Authors

// SPDX-License-Identifier: Apache-2.0

blkt · 2025-01-10T10:53:36Z

internal/providers/github/commenter.go

+// SPDX-FileCopyrightText: Copyright 2023 The Minder Authors
+// SPDX-License-Identifier: Apache-2.0


Suggested change

// SPDX-FileCopyrightText: Copyright 2023 The Minder Authors

// SPDX-License-Identifier: Apache-2.0

// SPDX-FileCopyrightText: Copyright 2025 The Minder Authors

// SPDX-License-Identifier: Apache-2.0

blkt · 2025-01-10T10:54:01Z

internal/engine/actions/alert/pull_request_comment/flush.go

+// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors
+// SPDX-License-Identifier: Apache-2.0


Suggested change

// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors

// SPDX-License-Identifier: Apache-2.0

// SPDX-FileCopyrightText: Copyright 2025 The Minder Authors

// SPDX-License-Identifier: Apache-2.0

rdimitrov · 2025-01-10T11:10:14Z

internal/engine/actions/alert/alert.go

 	}

 	return nil, fmt.Errorf("unknown alert type: %s", alertCfg.GetType())
 }
+
+func defaultName(ruletype *pb.RuleType) string {


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.

rdimitrov · 2025-01-10T11:21:30Z

internal/engine/actions/alert/pull_request_comment/pull_request_comment.go

-		logger.Info().Str("review_id", params.Metadata.ReviewID).Msg("PR comment dismissed")
-		// Success - return ErrActionTurnedOff to indicate the action was successful
-		return nil, fmt.Errorf("%s : %w", alert.Class(), enginerr.ErrActionTurnedOff)
+		return json.RawMessage(`{}`), nil


What would this do in the case where there was a previous evaluation where one or more rules failed and Minder made a comment, but now it's all good?

rdimitrov · 2025-01-10T11:23:54Z

internal/engine/actions/alert/pull_request_comment/pull_request_comment.go

 	case interfaces.ActionCmdDoNothing:
-		// Return the previous alert status.
+		// If the previous status didn't change (still a failure, for instance) we


Can you share what's the use case you have mind for this?

rdimitrov · 2025-01-10T11:27:16Z

internal/engine/actions/alert/pull_request_comment/pull_request_comment.go

+	}
+
+	// This was a successful result, so we don't need to alert
+	if !params.shouldAlert {


Why do you need to check for this here? My understanding is the shouldAlert flag is used to tell what the alert command should be.

JAORMX requested a review from a team as a code owner December 18, 2024 10:43

JAORMX marked this pull request as draft December 18, 2024 10:48

github-advanced-security bot found potential problems Dec 18, 2024

View reviewed changes

This was referenced Jan 7, 2025

Add PullRequestCommenter provider trait #5188

Closed

Add a shared actions context structure #5182

Closed

JAORMX added 7 commits January 10, 2025 11:17

Add PullRequestCommenter provider trait

055518c

This allows for providers to comment on pull requests Signed-off-by: Juan Antonio Osorio <[email protected]>

Fixes and hooking up new shared actions

4ff339f

Signed-off-by: Juan Antonio Osorio <[email protected]>

Add rule type info and formatting

7ce24f0

Signed-off-by: Juan Antonio Osorio <[email protected]>

Add todo comment

14bd995

Signed-off-by: Juan Antonio Osorio <[email protected]>

More formatting

e99d9ee

Signed-off-by: Juan Antonio Osorio <[email protected]>

Don't alert on successful rules.

96be386

Signed-off-by: Juan Antonio Osorio <[email protected]>

JAORMX force-pushed the pr-commenter-2 branch from fb5d46d to 96be386 Compare January 10, 2025 09:18

blkt reviewed Jan 10, 2025

View reviewed changes

rdimitrov reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unified PR machinery for actions like pull request comment #5223

Unified PR machinery for actions like pull request comment #5223

JAORMX commented Dec 18, 2024

blkt left a comment

blkt Jan 10, 2025 •

edited

Loading

blkt Jan 10, 2025

blkt Jan 10, 2025

blkt Jan 10, 2025

blkt Jan 10, 2025

blkt Jan 10, 2025

blkt Jan 10, 2025

rdimitrov Jan 10, 2025

rdimitrov Jan 10, 2025

rdimitrov Jan 10, 2025

rdimitrov Jan 10, 2025

		// SPDX-FileCopyrightText: Copyright 2023 The Minder Authors
		// SPDX-License-Identifier: Apache-2.0

Unified PR machinery for actions like pull request comment #5223

Are you sure you want to change the base?

Unified PR machinery for actions like pull request comment #5223

Conversation

JAORMX commented Dec 18, 2024

Summary

Change Type

Testing

Review Checklist:

blkt left a comment

Choose a reason for hiding this comment

blkt Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blkt Jan 10, 2025 •

edited

Loading