Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: support single comment summary with orchestrator mode #1850

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Prev Previous commit
Next Next commit
refactoring all the strategies
  • Loading branch information
motatoes committed Dec 20, 2024
commit eac4651678767e1675327b19f229bffd88a6c913
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{}
9 changes: 4 additions & 5 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,11 +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.SingleCommentStrategy{
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)
}
6 changes: 4 additions & 2 deletions cli/pkg/digger/digger_test.go
Original file line number Diff line number Diff line change
@@ -247,10 +247,11 @@ func TestCorrectCommandExecutionWhenApplying(t *testing.T) {
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{}
@@ -297,10 +298,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{
9 changes: 4 additions & 5 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,11 +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.SingleCommentStrategy{
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) (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
}
8 changes: 4 additions & 4 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) error {
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) {
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
}

8 changes: 4 additions & 4 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) error {
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
}
Loading
Oops, something went wrong.
Loading
Oops, something went wrong.