Fix pull request comment for multiple sources (#1022)

* Fix pull request comment for multiple sources * Refactor
int128 · Jan 4, 2024 · 6753940 · 6753940
1 parent 47dacc1
commit 6753940
Show file tree

Hide file tree

Showing 5 changed files with 298 additions and 132 deletions.
diff --git a/internal/notification/client.go b/internal/notification/client.go
@@ -2,12 +2,13 @@ package notification
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	argocdv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	"github.com/go-logr/logr"
+	"github.com/int128/argocd-commenter/internal/argocd"
 	"github.com/int128/argocd-commenter/internal/github"
-	"k8s.io/apimachinery/pkg/util/errors"
 )
 
 type Client interface {
@@ -30,7 +31,7 @@ func IsNotFoundError(err error) bool {
 
 type Comment struct {
 	GitHubRepository github.Repository
-	Revision         string
+	SourceRevision   argocd.SourceRevision
 	Body             string
 }
 
@@ -40,35 +41,29 @@ type client struct {
 
 func (c client) createComment(ctx context.Context, comment Comment, app argocdv1alpha1.Application) error {
 	logger := logr.FromContextOrDiscard(ctx).WithValues(
-		"revision", comment.Revision,
+		"revision", comment.SourceRevision.Revision,
 		"repository", comment.GitHubRepository,
 	)
-	pulls, err := c.ghc.ListPullRequests(ctx, comment.GitHubRepository, comment.Revision)
+	pulls, err := c.ghc.ListPullRequests(ctx, comment.GitHubRepository, comment.SourceRevision.Revision)
 	if err != nil {
-		return fmt.Errorf("unable to list pull requests of revision %s: %w", comment.Revision, err)
+		return fmt.Errorf("unable to list pull requests of revision %s: %w", comment.SourceRevision.Revision, err)
 	}
-	relatedPullNumbers := filterPullRequestsRelatedToEvent(pulls, app)
-	if len(relatedPullNumbers) == 0 {
+	relatedPulls := filterPullRequestsRelatedToEvent(pulls, comment.SourceRevision, app)
+	if len(relatedPulls) == 0 {
 		logger.Info("no pull request related to the revision")
 		return nil
 	}
-	if err := c.createPullRequestComment(ctx, comment.GitHubRepository, relatedPullNumbers, comment.Body); err != nil {
-		return fmt.Errorf("unable to create comment(s) on revision %s: %w", comment.Revision, err)
-	}
-	logger.Info("created comment(s)", "pulls", relatedPullNumbers)
-	return nil
-}
 
-func (c client) createPullRequestComment(ctx context.Context, repository github.Repository, pullNumbers []int, body string) error {
 	var errs []error
-	for _, pullNumber := range pullNumbers {
-		if err := c.ghc.CreateComment(ctx, repository, pullNumber, body); err != nil {
+	for _, pull := range relatedPulls {
+		if err := c.ghc.CreateComment(ctx, comment.GitHubRepository, pull.Number, comment.Body); err != nil {
 			errs = append(errs, err)
 			continue
 		}
+		logger.Info("created a comment", "pullNumber", pull.Number)
 	}
 	if len(errs) > 0 {
-		return errors.NewAggregate(errs)
+		return fmt.Errorf("unable to create comment(s) on revision %s: %w", comment.SourceRevision.Revision, errors.Join(errs...))
 	}
 	return nil
 }

diff --git a/internal/notification/healthcomment.go b/internal/notification/healthcomment.go
@@ -43,7 +43,7 @@ func generateCommentOnHealthChanged(app argocdv1alpha1.Application, argocdURL st
 	}
 	return &Comment{
 		GitHubRepository: *repository,
-		Revision:         sourceRevision.Revision,
+		SourceRevision:   sourceRevision,
 		Body:             body,
 	}
 }

diff --git a/internal/notification/phasecomment.go b/internal/notification/phasecomment.go
@@ -45,7 +45,7 @@ func generateCommentOnPhaseChanged(app argocdv1alpha1.Application, argocdURL str
 	}
 	return &Comment{
 		GitHubRepository: *repository,
-		Revision:         sourceRevision.Revision,
+		SourceRevision:   sourceRevision,
 		Body:             body,
 	}
 }

diff --git a/internal/notification/pull.go b/internal/notification/pull.go
@@ -1,39 +1,36 @@
 package notification
 
 import (
-	"path/filepath"
+	"path"
+	"slices"
 	"strings"
 
 	argocdv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+	"github.com/int128/argocd-commenter/internal/argocd"
 	"github.com/int128/argocd-commenter/internal/github"
 )
 
-func filterPullRequestsRelatedToEvent(pulls []github.PullRequest, app argocdv1alpha1.Application) []int {
-	var numbers []int
+func filterPullRequestsRelatedToEvent(pulls []github.PullRequest, sourceRevision argocd.SourceRevision, app argocdv1alpha1.Application) []github.PullRequest {
+	manifestGeneratePaths := getManifestGeneratePaths(app)
+
+	var relatedPulls []github.PullRequest
 	for _, pull := range pulls {
-		if isPullRequestRelatedToEvent(pull, app) {
-			numbers = append(numbers, pull.Number)
+		if isPullRequestRelatedToEvent(pull, sourceRevision, manifestGeneratePaths) {
+			relatedPulls = append(relatedPulls, pull)
 		}
 	}
-	return numbers
+	return relatedPulls
 }
 
-func isPullRequestRelatedToEvent(pull github.PullRequest, app argocdv1alpha1.Application) bool {
-	if app.Spec.Source == nil {
-		return false
-	}
-
-	// support manifest path annotation
-	// see https://argo-cd.readthedocs.io/en/stable/operator-manual/high_availability/#webhook-and-manifest-paths-annotation
-	// https://github.com/int128/argocd-commenter/pull/656
-	manifestGeneratePaths := getManifestGeneratePaths(app)
-
+func isPullRequestRelatedToEvent(pull github.PullRequest, sourceRevision argocd.SourceRevision, manifestGeneratePaths []string) bool {
+	absSourcePath := path.Join("/", sourceRevision.Source.Path)
 	for _, file := range pull.Files {
-		if strings.HasPrefix(file, app.Spec.Source.Path) {
+		absPullFile := path.Join("/", file)
+		if strings.HasPrefix(absPullFile, absSourcePath) {
 			return true
 		}
-		for _, path := range manifestGeneratePaths {
-			if strings.HasPrefix(file, path) {
+		for _, manifestGeneratePath := range manifestGeneratePaths {
+			if strings.HasPrefix(absPullFile, manifestGeneratePath) {
 				return true
 			}
 		}
@@ -44,30 +41,25 @@ func isPullRequestRelatedToEvent(pull github.PullRequest, app argocdv1alpha1.App
 // getManifestGeneratePaths returns canonical paths of "argocd.argoproj.io/manifest-generate-paths" annotation.
 // It returns nil if the field is nil or empty.
 // https://argo-cd.readthedocs.io/en/stable/operator-manual/high_availability/#webhook-and-manifest-paths-annotation
+// https://github.com/int128/argocd-commenter/pull/656
 func getManifestGeneratePaths(app argocdv1alpha1.Application) []string {
 	if app.Annotations == nil {
 		return nil
 	}
-	if app.Spec.Source == nil {
-		return nil
-	}
-	var canonicalPaths []string
-	annotatedPaths := strings.Split(app.Annotations["argocd.argoproj.io/manifest-generate-paths"], ";")
-	for _, path := range annotatedPaths {
-		if path == "" {
+	var absPaths []string
+	manifestGeneratePaths := strings.Split(app.Annotations["argocd.argoproj.io/manifest-generate-paths"], ";")
+	for _, manifestGeneratePath := range manifestGeneratePaths {
+		if manifestGeneratePath == "" {
 			return nil
 		}
-		// convert to absolute path
-		absolutePath := path
-		if !filepath.IsAbs(path) {
-			absolutePath = filepath.Join(app.Spec.Source.Path, path)
+		if path.IsAbs(manifestGeneratePath) {
+			absPaths = append(absPaths, path.Clean(manifestGeneratePath))
+			continue
 		}
-		// remove leading slash
-		if absolutePath[0:1] == "/" {
-			absolutePath = absolutePath[1:]
+
+		for _, source := range app.Spec.GetSources() {
+			absPaths = append(absPaths, path.Join("/", source.Path, manifestGeneratePath))
 		}
-		// add to list of manifest paths
-		canonicalPaths = append(canonicalPaths, absolutePath)
 	}
-	return canonicalPaths
+	return slices.Compact(absPaths)
 }