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
19 changes: 17 additions & 2 deletions backend/controllers/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,15 @@ func handlePullRequestEvent(gh utils.GithubClientProvider, payload *github.PullR
log.Printf("strconv.ParseInt error: %v", err)
commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not handle commentId: %v", err))
}
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSGithub, organisationId, impactedJobsMap, impactedProjectsMap, projectsGraph, installationId, branch, prNumber, repoOwner, repoName, repoFullName, commitSha, commentId, diggerYmlStr, 0)

placeholderComment, err := ghService.PublishComment(prNumber, "digger report placehoder")
if err != nil {
log.Printf("strconv.ParseInt error: %v", err)
commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not create placeholder commentId for report: %v", err))
return fmt.Errorf("comment reporter error: %v", err)
}
Comment on lines +525 to +531
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

Fix incorrect error message in placeholder comment creation

The error message incorrectly references "strconv.ParseInt" when the error is from publishing a comment.

Apply this fix:

 	placeholderComment, err := ghService.PublishComment(prNumber, "digger report placehoder")
 	if err != nil {
-		log.Printf("strconv.ParseInt error: %v", err)
+		log.Printf("failed to publish placeholder comment: %v", err)
 		commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not create placeholder commentId for report: %v", err))
 		return fmt.Errorf("comment reporter error: %v", err)
 	}
📝 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
placeholderComment, err := ghService.PublishComment(prNumber, "digger report placehoder")
if err != nil {
log.Printf("strconv.ParseInt error: %v", err)
commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not create placeholder commentId for report: %v", err))
return fmt.Errorf("comment reporter error: %v", err)
}
placeholderComment, err := ghService.PublishComment(prNumber, "digger report placehoder")
if err != nil {
log.Printf("failed to publish placeholder comment: %v", err)
commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not create placeholder commentId for report: %v", err))
return fmt.Errorf("comment reporter error: %v", err)
}


batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSGithub, organisationId, impactedJobsMap, impactedProjectsMap, projectsGraph, installationId, branch, prNumber, repoOwner, repoName, repoFullName, commitSha, commentId, &placeholderComment.Id, diggerYmlStr, 0)
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

Fix redeclaration of batchId variable

The batchId variable is redeclared using := operator when it's already defined. This will cause a compilation error.

-batchId, _, err := utils.ConvertJobsToDiggerJobs(
+batchId, _, err = utils.ConvertJobsToDiggerJobs(

Also applies to: 958-958

🧰 Tools
🪛 golangci-lint (1.62.2)

538-538: no new variables on left side of :=

(typecheck)

Comment on lines +525 to +533
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Extract duplicate placeholder comment logic into a helper function

The placeholder comment creation logic is duplicated between handlePullRequestEvent and handleIssueCommentEvent. This violates the DRY principle.

Extract the logic into a helper function:

func createPlaceholderComment(ghService *dg_github.GithubService, number int, commentReporterManager utils.CommentReporterManager) (*github.IssueComment, error) {
    placeholderComment, err := ghService.PublishComment(number, "digger report placeholder")
    if err != nil {
        log.Printf("failed to publish placeholder comment: %v", err)
        commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not create placeholder commentId for report: %v", err))
        return nil, fmt.Errorf("comment reporter error: %v", err)
    }
    return placeholderComment, nil
}

Also applies to: 942-949

if err != nil {
log.Printf("ConvertJobsToDiggerJobs error: %v", err)
commentReporterManager.UpdateComment(fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err))
Expand Down Expand Up @@ -874,7 +882,14 @@ func handleIssueCommentEvent(gh utils.GithubClientProvider, payload *github.Issu
return fmt.Errorf("comment reporter error: %v", err)
}

batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, "github", orgId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, installationId, *branch, issueNumber, repoOwner, repoName, repoFullName, *commitSha, reporterCommentId, diggerYmlStr, 0)
placeholderComment, err := ghService.PublishComment(issueNumber, "digger report placehoder")
if err != nil {
log.Printf("strconv.ParseInt error: %v", err)
commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not create placeholder commentId for report: %v", err))
return fmt.Errorf("comment reporter error: %v", err)
}
Comment on lines +942 to +947
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Fix incorrect error message and consider refactoring duplicated code

  1. The error message incorrectly references "strconv.ParseInt" when the error is from publishing a comment.
  2. This code block is duplicated from the handlePullRequestEvent function.

First, fix the error message:

 	placeholderComment, err := ghService.PublishComment(issueNumber, "digger report placehoder")
 	if err != nil {
-		log.Printf("strconv.ParseInt error: %v", err)
+		log.Printf("failed to publish placeholder comment: %v", err)
 		commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not create placeholder commentId for report: %v", err))
 		return fmt.Errorf("comment reporter error: %v", err)
 	}

Consider extracting the placeholder comment creation into a shared helper function to avoid code duplication:

func createPlaceholderComment(ghService *dg_github.GithubService, number int, commentReporterManager *utils.CommentReporterManager) (*github.IssueComment, error) {
    placeholderComment, err := ghService.PublishComment(number, "digger report placeholder")
    if err != nil {
        log.Printf("failed to publish placeholder comment: %v", err)
        commentReporterManager.UpdateComment(fmt.Sprintf(":x: could not create placeholder commentId for report: %v", err))
        return nil, fmt.Errorf("comment reporter error: %v", err)
    }
    return placeholderComment, nil
}


batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, "github", orgId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, installationId, *branch, issueNumber, repoOwner, repoName, repoFullName, *commitSha, reporterCommentId, &placeholderComment.Id, diggerYmlStr, 0)
if err != nil {
log.Printf("ConvertJobsToDiggerJobs error: %v", err)
commentReporterManager.UpdateComment(fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err))
Expand Down
2 changes: 2 additions & 0 deletions backend/migrations/20241204194016.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- Modify "digger_batches" table
ALTER TABLE "public"."digger_batches" ADD COLUMN "placeholder_comment_id_for_report" text NULL;
3 changes: 2 additions & 1 deletion backend/migrations/atlas.sum
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
h1:fW4YtFhVfirvwGtHxnGXjFwChYhoj/5pW/XaA6rViHw=
h1:SB1sRi1zY0IoKioJ+Y0FtuIjHQv6XCvcsAJg0AEJNs4=
20231227132525.sql h1:43xn7XC0GoJsCnXIMczGXWis9d504FAWi4F1gViTIcw=
20240115170600.sql h1:IW8fF/8vc40+eWqP/xDK+R4K9jHJ9QBSGO6rN9LtfSA=
20240116123649.sql h1:R1JlUIgxxF6Cyob9HdtMqiKmx/BfnsctTl5rvOqssQw=
Expand Down Expand Up @@ -34,3 +34,4 @@ h1:fW4YtFhVfirvwGtHxnGXjFwChYhoj/5pW/XaA6rViHw=
20241107163722.sql h1:tk28AgXggvpEigTkWMYMxIVDPNdEUFGijaFWBqvlZhA=
20241107172343.sql h1:wtM1+uJZY6NiiDYabuzj/LAANAV7+xyUCL5U23v3e+c=
20241114202249.sql h1:LCRf42VUYB/uugR4gwp+fGPUomT0KhFA3eLjBK9JlZo=
20241204194016.sql h1:RgiJkZlAJSJjNCRD5RHmRr+65O9UDCRwGohQwdIMhGg=
27 changes: 14 additions & 13 deletions backend/models/scheduler.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,20 @@ const DiggerVCSGithub DiggerVCSType = "github"
const DiggerVCSGitlab DiggerVCSType = "gitlab"

type DiggerBatch struct {
ID uuid.UUID `gorm:"primary_key"`
VCS DiggerVCSType
PrNumber int
CommentId *int64
Status orchestrator_scheduler.DiggerBatchStatus
BranchName string
DiggerConfig string
GithubInstallationId int64
GitlabProjectId int
RepoFullName string
RepoOwner string
RepoName string
BatchType orchestrator_scheduler.DiggerCommand
ID uuid.UUID `gorm:"primary_key"`
VCS DiggerVCSType
PrNumber int
CommentId *int64
PlaceholderCommentIdForReport *string
Status orchestrator_scheduler.DiggerBatchStatus
BranchName string
DiggerConfig string
GithubInstallationId int64
GitlabProjectId int
RepoFullName string
RepoOwner string
RepoName string
BatchType orchestrator_scheduler.DiggerCommand
// used for module source grouping comments
SourceDetails []byte
}
Expand Down
29 changes: 15 additions & 14 deletions backend/models/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -617,22 +617,23 @@ func (db *Database) GetDiggerBatch(batchId *uuid.UUID) (*DiggerBatch, error) {
return batch, nil
}

func (db *Database) CreateDiggerBatch(vcsType DiggerVCSType, githubInstallationId int64, repoOwner string, repoName string, repoFullname string, PRNumber int, diggerConfig string, branchName string, batchType scheduler.DiggerCommand, commentId *int64, gitlabProjectId int) (*DiggerBatch, error) {
func (db *Database) CreateDiggerBatch(vcsType DiggerVCSType, githubInstallationId int64, repoOwner string, repoName string, repoFullname string, PRNumber int, diggerConfig string, branchName string, batchType scheduler.DiggerCommand, commentId *int64, placeholderCommentIdForReport *string, gitlabProjectId int) (*DiggerBatch, error) {
uid := uuid.New()
batch := &DiggerBatch{
ID: uid,
VCS: vcsType,
GithubInstallationId: githubInstallationId,
RepoOwner: repoOwner,
RepoName: repoName,
RepoFullName: repoFullname,
PrNumber: PRNumber,
CommentId: commentId,
Status: scheduler.BatchJobCreated,
BranchName: branchName,
DiggerConfig: diggerConfig,
BatchType: batchType,
GitlabProjectId: gitlabProjectId,
ID: uid,
VCS: vcsType,
GithubInstallationId: githubInstallationId,
RepoOwner: repoOwner,
RepoName: repoName,
RepoFullName: repoFullname,
PrNumber: PRNumber,
CommentId: commentId,
PlaceholderCommentIdForReport: placeholderCommentIdForReport,
Status: scheduler.BatchJobCreated,
BranchName: branchName,
DiggerConfig: diggerConfig,
BatchType: batchType,
GitlabProjectId: gitlabProjectId,
}
result := db.GormDB.Save(batch)
if result.Error != nil {
Expand Down
4 changes: 3 additions & 1 deletion backend/services/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,10 @@ func GetSpecFromJob(job models.DiggerJob) (*spec.Spec, error) {
CommentId: strconv.FormatInt(*batch.CommentId, 10),
Job: jobSpec,
Reporter: spec.ReporterSpec{
ReportingStrategy: "comments_per_run",
//ReportingStrategy: "comments_per_run",
ReportingStrategy: "always_same_comment",
ReporterType: "lazy",
ReportCommentId: job.Batch.PlaceholderCommentIdForReport,
},
Lock: spec.LockSpec{
LockType: "noop",
Expand Down
4 changes: 2 additions & 2 deletions backend/utils/graphs.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
)

// ConvertJobsToDiggerJobs jobs is map with project name as a key and a Job as a value
func ConvertJobsToDiggerJobs(jobType scheduler.DiggerCommand, vcsType models.DiggerVCSType, organisationId uint, jobsMap map[string]scheduler.Job, projectMap map[string]configuration.Project, projectsGraph graph.Graph[string, configuration.Project], githubInstallationId int64, branch string, prNumber int, repoOwner string, repoName string, repoFullName string, commitSha string, commentId int64, diggerConfigStr string, gitlabProjectId int) (*uuid.UUID, map[string]*models.DiggerJob, error) {
func ConvertJobsToDiggerJobs(jobType scheduler.DiggerCommand, vcsType models.DiggerVCSType, organisationId uint, jobsMap map[string]scheduler.Job, projectMap map[string]configuration.Project, projectsGraph graph.Graph[string, configuration.Project], githubInstallationId int64, branch string, prNumber int, repoOwner string, repoName string, repoFullName string, commitSha string, commentId int64, placeholderCommentIdForReport *string, diggerConfigStr string, gitlabProjectId int) (*uuid.UUID, map[string]*models.DiggerJob, error) {
result := make(map[string]*models.DiggerJob)
organisation, err := models.DB.GetOrganisationById(organisationId)
if err != nil {
Expand Down Expand Up @@ -43,7 +43,7 @@ func ConvertJobsToDiggerJobs(jobType scheduler.DiggerCommand, vcsType models.Dig

log.Printf("marshalledJobsMap: %v\n", marshalledJobsMap)

batch, err := models.DB.CreateDiggerBatch(vcsType, githubInstallationId, repoOwner, repoName, repoFullName, prNumber, diggerConfigStr, branch, jobType, &commentId, gitlabProjectId)
batch, err := models.DB.CreateDiggerBatch(vcsType, githubInstallationId, repoOwner, repoName, repoFullName, prNumber, diggerConfigStr, branch, jobType, &commentId, placeholderCommentIdForReport, gitlabProjectId)
if err != nil {
return nil, nil, fmt.Errorf("failed to create batch: %v", err)
}
Expand Down
4 changes: 2 additions & 2 deletions ee/backend/controllers/gitlab.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ func handlePullRequestEvent(gitlabProvider utils.GitlabProvider, payload *gitlab
log.Printf("strconv.ParseInt error: %v", err)
utils.InitCommentReporter(glService, prNumber, fmt.Sprintf(":x: could not handle commentId: %v", err))
}
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSGitlab, organisationId, impactedJobsMap, impactedProjectsMap, projectsGraph, 0, branch, prNumber, repoOwner, repoName, repoFullName, commitSha, commentId, diggeryamlStr, projectId)
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSGitlab, organisationId, impactedJobsMap, impactedProjectsMap, projectsGraph, 0, branch, prNumber, repoOwner, repoName, repoFullName, commitSha, commentId, nil, diggeryamlStr, projectId)
if err != nil {
log.Printf("ConvertJobsToDiggerJobs error: %v", err)
utils.InitCommentReporter(glService, prNumber, fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err))
Expand Down Expand Up @@ -524,7 +524,7 @@ func handleIssueCommentEvent(gitlabProvider utils.GitlabProvider, payload *gitla
log.Printf("ParseInt err: %v", err)
return fmt.Errorf("parseint error: %v", err)
}
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSGitlab, organisationId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, 0, branch, issueNumber, repoOwner, repoName, repoFullName, commitSha, commentId64, diggerYmlStr, projectId)
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSGitlab, organisationId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, 0, branch, issueNumber, repoOwner, repoName, repoFullName, commitSha, commentId64, nil, diggerYmlStr, projectId)
if err != nil {
log.Printf("ConvertJobsToDiggerJobs error: %v", err)
utils.InitCommentReporter(glService, issueNumber, fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err))
Expand Down
2 changes: 1 addition & 1 deletion ee/backend/hooks/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ var DriftReconcilliationHook ce_controllers.IssueCommentHook = func(gh utils.Git
utils.InitCommentReporter(ghService, issueNumber, fmt.Sprintf(":x: could not handle commentId: %v", err))
}

batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, "github", orgId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, installationId, defaultBranch, issueNumber, repoOwner, repoName, repoFullName, "", reporterCommentId, diggerYmlStr, 0)
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, "github", orgId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, installationId, defaultBranch, issueNumber, repoOwner, repoName, repoFullName, "", reporterCommentId, nil, diggerYmlStr, 0)
if err != nil {
log.Printf("ConvertJobsToDiggerJobs error: %v", err)
utils.InitCommentReporter(ghService, issueNumber, fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err))
Expand Down
67 changes: 58 additions & 9 deletions libs/comment_utils/reporting/reporting.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,42 @@ type ReportStrategy interface {
Report(ciService ci.PullRequestService, PrNumber int, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) (commentId string, commentUrl string, error error)
}

type AlwaysSameCommentStrategy struct {
Title string
CommentId string
TimeOfRun time.Time
}

func (strategy AlwaysSameCommentStrategy) Report(ciService ci.PullRequestService, PrNumber int, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) (string, string, error) {
comments, err := ciService.GetComments(PrNumber)
if err != nil {
return "", "", fmt.Errorf("error getting comments: %v", err)
}

var commentBody *string
var commentUrl string
for _, comment := range comments {
if comment.Id == strategy.CommentId {
commentBody = comment.Body
commentUrl = comment.Url
}
}

var reportTitle string
if strategy.Title != "" {
reportTitle = strategy.Title + " " + strategy.TimeOfRun.Format("2006-01-02 15:04:05 (MST)")
} else {
reportTitle = "Digger run report at " + strategy.TimeOfRun.Format("2006-01-02 15:04:05 (MST)")
}

err = appendToExistingComment(ciService, PrNumber, *commentBody, report, reportTitle, strategy.CommentId, supportsCollapsibleComment)
if err != nil {
return "", "", fmt.Errorf("error while adding to existing comment: %v", err)
}

return strategy.CommentId, commentUrl, err
}
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

Fix potential nil pointer dereference in AlwaysSameCommentStrategy.Report method

If no comment matching strategy.CommentId is found, commentBody remains nil. Dereferencing it at line 138 will cause a panic. Ensure commentBody is not nil before dereferencing.

 	for _, comment := range comments {
 		if comment.Id == strategy.CommentId {
 			commentBody = comment.Body
 			commentUrl = comment.Url
 		}
 	}

+	if commentBody == nil {
+		return "", "", fmt.Errorf("no comment found with CommentId: %v", strategy.CommentId)
+	}

 	err = appendToExistingComment(ciService, PrNumber, *commentBody, report, reportTitle, strategy.CommentId, supportsCollapsibleComment)
📝 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
func (strategy AlwaysSameCommentStrategy) Report(ciService ci.PullRequestService, PrNumber int, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) (string, string, error) {
comments, err := ciService.GetComments(PrNumber)
if err != nil {
return "", "", fmt.Errorf("error getting comments: %v", err)
}
var commentBody *string
var commentUrl string
for _, comment := range comments {
if comment.Id == strategy.CommentId {
commentBody = comment.Body
commentUrl = comment.Url
}
}
var reportTitle string
if strategy.Title != "" {
reportTitle = strategy.Title + " " + strategy.TimeOfRun.Format("2006-01-02 15:04:05 (MST)")
} else {
reportTitle = "Digger run report at " + strategy.TimeOfRun.Format("2006-01-02 15:04:05 (MST)")
}
err = appendToExistingComment(ciService, PrNumber, *commentBody, report, reportTitle, strategy.CommentId, supportsCollapsibleComment)
if err != nil {
return "", "", fmt.Errorf("error while adding to existing comment: %v", err)
}
return strategy.CommentId, commentUrl, err
}
func (strategy AlwaysSameCommentStrategy) Report(ciService ci.PullRequestService, PrNumber int, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) (string, string, error) {
comments, err := ciService.GetComments(PrNumber)
if err != nil {
return "", "", fmt.Errorf("error getting comments: %v", err)
}
var commentBody *string
var commentUrl string
for _, comment := range comments {
if comment.Id == strategy.CommentId {
commentBody = comment.Body
commentUrl = comment.Url
}
}
var reportTitle string
if strategy.Title != "" {
reportTitle = strategy.Title + " " + strategy.TimeOfRun.Format("2006-01-02 15:04:05 (MST)")
} else {
reportTitle = "Digger run report at " + strategy.TimeOfRun.Format("2006-01-02 15:04:05 (MST)")
}
if commentBody == nil {
return "", "", fmt.Errorf("no comment found with CommentId: %v", strategy.CommentId)
}
err = appendToExistingComment(ciService, PrNumber, *commentBody, report, reportTitle, strategy.CommentId, supportsCollapsibleComment)
if err != nil {
return "", "", fmt.Errorf("error while adding to existing comment: %v", err)
}
return strategy.CommentId, commentUrl, err
}


type CommentPerRunStrategy struct {
Title string
TimeOfRun time.Time
Expand Down Expand Up @@ -156,26 +192,39 @@ func upsertComment(ciService ci.PullRequestService, PrNumber int, report string,
return fmt.Sprintf("%v", comment.Id), comment.Url, nil
}

err := appendToExistingComment(ciService, PrNumber, commentBody, report, reportTitle, commentIdForThisRun, supportsCollapsible)
if err != nil {
return "", "", err
}

return fmt.Sprintf("%v", commentIdForThisRun), commentUrl, nil

}

func appendToExistingComment(ciService ci.PullRequestService, prNumber int, existingCommentBody string, reportToAppend string, reportTitle string, commentId string, supportsCollapsible bool) error {

// strip first and last lines
lines := strings.Split(commentBody, "\n")
lines = lines[1 : len(lines)-1]
commentBody = strings.Join(lines, "\n")
lines := strings.Split(existingCommentBody, "\n")
//if len(lines) > 1 {
// lines = lines[1 : len(lines)-1]
//}
existingCommentBody = strings.Join(lines, "\n")

commentBody = commentBody + "\n\n" + report + "\n"
existingCommentBody = existingCommentBody + "\n\n" + reportToAppend + "\n"

var completeComment string
if !supportsCollapsible {
completeComment = utils.AsComment(reportTitle)(commentBody)
completeComment = utils.AsComment(reportTitle)(existingCommentBody)
} else {
completeComment = utils.AsCollapsibleComment(reportTitle, false)(commentBody)
completeComment = utils.AsCollapsibleComment(reportTitle, false)(existingCommentBody)
}

err := ciService.EditComment(PrNumber, commentIdForThisRun, completeComment)
err := ciService.EditComment(prNumber, commentId, completeComment)

if err != nil {
return "", "", fmt.Errorf("error editing comment: %v", err)
return fmt.Errorf("error editing comment: %v", err)
}
return fmt.Sprintf("%v", commentIdForThisRun), commentUrl, nil
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.

🛠️ Refactor suggestion

Clean up commented code and improve error handling

  1. The commented out code for stripping lines should either be implemented or removed
  2. The error message could be more descriptive by including the comment ID

Apply these improvements:

 func appendToExistingComment(ciService ci.PullRequestService, prNumber int, existingCommentBody string, reportToAppend string, reportTitle string, commentId string, supportsCollapsible bool) error {
 	// strip first and last lines
 	lines := strings.Split(existingCommentBody, "\n")
-	//if len(lines) > 1 {
-	//	lines = lines[1 : len(lines)-1]
-	//}
 	existingCommentBody = strings.Join(lines, "\n")
 	existingCommentBody = existingCommentBody + "\n\n" + reportToAppend + "\n"

 	var completeComment string
 	if !supportsCollapsible {
 		completeComment = utils.AsComment(reportTitle)(existingCommentBody)
 	} else {
 		completeComment = utils.AsCollapsibleComment(reportTitle, false)(existingCommentBody)
 	}

 	err := ciService.EditComment(prNumber, commentId, completeComment)
 	if err != nil {
-		return fmt.Errorf("error editing comment: %v", err)
+		return fmt.Errorf("error editing comment %s: %v", commentId, err)
 	}
 	return nil
 }
📝 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
func appendToExistingComment(ciService ci.PullRequestService, prNumber int, existingCommentBody string, reportToAppend string, reportTitle string, commentId string, supportsCollapsible bool) error {
// strip first and last lines
lines := strings.Split(commentBody, "\n")
lines = lines[1 : len(lines)-1]
commentBody = strings.Join(lines, "\n")
lines := strings.Split(existingCommentBody, "\n")
//if len(lines) > 1 {
// lines = lines[1 : len(lines)-1]
//}
existingCommentBody = strings.Join(lines, "\n")
commentBody = commentBody + "\n\n" + report + "\n"
existingCommentBody = existingCommentBody + "\n\n" + reportToAppend + "\n"
var completeComment string
if !supportsCollapsible {
completeComment = utils.AsComment(reportTitle)(commentBody)
completeComment = utils.AsComment(reportTitle)(existingCommentBody)
} else {
completeComment = utils.AsCollapsibleComment(reportTitle, false)(commentBody)
completeComment = utils.AsCollapsibleComment(reportTitle, false)(existingCommentBody)
}
err := ciService.EditComment(PrNumber, commentIdForThisRun, completeComment)
err := ciService.EditComment(prNumber, commentId, completeComment)
if err != nil {
return "", "", fmt.Errorf("error editing comment: %v", err)
return fmt.Errorf("error editing comment: %v", err)
}
return fmt.Sprintf("%v", commentIdForThisRun), commentUrl, nil
return nil
func appendToExistingComment(ciService ci.PullRequestService, prNumber int, existingCommentBody string, reportToAppend string, reportTitle string, commentId string, supportsCollapsible bool) error {
// strip first and last lines
lines := strings.Split(existingCommentBody, "\n")
existingCommentBody = strings.Join(lines, "\n")
existingCommentBody = existingCommentBody + "\n\n" + reportToAppend + "\n"
var completeComment string
if !supportsCollapsible {
completeComment = utils.AsComment(reportTitle)(existingCommentBody)
} else {
completeComment = utils.AsCollapsibleComment(reportTitle, false)(existingCommentBody)
}
err := ciService.EditComment(prNumber, commentId, completeComment)
if err != nil {
return fmt.Errorf("error editing comment %s: %v", commentId, err)
}
return nil
}

}

type LatestRunCommentStrategy struct {
Expand Down
7 changes: 4 additions & 3 deletions libs/spec/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ import (
)

type ReporterSpec struct {
ReporterType string `json:"reporter_type"`
ReportingStrategy string `json:"reporting_strategy"`
ReportTerraformOutput bool `json:"report_terraform_output"`
ReporterType string `json:"reporter_type"`
ReportingStrategy string `json:"reporting_strategy"`
ReportTerraformOutput bool `json:"report_terraform_output"`
ReportCommentId *string `json:"report_comment_id,omitempty"`
}

type CommentUpdaterSpec struct {
Expand Down
7 changes: 7 additions & 0 deletions libs/spec/providers.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,13 @@ func (r ReporterProvider) GetReporter(title string, reporterSpec ReporterSpec, c
return reporting.LatestRunCommentStrategy{
TimeOfRun: time.Now(),
}
case "always_same_comment":
return reporting.AlwaysSameCommentStrategy{
Title: title,
TimeOfRun: time.Now(),
CommentId: *reporterSpec.ReportCommentId,
}

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 nil check for ReportCommentId

The code dereferences reporterSpec.ReportCommentId without checking if it's nil, which could lead to a panic.

Add a nil check before using the pointer:

 case "always_same_comment":
+    if reporterSpec.ReportCommentId == nil {
+        return reporting.MultipleCommentsStrategy{}
+    }
     return reporting.AlwaysSameCommentStrategy{
         Title:     title,
         TimeOfRun: time.Now(),
         CommentId: *reporterSpec.ReportCommentId,
     }
📝 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
case "always_same_comment":
return reporting.AlwaysSameCommentStrategy{
Title: title,
TimeOfRun: time.Now(),
CommentId: *reporterSpec.ReportCommentId,
}
case "always_same_comment":
if reporterSpec.ReportCommentId == nil {
return reporting.MultipleCommentsStrategy{}
}
return reporting.AlwaysSameCommentStrategy{
Title: title,
TimeOfRun: time.Now(),
CommentId: *reporterSpec.ReportCommentId,
}

default:
return reporting.MultipleCommentsStrategy{}
}
Expand Down
Loading