Skip to content

WIP: support single comment summary with orchestrator mode #1850

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

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
20 changes: 18 additions & 2 deletions backend/controllers/github.go
Original file line number Diff line number Diff line change
@@ -34,9 +34,7 @@ import (
"net/http"
"net/url"
"os"
"path/filepath"
"reflect"
"runtime/debug"
"slices"
"strconv"
"strings"
@@ -430,6 +428,7 @@ func handlePullRequestEvent(gh utils.GithubClientProvider, payload *github.PullR
return fmt.Errorf("error processing event")
}
}

diggerCommand, err := orchestrator_scheduler.GetCommandFromJob(jobsForImpactedProjects[0])
if err != nil {
log.Printf("could not determine digger command from job: %v", jobsForImpactedProjects[0].Commands)
@@ -523,6 +522,15 @@ func handlePullRequestEvent(gh utils.GithubClientProvider, payload *github.PullR
}

batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSGithub, organisationId, impactedJobsMap, impactedProjectsMap, projectsGraph, installationId, branch, prNumber, repoOwner, repoName, repoFullName, commitSha, commentId, diggerYmlStr, 0, aiSummaryCommentId, config.ReportTerraformOutputs)

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))
@@ -931,6 +939,14 @@ func handleIssueCommentEvent(gh utils.GithubClientProvider, payload *github.Issu
}

batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, "github", orgId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, installationId, *branch, issueNumber, repoOwner, repoName, repoFullName, *commitSha, reporterCommentId, diggerYmlStr, 0, aiSummaryCommentId, config.ReportTerraformOutputs)
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))
2 changes: 2 additions & 0 deletions backend/migrations/20250102194016.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:kHWmqYJMe9ZbdcztvM02SVEs5lyw/RFbKzZmqgbmSIk=
h1:DtynCDyaa2VQhu0lgQmvTIZXoG31raLDJNBIyEWKRwg=
20231227132525.sql h1:43xn7XC0GoJsCnXIMczGXWis9d504FAWi4F1gViTIcw=
20240115170600.sql h1:IW8fF/8vc40+eWqP/xDK+R4K9jHJ9QBSGO6rN9LtfSA=
20240116123649.sql h1:R1JlUIgxxF6Cyob9HdtMqiKmx/BfnsctTl5rvOqssQw=
@@ -35,3 +35,4 @@ h1:kHWmqYJMe9ZbdcztvM02SVEs5lyw/RFbKzZmqgbmSIk=
20241107172343.sql h1:E1j+7R5TZlyCKEpyYmH1mJ2zh+y5hVbtQ/PuEMJR7us=
20241114202249.sql h1:P2DhJK8MLe8gSAAz+Y5KNmsvKVw8KfLQPCncynYXEfM=
20241229112312.sql h1:Fr06uwt7LcQoLh6bjGzKB+uy9i8+uk8m6jfi+OBBbP4=
20250102194016.sql h1:Q6nnbPO/zA0PTdZXowv51sL0aUga1b8dA0yfVHgmAZs=
27 changes: 14 additions & 13 deletions backend/models/scheduler.go
Original file line number Diff line number Diff line change
@@ -22,20 +22,21 @@ const DiggerVCSGithub DiggerVCSType = "github"
const DiggerVCSGitlab DiggerVCSType = "gitlab"

type DiggerBatch struct {
ID uuid.UUID `gorm:"primary_key"`
VCS DiggerVCSType
PrNumber int
CommentId *int64
ID uuid.UUID `gorm:"primary_key"`
VCS DiggerVCSType
PrNumber int
CommentId *int64
AiSummaryCommentId string
Status orchestrator_scheduler.DiggerBatchStatus
BranchName string
DiggerConfig string
GithubInstallationId int64
GitlabProjectId int
RepoFullName string
RepoOwner string
RepoName string
BatchType orchestrator_scheduler.DiggerCommand
PlaceholderCommentIdForReport *string
Status orchestrator_scheduler.DiggerBatchStatus
BranchName string
DiggerConfig string
GithubInstallationId int64
GitlabProjectId int
RepoFullName string
RepoOwner string
RepoName string
BatchType orchestrator_scheduler.DiggerCommand
ReportTerraformOutputs bool
// used for module source grouping comments
SourceDetails []byte
3 changes: 2 additions & 1 deletion backend/models/storage.go
Original file line number Diff line number Diff line change
@@ -617,7 +617,7 @@ 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, aiSummaryCommentId string, reportTerraformOutputs bool) (*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, aiSummaryCommentId string, reportTerraformOutputs bool) (*DiggerBatch, error) {
uid := uuid.New()
batch := &DiggerBatch{
ID: uid,
@@ -628,6 +628,7 @@ func (db *Database) CreateDiggerBatch(vcsType DiggerVCSType, githubInstallationI
RepoFullName: repoFullname,
PrNumber: PRNumber,
CommentId: commentId,
PlaceholderCommentIdForReport: placeholderCommentIdForReport,
Status: scheduler.BatchJobCreated,
BranchName: branchName,
DiggerConfig: diggerConfig,
6 changes: 3 additions & 3 deletions backend/services/spec.go
Original file line number Diff line number Diff line change
@@ -116,9 +116,9 @@ func GetSpecFromJob(job models.DiggerJob) (*spec.Spec, error) {
CommentId: strconv.FormatInt(*batch.CommentId, 10),
Job: jobSpec,
Reporter: spec.ReporterSpec{
ReportingStrategy: "comments_per_run",
ReporterType: "lazy",
ReportTerraformOutput: batch.ReportTerraformOutputs,
ReportingStrategy: "comments_per_run",
ReporterType: "basic",
ReportCommentId: job.Batch.PlaceholderCommentIdForReport,
},
Lock: spec.LockSpec{
LockType: "noop",
4 changes: 2 additions & 2 deletions backend/utils/graphs.go
Original file line number Diff line number Diff line change
@@ -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, aiSummaryCommentId string, reportTerraformOutput bool) (*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, aiSummaryCommentId string, reportTerraformOutput bool) (*uuid.UUID, map[string]*models.DiggerJob, error) {
result := make(map[string]*models.DiggerJob)
organisation, err := models.DB.GetOrganisationById(organisationId)
if err != nil {
@@ -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, aiSummaryCommentId, reportTerraformOutput)
batch, err := models.DB.CreateDiggerBatch(vcsType, githubInstallationId, repoOwner, repoName, repoFullName, prNumber, diggerConfigStr, branch, jobType, &commentId, placeholderCommentIdForReport, gitlabProjectId, aiSummaryCommentId, reportTerraformOutput)
if err != nil {
return nil, nil, fmt.Errorf("failed to create batch: %v", err)
}
14 changes: 10 additions & 4 deletions cli/cmd/digger/main_test.go
Original file line number Diff line number Diff line change
@@ -895,8 +895,11 @@ func TestGitHubNewPullRequestContext(t *testing.T) {
impactedProjects, requestedProject, prNumber, err := dggithub.ProcessGitHubEvent(ghEvent, &diggerConfig, &prManager)

reporter := &reporting.CiReporter{
CiService: &prManager,
PrNumber: prNumber,
CiService: &prManager,
PrNumber: prNumber,
IsSupportMarkdown: true,
IsSuppressed: false,
ReportStrategy: reporting.NewMultipleCommentsStrategy(),
Comment on lines +898 to +902
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

Add test coverage for single comment strategy

The PR aims to support single comment summary, but the test initializes MultipleCommentsStrategy. Consider:

  1. Adding test cases that verify the single comment strategy
  2. Validating the reporting behavior with different strategies
 reporter := &reporting.CiReporter{
 	CiService:         &prManager,
 	PrNumber:          prNumber,
 	IsSupportMarkdown: true,
 	IsSuppressed:      false,
-	ReportStrategy:    reporting.NewMultipleCommentsStrategy(),
+	ReportStrategy:    reporting.NewSingleCommentStrategy(),
 }

+func TestGitHubNewPullRequestContextWithSingleComment(t *testing.T) {
+	// Add test case for single comment strategy
+	// Verify the reporting behavior
+}

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

}

event := context.Event.(github.PullRequestEvent)
@@ -923,8 +926,11 @@ func TestGitHubNewCommentContext(t *testing.T) {
planStorage := storage.MockPlanStorage{}
impactedProjects, requestedProject, prNumber, err := dggithub.ProcessGitHubEvent(ghEvent, &diggerConfig, &prManager)
reporter := &reporting.CiReporter{
CiService: &prManager,
PrNumber: prNumber,
CiService: &prManager,
PrNumber: prNumber,
ReportStrategy: reporting.NewMultipleCommentsStrategy(),
IsSuppressed: false,
IsSupportMarkdown: true,
}

policyChecker := policy.MockPolicyChecker{}
13 changes: 4 additions & 9 deletions cli/cmd/digger/root.go
Original file line number Diff line number Diff line change
@@ -14,7 +14,6 @@ import (
"log"
"net/http"
"os"
"time"
)

type RunConfig struct {
@@ -93,15 +92,11 @@ 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{
TimeOfRun: time.Now(),
}
} else if os.Getenv("REPORTING_STRATEGY") == "latest_run_comment" {
ReportStrategy = &reporting.LatestRunCommentStrategy{
TimeOfRun: time.Now(),
}
strategy := reporting.NewSingleCommentStrategy()
ReportStrategy = &strategy
} else {
ReportStrategy = &reporting.MultipleCommentsStrategy{}
strategy := reporting.NewMultipleCommentsStrategy()
ReportStrategy = &strategy
}

var err error
39 changes: 20 additions & 19 deletions cli/pkg/digger/digger.go
Original file line number Diff line number Diff line change
@@ -124,13 +124,14 @@ func RunJobs(jobs []orchestrator.Job, prService ci.PullRequestService, orgServic
}

if allAppliesSuccess == true && reportFinalStatusToBackend == true {
_, jobPrCommentUrl, err := reporter.Flush()
_, jobPrCommentUrls, err := reporter.Flush()
if err != nil {
log.Printf("error while sending job comments %v", err)
return false, false, fmt.Errorf("error while sending job comments %v", err)
}

currentJob := jobs[0]
jobPrCommentUrl := jobPrCommentUrls[0]
Comment on lines +127 to +134
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 bounds checking for jobPrCommentUrls array

The code assumes jobPrCommentUrls has at least one element. This could lead to a panic if the array is empty.

Add a bounds check before accessing the first element:

 _, jobPrCommentUrls, err := reporter.Flush()
 if err != nil {
     log.Printf("error while sending job comments %v", err)
     return false, false, fmt.Errorf("error while sending job comments %v", err)
 }
+if len(jobPrCommentUrls) == 0 {
+    return false, false, fmt.Errorf("no comment URLs returned from reporter")
+}
 currentJob := jobs[0]
 jobPrCommentUrl := jobPrCommentUrls[0]
📝 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
_, jobPrCommentUrls, err := reporter.Flush()
if err != nil {
log.Printf("error while sending job comments %v", err)
return false, false, fmt.Errorf("error while sending job comments %v", err)
}
currentJob := jobs[0]
jobPrCommentUrl := jobPrCommentUrls[0]
_, jobPrCommentUrls, err := reporter.Flush()
if err != nil {
log.Printf("error while sending job comments %v", err)
return false, false, fmt.Errorf("error while sending job comments %v", err)
}
if len(jobPrCommentUrls) == 0 {
return false, false, fmt.Errorf("no comment URLs returned from reporter")
}
currentJob := jobs[0]
jobPrCommentUrl := jobPrCommentUrls[0]

repoNameForBackendReporting := strings.ReplaceAll(currentJob.Namespace, "/", "-")
projectNameForBackendReporting := currentJob.ProjectName
// TODO: handle the apply result summary as well to report it to backend. Possibly reporting changed resources as well
@@ -170,12 +171,12 @@ func RunJobs(jobs []orchestrator.Job, prService ci.PullRequestService, orgServic
func reportPolicyError(projectName string, command string, requestedBy string, reporter reporting.Reporter) string {
msg := fmt.Sprintf("User %s is not allowed to perform action: %s. Check your policies :x:", requestedBy, command)
if reporter.SupportsMarkdown() {
_, _, err := reporter.Report(msg, coreutils.AsCollapsibleComment(fmt.Sprintf("Policy violation for <b>%v - %v</b>", projectName, command), false))
err := reporter.Report(projectName, msg, coreutils.AsCollapsibleComment(fmt.Sprintf("Policy violation for <b>%v - %v</b>", projectName, command), false))
if err != nil {
log.Printf("Error publishing comment: %v", err)
}
} else {
_, _, err := reporter.Report(msg, coreutils.AsComment(fmt.Sprintf("Policy violation for %v - %v", projectName, command)))
err := reporter.Report(projectName, msg, coreutils.AsComment(fmt.Sprintf("Policy violation for %v - %v", projectName, command)))
if err != nil {
log.Printf("Error publishing comment: %v", err)
}
@@ -284,7 +285,7 @@ func run(command string, job orchestrator.Job, policyChecker policy.Checker, org
return nil, msg, fmt.Errorf(msg)
} else if planPerformed {
if isNonEmptyPlan {
reportTerraformPlanOutput(reporter, projectLock.LockId(), plan)
reportTerraformPlanOutput(reporter, job.ProjectName, plan)
planIsAllowed, messages, err := policyChecker.CheckPlanPolicy(SCMrepository, SCMOrganisation, job.ProjectName, job.ProjectDir, planJsonOutput)
if err != nil {
msg := fmt.Sprintf("Failed to validate plan. %v", err)
@@ -311,7 +312,7 @@ func run(command string, job orchestrator.Job, policyChecker policy.Checker, org
preformattedMessaged = append(preformattedMessaged, fmt.Sprintf(" %v", message))
}
planReportMessage = planReportMessage + strings.Join(preformattedMessaged, "<br>")
_, _, err = reporter.Report(planReportMessage, planPolicyFormatter)
err = reporter.Report(job.ProjectName, planReportMessage, planPolicyFormatter)

if err != nil {
log.Printf("Failed to report plan. %v", err)
@@ -320,14 +321,14 @@ func run(command string, job orchestrator.Job, policyChecker policy.Checker, org
log.Printf(msg)
return nil, msg, fmt.Errorf(msg)
} else {
_, _, err := reporter.Report("Terraform plan validation checks succeeded :white_check_mark:", planPolicyFormatter)
err := reporter.Report(job.ProjectName, "Terraform plan validation checks succeeded :white_check_mark:", planPolicyFormatter)
if err != nil {
log.Printf("Failed to report plan. %v", err)
}
reportPlanSummary(reporter, planSummary)
reportPlanSummary(job.ProjectName, reporter, planSummary)
}
} else {
reportEmptyPlanOutput(reporter, projectLock.LockId())
reportEmptyPlanOutput(job.ProjectName, reporter, projectLock.LockId())
}
err := prService.SetStatus(*job.PullRequestNumber, "success", job.ProjectName+"/plan")
if err != nil {
@@ -370,7 +371,7 @@ func run(command string, job orchestrator.Job, policyChecker policy.Checker, org
}
log.Printf("PR status, mergeable: %v, merged: %v and skipMergeCheck %v\n", isMergeable, isMerged, job.SkipMergeCheck)
if !isMergeable && !isMerged && !job.SkipMergeCheck {
comment := reportApplyMergeabilityError(reporter)
comment := reportApplyMergeabilityError(job.ProjectName, reporter)
prService.SetStatus(*job.PullRequestNumber, "failure", job.ProjectName+"/apply")

return nil, comment, fmt.Errorf(comment)
@@ -491,25 +492,25 @@ func run(command string, job orchestrator.Job, policyChecker policy.Checker, org
return &execution.DiggerExecutorResult{}, "", nil
}

func reportApplyMergeabilityError(reporter reporting.Reporter) string {
func reportApplyMergeabilityError(projectName string, reporter reporting.Reporter) string {
comment := "cannot perform Apply since the PR is not currently mergeable"
log.Println(comment)

if reporter.SupportsMarkdown() {
_, _, err := reporter.Report(comment, coreutils.AsCollapsibleComment("Apply error", false))
err := reporter.Report(projectName, comment, coreutils.AsCollapsibleComment("Apply error", false))
if err != nil {
log.Printf("error publishing comment: %v\n", err)
}
} else {
_, _, err := reporter.Report(comment, coreutils.AsComment("Apply error"))
err := reporter.Report(projectName, comment, coreutils.AsComment("Apply error"))
if err != nil {
log.Printf("error publishing comment: %v\n", err)
}
}
return comment
}

func reportTerraformPlanOutput(reporter reporting.Reporter, projectId string, plan string) {
func reportTerraformPlanOutput(reporter reporting.Reporter, projectName string, plan string) {
var formatter func(string) string

if reporter.SupportsMarkdown() {
@@ -518,13 +519,13 @@ func reportTerraformPlanOutput(reporter reporting.Reporter, projectId string, pl
formatter = coreutils.GetTerraformOutputAsComment("Plan output")
}

_, _, err := reporter.Report(plan, formatter)
err := reporter.Report(projectName, plan, formatter)
if err != nil {
log.Printf("Failed to report plan. %v", err)
}
}

func reportPlanSummary(reporter reporting.Reporter, summary string) {
func reportPlanSummary(projectName string, reporter reporting.Reporter, summary string) {
var formatter func(string) string

if reporter.SupportsMarkdown() {
@@ -533,19 +534,19 @@ func reportPlanSummary(reporter reporting.Reporter, summary string) {
formatter = coreutils.AsComment("Plan summary")
}

_, _, err := reporter.Report("\n"+summary, formatter)
err := reporter.Report(projectName, "\n"+summary, formatter)
if err != nil {
log.Printf("Failed to report plan summary. %v", err)
}
}

func reportEmptyPlanOutput(reporter reporting.Reporter, projectId string) {
func reportEmptyPlanOutput(projectName string, reporter reporting.Reporter, projectId string) {
identityFormatter := func(comment string) string {
return comment
}
_, _, err := reporter.Report("→ No changes in terraform output for "+projectId, identityFormatter)
err := reporter.Report(projectName, "→ No changes in terraform output for "+projectId, identityFormatter)
// suppress the comment (if reporter is suppressible)
reporter.Suppress()
reporter.Suppress("")
if err != nil {
log.Printf("Failed to report plan. %v", err)
}
Comment on lines +543 to 552
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

Handle error return value from reporter.Suppress

The error return value from reporter.Suppress is not checked, which could hide important errors.

Add error handling:

 func reportEmptyPlanOutput(projectName string, reporter reporting.Reporter, projectId string) {
     identityFormatter := func(comment string) string {
         return comment
     }
     err := reporter.Report(projectName, "→ No changes in terraform output for "+projectId, identityFormatter)
-    reporter.Suppress("")
+    if err := reporter.Suppress(""); err != nil {
+        log.Printf("Failed to suppress report: %v", err)
+    }
     if err != nil {
         log.Printf("Failed to report plan. %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
func reportEmptyPlanOutput(projectName string, reporter reporting.Reporter, projectId string) {
identityFormatter := func(comment string) string {
return comment
}
_, _, err := reporter.Report("→ No changes in terraform output for "+projectId, identityFormatter)
err := reporter.Report(projectName, "→ No changes in terraform output for "+projectId, identityFormatter)
// suppress the comment (if reporter is suppressible)
reporter.Suppress()
reporter.Suppress("")
if err != nil {
log.Printf("Failed to report plan. %v", err)
}
func reportEmptyPlanOutput(projectName string, reporter reporting.Reporter, projectId string) {
identityFormatter := func(comment string) string {
return comment
}
err := reporter.Report(projectName, "→ No changes in terraform output for "+projectId, identityFormatter)
// suppress the comment (if reporter is suppressible)
if err := reporter.Suppress(""); err != nil {
log.Printf("Failed to suppress report: %v", err)
}
if err != nil {
log.Printf("Failed to report plan. %v", err)
}
🧰 Tools
🪛 golangci-lint (1.62.2)

549-549: Error return value of reporter.Suppress is not checked

(errcheck)

16 changes: 11 additions & 5 deletions cli/pkg/digger/digger_test.go
Original file line number Diff line number Diff line change
@@ -86,7 +86,12 @@ func (m *MockPRManager) GetApprovals(prNumber int) ([]string, error) {

func (m *MockPRManager) PublishComment(prNumber int, comment string) (*ci.Comment, error) {
m.Commands = append(m.Commands, RunInfo{"PublishComment", strconv.Itoa(prNumber) + " " + comment, time.Now()})
return nil, nil
return &ci.Comment{
Id: "",
DiscussionId: "",
Body: nil,
Url: "",
}, nil
}

func (m *MockPRManager) ListIssues() ([]*ci.Issue, error) {
@@ -241,16 +246,16 @@ func (m MockPlanPathProvider) LocalPlanFilePath() string {
}

func TestCorrectCommandExecutionWhenApplying(t *testing.T) {

commandRunner := &MockCommandRunner{}
terraformExecutor := &MockTerraformExecutor{}
prManager := &MockPRManager{}
lock := &MockProjectLock{}
planStorage := &MockPlanStorage{}
strategy := reporting.NewMultipleCommentsStrategy()
reporter := &reporting.CiReporter{
CiService: prManager,
PrNumber: 1,
ReportStrategy: &reporting.MultipleCommentsStrategy{},
ReportStrategy: &strategy,
IsSupportMarkdown: true,
}
planPathProvider := &MockPlanPathProvider{}
@@ -287,7 +292,7 @@ func TestCorrectCommandExecutionWhenApplying(t *testing.T) {

commandStrings := allCommandsInOrderWithParams(terraformExecutor, commandRunner, prManager, lock, planStorage, planPathProvider)

assert.Equal(t, []string{"RetrievePlan plan", "Init ", "Apply ", "PublishComment 1 <details ><summary>Apply output</summary>\n\n```terraform\n\n```\n</details>", "Run echo"}, commandStrings)
assert.Equal(t, []string{"RetrievePlan plan", "Init ", "Apply ", "Run echo"}, commandStrings)
}

func TestCorrectCommandExecutionWhenDestroying(t *testing.T) {
@@ -297,10 +302,11 @@ func TestCorrectCommandExecutionWhenDestroying(t *testing.T) {
prManager := &MockPRManager{}
lock := &MockProjectLock{}
planStorage := &MockPlanStorage{}
strategy := reporting.NewMultipleCommentsStrategy()
reporter := &reporting.CiReporter{
CiService: prManager,
PrNumber: 1,
ReportStrategy: &reporting.MultipleCommentsStrategy{},
ReportStrategy: &strategy,
}
planPathProvider := &MockPlanPathProvider{}
executor := execution.DiggerExecutor{
3 changes: 2 additions & 1 deletion cli/pkg/integration/integration_test.go
Original file line number Diff line number Diff line change
@@ -44,10 +44,11 @@ func getProjectLockForTests() (error, *locking.PullRequestLock) {
repositoryName := "test_dynamodb_lock"
ghToken := "token"
githubPrService, _ := dg_github.GithubServiceProviderBasic{}.NewService(ghToken, repositoryName, repoOwner)
strategy := reporting.NewMultipleCommentsStrategy()
reporter := reporting.CiReporter{
CiService: &githubPrService,
PrNumber: 1,
ReportStrategy: &reporting.MultipleCommentsStrategy{},
ReportStrategy: &strategy,
}

projectLock := &locking.PullRequestLock{
4 changes: 2 additions & 2 deletions ee/backend/controllers/gitlab.go
Original file line number Diff line number Diff line change
@@ -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, "", false)
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSGitlab, organisationId, impactedJobsMap, impactedProjectsMap, projectsGraph, 0, branch, prNumber, repoOwner, repoName, repoFullName, commitSha, commentId, nil, diggeryamlStr, projectId, "", false)
if err != nil {
log.Printf("ConvertJobsToDiggerJobs error: %v", err)
utils.InitCommentReporter(glService, prNumber, fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err))
@@ -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, "", false)
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSGitlab, organisationId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, 0, branch, issueNumber, repoOwner, repoName, repoFullName, commitSha, commentId64, nil, diggerYmlStr, projectId, "", false)
if err != nil {
log.Printf("ConvertJobsToDiggerJobs error: %v", err)
utils.InitCommentReporter(glService, issueNumber, fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err))
2 changes: 1 addition & 1 deletion ee/backend/hooks/github.go
Original file line number Diff line number Diff line change
@@ -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, "", false)
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, "github", orgId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, installationId, defaultBranch, issueNumber, repoOwner, repoName, repoFullName, "", reporterCommentId, nil, diggerYmlStr, 0, "", false)
if err != nil {
log.Printf("ConvertJobsToDiggerJobs error: %v", err)
utils.InitCommentReporter(ghService, issueNumber, fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err))
13 changes: 4 additions & 9 deletions ee/cli/cmd/digger/root.go
Original file line number Diff line number Diff line change
@@ -14,7 +14,6 @@ import (
"log"
"net/http"
"os"
"time"
)

type RunConfig struct {
@@ -87,15 +86,11 @@ 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{
TimeOfRun: time.Now(),
}
} else if os.Getenv("REPORTING_STRATEGY") == "latest_run_comment" {
ReportStrategy = &reporting.LatestRunCommentStrategy{
TimeOfRun: time.Now(),
}
strategy := reporting.NewSingleCommentStrategy()
ReportStrategy = &strategy
} else {
ReportStrategy = &reporting.MultipleCommentsStrategy{}
strategy := reporting.NewMultipleCommentsStrategy()
ReportStrategy = &strategy
}

var err error
6 changes: 3 additions & 3 deletions libs/comment_utils/reporting/core.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package reporting

type Reporter interface {
Report(report string, reportFormatting func(report string) string) (commentId string, commentUrl string, error error)
Flush() (string, string, error)
Suppress() error
Report(projectName string, report string, reportFormatting func(report string) string) (error error)
Flush() ([]string, []string, error)
Suppress(projectName string) error
SupportsMarkdown() bool
}
10 changes: 5 additions & 5 deletions libs/comment_utils/reporting/mock.go
Original file line number Diff line number Diff line change
@@ -4,16 +4,16 @@ type MockReporter struct {
commands []string
}

func (mockReporter *MockReporter) Report(report string, reportFormatting func(report string) string) (string, string, error) {
func (mockReporter *MockReporter) Report(projectName string, report string, reportFormatting func(report string) string) error {
mockReporter.commands = append(mockReporter.commands, "Report")
return "", "", nil
return nil
Comment on lines +7 to +9
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

Enhance mock implementation consistency

The mock implementation has several areas for improvement:

  1. Command tracking is inconsistent across methods
  2. The Suppress method parameter is unnamed
  3. Flush method should track command execution
 func (mockReporter *MockReporter) Report(projectName string, report string, reportFormatting func(report string) string) error {
 	mockReporter.commands = append(mockReporter.commands, "Report")
 	return nil
 }

 func (mockReporter *MockReporter) Flush() ([]string, []string, error) {
+	mockReporter.commands = append(mockReporter.commands, "Flush")
 	return []string{}, []string{}, nil
 }

-func (mockReporter *MockReporter) Suppress(string) error {
+func (mockReporter *MockReporter) Suppress(projectName string) error {
+	mockReporter.commands = append(mockReporter.commands, "Suppress")
 	return nil
 }

Also applies to: 12-13, 16-16

}

func (mockReporter *MockReporter) Flush() (string, string, error) {
return "", "", nil
func (mockReporter *MockReporter) Flush() ([]string, []string, error) {
return []string{}, []string{}, nil
}

func (mockReporter *MockReporter) Suppress() error {
func (mockReporter *MockReporter) Suppress(string) error {
return nil
}

10 changes: 5 additions & 5 deletions libs/comment_utils/reporting/noop.go
Original file line number Diff line number Diff line change
@@ -2,18 +2,18 @@ package reporting

type NoopReporter struct{}

func (reporter NoopReporter) Report(report string, reportFormatting func(report string) string) (string, string, error) {
return "", "", nil
func (reporter NoopReporter) Report(projectName string, report string, reportFormatting func(report string) string) error {
return nil
}

func (reporter NoopReporter) Flush() (string, string, error) {
return "", "", nil
func (reporter NoopReporter) Flush() ([]string, []string, error) {
return []string{}, []string{}, nil
}

func (reporter NoopReporter) SupportsMarkdown() bool {
return false
}

func (reporter NoopReporter) Suppress() error {
func (reporter NoopReporter) Suppress(string) error {
return nil
}
255 changes: 121 additions & 134 deletions libs/comment_utils/reporting/reporting.go
Original file line number Diff line number Diff line change
@@ -5,197 +5,184 @@ import (
"github.com/diggerhq/digger/libs/ci"
"github.com/diggerhq/digger/libs/comment_utils/utils"
"log"
"strings"
"time"
)

type CiReporter struct {
CiService ci.PullRequestService
PrNumber int
IsSupportMarkdown bool
IsSuppressed bool
ReportStrategy ReportStrategy
}

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

func (ciReporter CiReporter) Flush() (string, string, error) {
return "", "", nil
func (ciReporter CiReporter) Flush() ([]string, []string, error) {
commentIds, commentUrls, err := ciReporter.ReportStrategy.Flush(ciReporter.CiService, ciReporter.PrNumber, ciReporter.IsSupportMarkdown)
return commentIds, commentUrls, err
}

func (ciReporter CiReporter) Suppress() error {
return nil
func (ciReporter CiReporter) Suppress(projectName string) error {
return ciReporter.ReportStrategy.Suppress(projectName)
}

func (ciReporter CiReporter) SupportsMarkdown() bool {
return ciReporter.IsSupportMarkdown
}

type CiReporterLazy struct {
CiReporter CiReporter
isSuppressed bool
reports []string
formatters []func(report string) string
}

func NewCiReporterLazy(ciReporter CiReporter) *CiReporterLazy {
return &CiReporterLazy{
CiReporter: ciReporter,
isSuppressed: false,
reports: []string{},
formatters: []func(report string) string{},
}
}

func (lazyReporter *CiReporterLazy) Report(report string, reportFormatting func(report string) string) (string, string, error) {
lazyReporter.reports = append(lazyReporter.reports, report)
lazyReporter.formatters = append(lazyReporter.formatters, reportFormatting)
return "", "", nil
}

func (lazyReporter *CiReporterLazy) Flush() (string, string, error) {
if lazyReporter.isSuppressed {
log.Printf("Reporter is suprresed, ignoring messages ...")
return "", "", nil
}
var commentId, commentUrl string
for i, _ := range lazyReporter.formatters {
var err error
commentId, commentUrl, err = lazyReporter.CiReporter.ReportStrategy.Report(lazyReporter.CiReporter.CiService, lazyReporter.CiReporter.PrNumber, lazyReporter.reports[i], lazyReporter.formatters[i], lazyReporter.SupportsMarkdown())
if err != nil {
log.Printf("failed to report strategy: ")
return "", "", err
}
}
// clear the buffers
lazyReporter.formatters = []func(comment string) string{}
lazyReporter.reports = []string{}
return commentId, commentUrl, nil
}

func (lazyReporter *CiReporterLazy) Suppress() error {
lazyReporter.isSuppressed = true
return nil
}

func (lazyReporter *CiReporterLazy) SupportsMarkdown() bool {
return lazyReporter.CiReporter.IsSupportMarkdown
}

type StdOutReporter struct{}

func (reporter StdOutReporter) Report(report string, reportFormatting func(report string) string) (string, string, error) {
func (reporter StdOutReporter) Report(projectName string, report string, reportFormatting func(report string) string) error {
log.Printf("Info: %v", report)
return "", "", nil
return nil
}

func (reporter StdOutReporter) Flush() (string, string, error) {
return "", "", nil
func (reporter StdOutReporter) Flush() ([]string, []string, error) {
return []string{}, []string{}, nil
}

func (reporter StdOutReporter) SupportsMarkdown() bool {
return false
}

func (reporter StdOutReporter) Suppress() error {
func (reporter StdOutReporter) Suppress(string) error {
return nil
}

type ReportStrategy interface {
Report(ciService ci.PullRequestService, PrNumber int, report string, reportFormatter func(report string) string, supportsCollapsibleComment bool) (commentId string, commentUrl string, error 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)
Suppress(projectName string) error
}

type CommentPerRunStrategy struct {
Title string
TimeOfRun time.Time
type ReportFormat struct {
Report 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) {
comments, err := ciService.GetComments(PrNumber)
if err != nil {
return "", "", fmt.Errorf("error getting comments: %v", err)
}
type SingleCommentStrategy struct {
TimeOfRun time.Time
formatters map[string][]ReportFormat
}

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)")
func NewSingleCommentStrategy() SingleCommentStrategy {
return SingleCommentStrategy{
TimeOfRun: time.Now(),
formatters: make(map[string][]ReportFormat),
}
commentId, commentUrl, err := upsertComment(ciService, PrNumber, report, reportFormatter, comments, reportTitle, supportsCollapsibleComment)
return commentId, commentUrl, err
}

func upsertComment(ciService ci.PullRequestService, PrNumber int, report string, reportFormatter func(report string) string, comments []ci.Comment, reportTitle string, supportsCollapsible bool) (string, string, error) {
report = reportFormatter(report)
commentIdForThisRun := ""
var commentBody string
var commentUrl string
for _, comment := range comments {
if strings.Contains(*comment.Body, reportTitle) {
commentIdForThisRun = comment.Id
commentBody = *comment.Body
commentUrl = comment.Url
break
}
}

func (strategy SingleCommentStrategy) Report(projectName string, report string, reportFormatter func(report string) string) error {
if _, exists := strategy.formatters[projectName]; !exists {
strategy.formatters[projectName] = []ReportFormat{}
}
strategy.formatters[projectName] = append(strategy.formatters[projectName], ReportFormat{
Report: report,
ReportFormatter: reportFormatter,
})

if commentIdForThisRun == "" {
var commentMessage string
if !supportsCollapsible {
commentMessage = utils.AsComment(reportTitle)(report)
} else {
commentMessage = utils.AsCollapsibleComment(reportTitle, false)(report)
return nil
}

func (strategy SingleCommentStrategy) Flush(ciService ci.PullRequestService, PrNumber int, supportsCollapsibleComment bool) ([]string, []string, error) {
var completeComment = ""
for projectName, projectFormatters := range strategy.formatters {
projectTitle := "report for " + projectName + " " + strategy.TimeOfRun.Format("2006-01-02 15:04:05 (MST)")
var projectComment = ""
for _, f := range projectFormatters {
report := f.ReportFormatter(f.Report)
projectComment = projectComment + "\n" + report
}
comment, err := ciService.PublishComment(PrNumber, commentMessage)
if err != nil {
return "", "", fmt.Errorf("error publishing comment: %v", err)
if !supportsCollapsibleComment {
projectComment = utils.AsComment(projectTitle)(projectComment)
} else {
projectComment = utils.AsCollapsibleComment(projectTitle, false)(projectComment)
}
return fmt.Sprintf("%v", comment.Id), comment.Url, nil
completeComment = completeComment + "\n" + projectComment
}

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

commentBody = commentBody + "\n\n" + report + "\n"

var completeComment string
if !supportsCollapsible {
completeComment = utils.AsComment(reportTitle)(commentBody)
} else {
completeComment = utils.AsCollapsibleComment(reportTitle, false)(commentBody)
c, err := ciService.PublishComment(PrNumber, completeComment)
if err != nil {
log.Printf("error while publishing reporter comment: %v", err)
return nil, nil, fmt.Errorf("error while publishing reporter comment: %v", err)
}
return []string{c.Id}, []string{c.Url}, nil
}

err := ciService.EditComment(PrNumber, commentIdForThisRun, completeComment)
func (strategy SingleCommentStrategy) Suppress(projectName string) error {
// TODO: figure out how to suppress a particular project (probably pop it from the formatters map?)
return nil
}

if err != nil {
return "", "", fmt.Errorf("error editing comment: %v", err)
}
return fmt.Sprintf("%v", commentIdForThisRun), commentUrl, nil
type MultipleCommentsStrategy struct {
formatters map[string][]ReportFormat
TimeOfRun time.Time
}

type LatestRunCommentStrategy struct {
TimeOfRun time.Time
func NewMultipleCommentsStrategy() MultipleCommentsStrategy {
return MultipleCommentsStrategy{
TimeOfRun: time.Now(),
formatters: make(map[string][]ReportFormat),
}
}

func (strategy LatestRunCommentStrategy) 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)
func (strategy MultipleCommentsStrategy) Report(projectName string, report string, reportFormatter func(report string) string) error {
if _, exists := strategy.formatters[projectName]; !exists {
strategy.formatters[projectName] = []ReportFormat{}
}
strategy.formatters[projectName] = append(strategy.formatters[projectName], ReportFormat{
Report: report,
ReportFormatter: reportFormatter,
})

reportTitle := "Digger latest run report"
commentId, commentUrl, err := upsertComment(ciService, PrNumber, report, reportFormatter, comments, reportTitle, supportsCollapsibleComment)
return commentId, commentUrl, err
return nil
}

type MultipleCommentsStrategy struct{}
func (strategy MultipleCommentsStrategy) Flush(ciService ci.PullRequestService, PrNumber int, supportsCollapsibleComment bool) ([]string, []string, error) {
hasError := false
var latestError error = nil
commentIds := make([]string, 0)
commentUrls := make([]string, 0)
for projectName, projectFormatters := range strategy.formatters {
projectTitle := "Digger run report for " + projectName + " " + strategy.TimeOfRun.Format("2006-01-02 15:04:05 (MST)")
var projectComment = ""
for _, f := range projectFormatters {
report := f.ReportFormatter(f.Report)
projectComment = projectComment + "\n" + report
}
if !supportsCollapsibleComment {
projectComment = utils.AsComment(projectTitle)(projectComment)
} else {
projectComment = utils.AsCollapsibleComment(projectTitle, false)(projectComment)
}
c, err := ciService.PublishComment(PrNumber, projectComment)
if err != nil {
log.Printf("error while publishing reporter comment: %v", err)
hasError = true
latestError = err
// append placeholders
commentIds = append(commentIds, "0")
commentUrls = append(commentUrls, "")

} else {
commentIds = append(commentIds, c.Id)
commentUrls = append(commentUrls, c.Url)
}
}

if hasError {
log.Printf("could not publish all comments")
return commentIds, commentUrls, latestError
}

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
return commentIds, commentUrls, nil
}

func (strategy MultipleCommentsStrategy) Suppress(projectName string) error {
// TODO: figure out how to suppress a particular project (probably pop it from the formatters map?)
return nil
}
14 changes: 10 additions & 4 deletions libs/comment_utils/reporting/utils.go
Original file line number Diff line number Diff line change
@@ -5,7 +5,6 @@ import (
"github.com/diggerhq/digger/libs/ci"
dg_configuration "github.com/diggerhq/digger/libs/digger_config"
"log"
"time"
)

type SourceDetails struct {
@@ -28,15 +27,22 @@ 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: NewSingleCommentStrategy(),
}
commentId, _, err := reporter.Report("Comment Reporter", func(report string) string { return "" })

err := reporter.Report(location, "Comment Reporter", func(report string) string { return "" })
if err != nil {
log.Printf("Error reporting source module comment: %v", err)
return nil, fmt.Errorf("error reporting source module comment: %v", err)
}

commentIds, _, err := reporter.Flush()
if err != nil {
log.Printf("Error reporting source module comment: %v", err)
return nil, fmt.Errorf("error reporting source module comment: %v", err)
}

sourceDetails = append(sourceDetails, SourceDetails{location, commentId, projects})
sourceDetails = append(sourceDetails, SourceDetails{location, commentIds[0], projects})
}

return sourceDetails, nil
44 changes: 22 additions & 22 deletions libs/execution/execution.go
Original file line number Diff line number Diff line change
@@ -223,7 +223,7 @@ func (d DiggerExecutor) Plan() (*iac_utils.IacSummary, bool, bool, string, strin
if step.Action == "init" {
_, stderr, err := d.TerraformExecutor.Init(step.ExtraArgs, d.StateEnvVars)
if err != nil {
reportError(d.Reporter, stderr)
reportError(d.ProjectName, d.Reporter, stderr)
return nil, false, false, "", "", fmt.Errorf("error running init: %v", err)
}
}
@@ -288,18 +288,18 @@ func (d DiggerExecutor) Plan() (*iac_utils.IacSummary, bool, bool, string, strin
}
}
}
reportAdditionalOutput(d.Reporter, d.projectId())
reportAdditionalOutput(d.Reporter, d.ProjectName)
return planSummary, true, !isEmptyPlan, plan, terraformPlanOutput, nil
}

func reportError(r reporting.Reporter, stderr string) {
func reportError(projectName string, r reporting.Reporter, stderr string) {
if r.SupportsMarkdown() {
_, _, commentErr := r.Report(stderr, utils.AsCollapsibleComment("Error during init.", false))
commentErr := r.Report(projectName, stderr, utils.AsCollapsibleComment("Error during init.", false))
if commentErr != nil {
log.Printf("error publishing comment: %v", commentErr)
}
} else {
_, _, commentErr := r.Report(stderr, utils.AsComment("Error during init."))
commentErr := r.Report(projectName, stderr, utils.AsComment("Error during init."))
if commentErr != nil {
log.Printf("error publishing comment: %v", commentErr)
}
@@ -337,7 +337,7 @@ func (d DiggerExecutor) Apply() (*iac_utils.IacSummary, bool, string, error) {
if step.Action == "init" {
stdout, stderr, err := d.TerraformExecutor.Init(step.ExtraArgs, d.StateEnvVars)
if err != nil {
reportTerraformError(d.Reporter, stderr)
reportTerraformError(d.ProjectName, d.Reporter, stderr)
return nil, false, stdout, fmt.Errorf("error running init: %v", err)
}
}
@@ -346,9 +346,9 @@ func (d DiggerExecutor) Apply() (*iac_utils.IacSummary, bool, string, error) {
stdout, stderr, err := d.TerraformExecutor.Apply(applyArgs, plansFilename, d.CommandEnvVars)
applyOutput = cleanupTerraformApply(true, err, stdout, stderr)

reportTerraformApplyOutput(d.Reporter, d.projectId(), applyOutput)
reportTerraformApplyOutput(d.Reporter, d.ProjectName, applyOutput)
if err != nil {
reportApplyError(d.Reporter, err)
reportApplyError(d.ProjectName, d.Reporter, err)
return nil, false, stdout, fmt.Errorf("error executing apply: %v", err)
}

@@ -370,65 +370,65 @@ func (d DiggerExecutor) Apply() (*iac_utils.IacSummary, bool, string, error) {
}
}
}
reportAdditionalOutput(d.Reporter, d.projectId())
reportAdditionalOutput(d.Reporter, d.ProjectName)
return &summary, true, applyOutput, nil
}

func reportApplyError(r reporting.Reporter, err error) {
func reportApplyError(projectName string, r reporting.Reporter, err error) {
if r.SupportsMarkdown() {
_, _, commentErr := r.Report(err.Error(), utils.AsCollapsibleComment("Error during applying.", false))
commentErr := r.Report(projectName, err.Error(), utils.AsCollapsibleComment("Error during applying.", false))
if commentErr != nil {
log.Printf("error publishing comment: %v", err)
}
} else {
_, _, commentErr := r.Report(err.Error(), utils.AsComment("Error during applying."))
commentErr := r.Report(projectName, err.Error(), utils.AsComment("Error during applying."))
if commentErr != nil {
log.Printf("error publishing comment: %v", err)
}
}
}

func reportTerraformApplyOutput(r reporting.Reporter, projectId string, applyOutput string) {
func reportTerraformApplyOutput(r reporting.Reporter, projectName string, applyOutput string) {
var formatter func(string) string
if r.SupportsMarkdown() {
formatter = utils.GetTerraformOutputAsCollapsibleComment("Apply output", false)
} else {
formatter = utils.GetTerraformOutputAsComment("Apply output")
}

_, _, commentErr := r.Report(applyOutput, formatter)
commentErr := r.Report(projectName, applyOutput, formatter)
if commentErr != nil {
log.Printf("error publishing comment: %v", commentErr)
}
}

func reportTerraformError(r reporting.Reporter, stderr string) {
func reportTerraformError(projectName string, r reporting.Reporter, stderr string) {
if r.SupportsMarkdown() {
_, _, commentErr := r.Report(stderr, utils.GetTerraformOutputAsCollapsibleComment("Error during init.", false))
commentErr := r.Report(projectName, stderr, utils.GetTerraformOutputAsCollapsibleComment("Error during init.", false))
if commentErr != nil {
log.Printf("error publishing comment: %v", commentErr)
}
} else {
_, _, commentErr := r.Report(stderr, utils.GetTerraformOutputAsComment("Error during init."))
commentErr := r.Report(projectName, stderr, utils.GetTerraformOutputAsComment("Error during init."))
if commentErr != nil {
log.Printf("error publishing comment: %v", commentErr)
}
}
}

func reportAdditionalOutput(r reporting.Reporter, projectId string) {
func reportAdditionalOutput(r reporting.Reporter, projectName string) {
var formatter func(string) string
if r.SupportsMarkdown() {
formatter = utils.GetTerraformOutputAsCollapsibleComment("Additional output for <b>"+projectId+"</b>", false)
formatter = utils.GetTerraformOutputAsCollapsibleComment("Additional output for <b>"+projectName+"</b>", false)
} else {
formatter = utils.GetTerraformOutputAsComment("Additional output for " + projectId)
formatter = utils.GetTerraformOutputAsComment("Additional output for " + projectName)
}
diggerOutPath := os.Getenv("DIGGER_OUT")
if _, err := os.Stat(diggerOutPath); err == nil {
output, _ := os.ReadFile(diggerOutPath)
outputStr := string(output)
if len(outputStr) > 0 {
_, _, commentErr := r.Report(outputStr, formatter)
commentErr := r.Report(projectName, outputStr, formatter)
if commentErr != nil {
log.Printf("error publishing comment: %v", commentErr)
}
@@ -459,7 +459,7 @@ func (d DiggerExecutor) Destroy() (bool, error) {
if step.Action == "init" {
_, stderr, err := d.TerraformExecutor.Init(step.ExtraArgs, d.StateEnvVars)
if err != nil {
reportError(d.Reporter, stderr)
reportError(d.ProjectName, d.Reporter, stderr)
return false, fmt.Errorf("error running init: %v", err)
}
}
28 changes: 14 additions & 14 deletions libs/locking/locking.go
Original file line number Diff line number Diff line change
@@ -74,7 +74,7 @@ func (projectLock *PullRequestLock) Lock() (bool, error) {
transactionIdStr := strconv.Itoa(*existingLockTransactionId)
comment := "Project " + projectLock.projectId() + " locked by another PR #" + transactionIdStr + " (failed to acquire lock " + projectLock.ProjectNamespace + "). The locking plan must be applied or discarded before future plans can execute"

reportLockingFailed(projectLock.Reporter, comment)
reportLockingFailed(projectLock.ProjectName, projectLock.Reporter, comment)
return false, fmt.Errorf(comment)
}
}
@@ -87,35 +87,35 @@ func (projectLock *PullRequestLock) Lock() (bool, error) {

if lockAcquired && !isNoOpLock {
comment := "Project " + projectLock.projectId() + " has been locked by PR #" + strconv.Itoa(projectLock.PrNumber)
reportingLockingSuccess(projectLock.Reporter, comment)
reportingLockingSuccess(projectLock.ProjectName, projectLock.Reporter, comment)
log.Println("project " + projectLock.projectId() + " locked successfully. PR # " + strconv.Itoa(projectLock.PrNumber))

}
return lockAcquired, nil
}

func reportingLockingSuccess(r reporting.Reporter, comment string) {
func reportingLockingSuccess(projectName string, r reporting.Reporter, comment string) {
if r.SupportsMarkdown() {
_, _, err := r.Report(comment, utils.AsCollapsibleComment("Locking successful", false))
err := r.Report(projectName, comment, utils.AsCollapsibleComment("Locking successful", false))
if err != nil {
log.Println("failed to publish comment: " + err.Error())
}
} else {
_, _, err := r.Report(comment, utils.AsComment("Locking successful"))
err := r.Report(projectName, comment, utils.AsComment("Locking successful"))
if err != nil {
log.Println("failed to publish comment: " + err.Error())
}
}
}

func reportLockingFailed(r reporting.Reporter, comment string) {
func reportLockingFailed(projectName string, r reporting.Reporter, comment string) {
if r.SupportsMarkdown() {
_, _, err := r.Report(comment, utils.AsCollapsibleComment("Locking failed", false))
err := r.Report(projectName, comment, utils.AsCollapsibleComment("Locking failed", false))
if err != nil {
log.Println("failed to publish comment: " + err.Error())
}
} else {
_, _, err := r.Report(comment, utils.AsComment("Locking failed"))
err := r.Report(projectName, comment, utils.AsComment("Locking failed"))
if err != nil {
log.Println("failed to publish comment: " + err.Error())
}
@@ -146,7 +146,7 @@ func (projectLock *PullRequestLock) verifyNoHangingLocks() (bool, error) {
}
transactionIdStr := strconv.Itoa(*transactionId)
comment := "Project " + projectLock.projectId() + " locked by another PR #" + transactionIdStr + "(failed to acquire lock " + projectLock.ProjectName + "). The locking plan must be applied or discarded before future plans can execute"
reportLockingFailed(projectLock.Reporter, comment)
reportLockingFailed(projectLock.ProjectName, projectLock.Reporter, comment)
return false, fmt.Errorf(comment)
}
return true, nil
@@ -171,7 +171,7 @@ func (projectLock *PullRequestLock) Unlock() (bool, error) {
}
if lockReleased {
comment := "Project unlocked (" + projectLock.projectId() + ")."
reportSuccessfulUnlocking(projectLock.Reporter, comment)
reportSuccessfulUnlocking(projectLock.ProjectName, projectLock.Reporter, comment)

log.Println("Project unlocked")
return true, nil
@@ -181,14 +181,14 @@ func (projectLock *PullRequestLock) Unlock() (bool, error) {
return false, nil
}

func reportSuccessfulUnlocking(r reporting.Reporter, comment string) {
func reportSuccessfulUnlocking(projectName string, r reporting.Reporter, comment string) {
if r.SupportsMarkdown() {
_, _, err := r.Report(comment, utils.AsCollapsibleComment("Unlocking successful", false))
err := r.Report(projectName, comment, utils.AsCollapsibleComment("Unlocking successful", false))
if err != nil {
log.Println("failed to publish comment: " + err.Error())
}
} else {
_, _, err := r.Report(comment, utils.AsComment("Unlocking successful"))
err := r.Report(projectName, comment, utils.AsComment("Unlocking successful"))
if err != nil {
log.Println("failed to publish comment: " + err.Error())
}
@@ -210,7 +210,7 @@ func (projectLock *PullRequestLock) ForceUnlock() error {

if lockReleased {
comment := "Project unlocked (" + projectLock.projectId() + ")."
reportSuccessfulUnlocking(projectLock.Reporter, comment)
reportSuccessfulUnlocking(projectLock.ProjectName, projectLock.Reporter, comment)
log.Println("Project unlocked")
}
return nil
7 changes: 4 additions & 3 deletions libs/spec/models.go
Original file line number Diff line number Diff line change
@@ -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 {
21 changes: 2 additions & 19 deletions libs/spec/providers.go
Original file line number Diff line number Diff line change
@@ -27,7 +27,6 @@ import (
"net/http"
"os"
"strings"
"time"
)

type JobSpecProvider struct{}
@@ -117,16 +116,9 @@ 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{
Title: title,
TimeOfRun: time.Now(),
}
case "latest_run_comment":
return reporting.LatestRunCommentStrategy{
TimeOfRun: time.Now(),
}
return reporting.NewSingleCommentStrategy()
default:
return reporting.MultipleCommentsStrategy{}
return reporting.NewMultipleCommentsStrategy()
}
}

@@ -141,15 +133,6 @@ func (r ReporterProvider) GetReporter(title string, reporterSpec ReporterSpec, c
IsSupportMarkdown: true,
ReportStrategy: strategy,
}, nil
case "lazy":
strategy := getStrategy(reporterSpec.ReportingStrategy)
ciReporter := reporting.CiReporter{
CiService: ciService,
PrNumber: prNumber,
IsSupportMarkdown: true,
ReportStrategy: strategy,
}
return reporting.NewCiReporterLazy(ciReporter), nil
default:
return reporting.NoopReporter{}, nil
}