Skip to content
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

WIP: support single comment summary with orchestrator mode #1850

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Prev Previous commit
Next Next commit
temp commit
  • Loading branch information
motatoes committed Dec 19, 2024
commit 5a33821a6c239d8ea3a3fea88d389f73a6bf3e04
2 changes: 1 addition & 1 deletion cli/cmd/digger/root.go
Original file line number Diff line number Diff line change
@@ -93,7 +93,7 @@ func PreRun(cmd *cobra.Command, args []string) {
//PolicyChecker = policy.NewPolicyChecker(hostName, orgName, token)

if os.Getenv("REPORTING_STRATEGY") == "comments_per_run" || os.Getenv("ACCUMULATE_PLANS") == "true" {
ReportStrategy = &reporting.CommentPerRunStrategy{
ReportStrategy = &reporting.SingleCommentStrategy{
TimeOfRun: time.Now(),
}
} else {
2 changes: 1 addition & 1 deletion ee/cli/cmd/digger/root.go
Original file line number Diff line number Diff line change
@@ -87,7 +87,7 @@ func PreRun(cmd *cobra.Command, args []string) {
BackendApi = NewBackendApi(hostName, token)

if os.Getenv("REPORTING_STRATEGY") == "comments_per_run" || os.Getenv("ACCUMULATE_PLANS") == "true" {
ReportStrategy = &reporting.CommentPerRunStrategy{
ReportStrategy = &reporting.SingleCommentStrategy{
TimeOfRun: time.Now(),
}
} else {
46 changes: 31 additions & 15 deletions libs/comment_utils/reporting/reporting.go
Original file line number Diff line number Diff line change
@@ -17,7 +17,7 @@ type CiReporter struct {
}

func (ciReporter CiReporter) Report(report string, reportFormatting func(report string) string) error {
_, _, err := ciReporter.ReportStrategy.Report(ciReporter.CiService, ciReporter.PrNumber, report, reportFormatting, ciReporter.SupportsMarkdown())
_, _, err := ciReporter.ReportStrategy.Report("", ciReporter.CiService, ciReporter.PrNumber, report, reportFormatting, ciReporter.SupportsMarkdown())
return err
}

@@ -53,16 +53,30 @@ func (reporter StdOutReporter) Suppress() error {
}

type ReportStrategy interface {
Report(ciService ci.PullRequestService, PrNumber int, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) (commentId string, commentUrl string, error error)
Flush() (string, string, error)
Report(projectName string, report string, reportFormatter func(report string) string) (error error)
Flush(ciService ci.PullRequestService, PrNumber int, supportsCollapsibleComment bool) (commentId string, commentUrl string, error error)
}

type CommentPerRunStrategy struct {
Title string
TimeOfRun time.Time
type ReportFormat struct {
ReportTitle string
ReportFormatter func(report string) string
}

func (strategy CommentPerRunStrategy) Report(ciService ci.PullRequestService, PrNumber int, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) (string, string, error) {
type SingleCommentStrategy struct {
Title string
TimeOfRun time.Time
Formatters map[string]ReportFormat
}

func (strategy SingleCommentStrategy) Report(projectName string, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) error {
strategy.Formatters[projectName] = ReportFormat{
ReportTitle: report,
ReportFormatter: reportFormatter,
}
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add thread safety to map access in SingleCommentStrategy

The Formatters map is accessed without synchronization, which could lead to race conditions in concurrent usage.

+type SingleCommentStrategy struct {
+	Title      string
+	TimeOfRun  time.Time
+	Formatters map[string]ReportFormat
+	mu         sync.RWMutex
+}

 func (strategy SingleCommentStrategy) Report(projectName string, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) error {
+	strategy.mu.Lock()
+	defer strategy.mu.Unlock()
 	strategy.Formatters[projectName] = ReportFormat{
 		ReportTitle:     report,
 		ReportFormatter: reportFormatter,
 	}
 	return nil
 }

Committable suggestion skipped: line range outside the PR's diff.


func (strategy SingleCommentStrategy) Flush(ciService ci.PullRequestService, PrNumber int, supportsCollapsibleComment bool) (string, string, error) {
comments, err := ciService.GetComments(PrNumber)
if err != nil {
return "", "", fmt.Errorf("error getting comments: %v", err)
@@ -76,9 +90,6 @@ func (strategy CommentPerRunStrategy) Report(ciService ci.PullRequestService, Pr
}
commentId, commentUrl, err := upsertComment(ciService, PrNumber, report, reportFormatter, comments, reportTitle, supportsCollapsibleComment)
return commentId, commentUrl, err
}

func (s CommentPerRunStrategy) Flush() (string, string, error) {
return "", "", nil
}

@@ -132,13 +143,18 @@ func upsertComment(ciService ci.PullRequestService, PrNumber int, report string,
return fmt.Sprintf("%v", commentIdForThisRun), commentUrl, nil
}

type MultipleCommentsStrategy struct{}
type MultipleCommentsStrategy struct {
Formatters map[string]ReportFormat
}

func (strategy MultipleCommentsStrategy) Report(ciService ci.PullRequestService, PrNumber int, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) (string, string, error) {
_, err := ciService.PublishComment(PrNumber, reportFormatter(report))
return "", "", err
func (strategy MultipleCommentsStrategy) Report(projectName string, report string, reportFormatter func(report string) string) error {
strategy.Formatters[projectName] = ReportFormat{
ReportTitle: report,
ReportFormatter: reportFormatter,
}
return nil
}

func (s MultipleCommentsStrategy) Flush() (string, string, error) {
func (strategy MultipleCommentsStrategy) Flush(ciService ci.PullRequestService, PrNumber int, supportsCollapsibleComment bool) (string, string, error) {
return "", "", nil
}
2 changes: 1 addition & 1 deletion libs/comment_utils/reporting/utils.go
Original file line number Diff line number Diff line change
@@ -28,7 +28,7 @@ func PostInitialSourceComments(ghService ci.PullRequestService, prNumber int, im
reporter := CiReporter{
PrNumber: prNumber,
CiService: ghService,
ReportStrategy: CommentPerRunStrategy{fmt.Sprintf("Report for location: %v", location), time.Now()},
ReportStrategy: SingleCommentStrategy{fmt.Sprintf("Report for location: %v", location), time.Now()},
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider initializing the Formatters map in SingleCommentStrategy

The SingleCommentStrategy struct requires a Formatters map, but it's not being initialized here. This could lead to a nil map panic when Report is called.

-			ReportStrategy: SingleCommentStrategy{fmt.Sprintf("Report for location: %v", location), time.Now()},
+			ReportStrategy: SingleCommentStrategy{
+				Title:      fmt.Sprintf("Report for location: %v", location),
+				TimeOfRun:  time.Now(),
+				Formatters: make(map[string]ReportFormat),
+			},
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ReportStrategy: SingleCommentStrategy{fmt.Sprintf("Report for location: %v", location), time.Now()},
ReportStrategy: SingleCommentStrategy{
Title: fmt.Sprintf("Report for location: %v", location),
TimeOfRun: time.Now(),
Formatters: make(map[string]ReportFormat),
},

}
err := reporter.Report("Comment Reporter", func(report string) string { return "" })
if err != nil {
2 changes: 1 addition & 1 deletion libs/spec/providers.go
Original file line number Diff line number Diff line change
@@ -117,7 +117,7 @@ func (r ReporterProvider) GetReporter(title string, reporterSpec ReporterSpec, c
getStrategy := func(strategy string) reporting.ReportStrategy {
switch reporterSpec.ReportingStrategy {
case "comments_per_run":
return reporting.CommentPerRunStrategy{
return reporting.SingleCommentStrategy{
Title: title,
TimeOfRun: time.Now(),
}
Loading
Oops, something went wrong.