diff --git a/internal/notification/client.go b/internal/notification/client.go index b68188d9..cac07b61 100644 --- 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 index 80bf2714..3c52b237 100644 --- 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 index 0895ce7f..007f7216 100644 --- 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 index 46ddc705..99152c78 100644 --- 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) } diff --git a/internal/notification/pull_test.go b/internal/notification/pull_test.go index c5c2435f..0e8c133c 100644 --- a/internal/notification/pull_test.go +++ b/internal/notification/pull_test.go @@ -5,9 +5,72 @@ import ( argocdv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/google/go-cmp/cmp" + "github.com/int128/argocd-commenter/internal/argocd" + "github.com/int128/argocd-commenter/internal/github" v1meta "k8s.io/apimachinery/pkg/apis/meta/v1" ) +func Test_isPullRequestRelatedToEvent(t *testing.T) { + t.Run("source path matches", func(t *testing.T) { + pull := github.PullRequest{ + Files: []string{ + "applications/app1/deployment.yaml", + "applications/app2/deployment.yaml", + }, + } + sourceRevision := argocd.SourceRevision{ + Source: argocdv1alpha1.ApplicationSource{ + Path: "applications/app2", + }, + } + got := isPullRequestRelatedToEvent(pull, sourceRevision, nil) + const want = true + if want != got { + t.Errorf("isPullRequestRelatedToEvent wants %v but was %v", want, got) + } + }) + + t.Run("manifest generate path matches", func(t *testing.T) { + pull := github.PullRequest{ + Files: []string{ + "applications/app1/deployment.yaml", + "applications/app2/deployment.yaml", + }, + } + sourceRevision := argocd.SourceRevision{ + Source: argocdv1alpha1.ApplicationSource{ + Path: "applications/app3", + }, + } + manifestGeneratePaths := []string{"/applications/app1"} + got := isPullRequestRelatedToEvent(pull, sourceRevision, manifestGeneratePaths) + const want = true + if want != got { + t.Errorf("isPullRequestRelatedToEvent wants %v but was %v", want, got) + } + }) + + t.Run("no match", func(t *testing.T) { + pull := github.PullRequest{ + Files: []string{ + "applications/app1/deployment.yaml", + "applications/app2/deployment.yaml", + }, + } + sourceRevision := argocd.SourceRevision{ + Source: argocdv1alpha1.ApplicationSource{ + Path: "applications/app3", + }, + } + manifestGeneratePaths := []string{"/applications/app4"} + got := isPullRequestRelatedToEvent(pull, sourceRevision, manifestGeneratePaths) + const want = false + if want != got { + t.Errorf("isPullRequestRelatedToEvent wants %v but was %v", want, got) + } + }) +} + func Test_getManifestGeneratePaths(t *testing.T) { t.Run("nil annotation", func(t *testing.T) { manifestGeneratePaths := getManifestGeneratePaths(argocdv1alpha1.Application{}) @@ -17,104 +80,220 @@ func Test_getManifestGeneratePaths(t *testing.T) { }) t.Run("empty annotation", func(t *testing.T) { - app := argocdv1alpha1.Application{ - ObjectMeta: v1meta.ObjectMeta{ - Annotations: map[string]string{ - "argocd.argoproj.io/manifest-generate-paths": "", + t.Run("single source", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "", + }, }, - }, - Spec: argocdv1alpha1.ApplicationSpec{ - Source: &argocdv1alpha1.ApplicationSource{ - Path: "/applications/app1", + Spec: argocdv1alpha1.ApplicationSpec{ + Source: &argocdv1alpha1.ApplicationSource{ + Path: "/applications/app1", + }, }, - }, - } - manifestGeneratePaths := getManifestGeneratePaths(app) - if manifestGeneratePaths != nil { - t.Errorf("manifestGeneratePaths wants nil but was %+v", manifestGeneratePaths) - } + } + manifestGeneratePaths := getManifestGeneratePaths(app) + if manifestGeneratePaths != nil { + t.Errorf("manifestGeneratePaths wants nil but was %+v", manifestGeneratePaths) + } + }) + t.Run("multiple sources", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "", + }, + }, + Spec: argocdv1alpha1.ApplicationSpec{ + Sources: argocdv1alpha1.ApplicationSources{ + {Path: "/applications/app1"}, + {Path: "/applications/app2"}, + }, + }, + } + manifestGeneratePaths := getManifestGeneratePaths(app) + if manifestGeneratePaths != nil { + t.Errorf("manifestGeneratePaths wants nil but was %+v", manifestGeneratePaths) + } + }) }) t.Run("absolute path", func(t *testing.T) { - app := argocdv1alpha1.Application{ - ObjectMeta: v1meta.ObjectMeta{ - Annotations: map[string]string{ - "argocd.argoproj.io/manifest-generate-paths": "/components/app1", + t.Run("single source", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "/components/app1", + }, }, - }, - Spec: argocdv1alpha1.ApplicationSpec{ - Source: &argocdv1alpha1.ApplicationSource{ - Path: "/applications/app1", + Spec: argocdv1alpha1.ApplicationSpec{ + Source: &argocdv1alpha1.ApplicationSource{ + Path: "/applications/app1", + }, }, - }, - } - manifestGeneratePaths := getManifestGeneratePaths(app) - want := []string{"components/app1"} - if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { - t.Errorf("want != manifestGeneratePaths:\n%s", diff) - } + } + manifestGeneratePaths := getManifestGeneratePaths(app) + want := []string{"/components/app1"} + if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { + t.Errorf("want != manifestGeneratePaths:\n%s", diff) + } + }) + t.Run("multiple sources", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "/components/app1", + }, + }, + Spec: argocdv1alpha1.ApplicationSpec{ + Sources: argocdv1alpha1.ApplicationSources{ + {Path: "/applications/app1"}, + {Path: "/applications/app2"}, + }, + }, + } + manifestGeneratePaths := getManifestGeneratePaths(app) + want := []string{"/components/app1"} + if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { + t.Errorf("want != manifestGeneratePaths:\n%s", diff) + } + }) }) t.Run("relative path of ascendant", func(t *testing.T) { - app := argocdv1alpha1.Application{ - ObjectMeta: v1meta.ObjectMeta{ - Annotations: map[string]string{ - "argocd.argoproj.io/manifest-generate-paths": "../manifests1", + t.Run("single source", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "../manifests1", + }, }, - }, - Spec: argocdv1alpha1.ApplicationSpec{ - Source: &argocdv1alpha1.ApplicationSource{ - Path: "/applications/app1", + Spec: argocdv1alpha1.ApplicationSpec{ + Source: &argocdv1alpha1.ApplicationSource{ + Path: "/applications/app1", + }, }, - }, - } - manifestGeneratePaths := getManifestGeneratePaths(app) - want := []string{"applications/manifests1"} - if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { - t.Errorf("want != manifestGeneratePaths:\n%s", diff) - } + } + manifestGeneratePaths := getManifestGeneratePaths(app) + want := []string{"/applications/manifests1"} + if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { + t.Errorf("want != manifestGeneratePaths:\n%s", diff) + } + }) + t.Run("multiple sources", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "../manifests1", + }, + }, + Spec: argocdv1alpha1.ApplicationSpec{ + Sources: argocdv1alpha1.ApplicationSources{ + {Path: "/applications/app1"}, + {Path: "/applications/app2"}, + }, + }, + } + manifestGeneratePaths := getManifestGeneratePaths(app) + want := []string{"/applications/manifests1"} + if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { + t.Errorf("want != manifestGeneratePaths:\n%s", diff) + } + }) }) t.Run("relative path of period", func(t *testing.T) { - app := argocdv1alpha1.Application{ - ObjectMeta: v1meta.ObjectMeta{ - Annotations: map[string]string{ - "argocd.argoproj.io/manifest-generate-paths": ".", + t.Run("single source", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": ".", + }, }, - }, - Spec: argocdv1alpha1.ApplicationSpec{ - Source: &argocdv1alpha1.ApplicationSource{ - Path: "/applications/app1", + Spec: argocdv1alpha1.ApplicationSpec{ + Source: &argocdv1alpha1.ApplicationSource{ + Path: "/applications/app1", + }, }, - }, - } - manifestGeneratePaths := getManifestGeneratePaths(app) - want := []string{"applications/app1"} - if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { - t.Errorf("want != manifestGeneratePaths:\n%s", diff) - } + } + manifestGeneratePaths := getManifestGeneratePaths(app) + want := []string{"/applications/app1"} + if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { + t.Errorf("want != manifestGeneratePaths:\n%s", diff) + } + }) + t.Run("multiple sources", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": ".", + }, + }, + Spec: argocdv1alpha1.ApplicationSpec{ + Sources: argocdv1alpha1.ApplicationSources{ + {Path: "/applications/app1"}, + {Path: "/applications/app2"}, + }, + }, + } + manifestGeneratePaths := getManifestGeneratePaths(app) + want := []string{ + "/applications/app1", + "/applications/app2", + } + if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { + t.Errorf("want != manifestGeneratePaths:\n%s", diff) + } + }) }) t.Run("multiple paths", func(t *testing.T) { - app := argocdv1alpha1.Application{ - ObjectMeta: v1meta.ObjectMeta{ - Annotations: map[string]string{ - "argocd.argoproj.io/manifest-generate-paths": ".;../manifests1", + t.Run("single source", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": ".;../manifests1", + }, }, - }, - Spec: argocdv1alpha1.ApplicationSpec{ - Source: &argocdv1alpha1.ApplicationSource{ - Path: "/applications/app1", + Spec: argocdv1alpha1.ApplicationSpec{ + Source: &argocdv1alpha1.ApplicationSource{ + Path: "/applications/app1", + }, }, - }, - } - manifestGeneratePaths := getManifestGeneratePaths(app) - want := []string{ - "applications/app1", - "applications/manifests1", - } - if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { - t.Errorf("want != manifestGeneratePaths:\n%s", diff) - } + } + manifestGeneratePaths := getManifestGeneratePaths(app) + want := []string{ + "/applications/app1", + "/applications/manifests1", + } + if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { + t.Errorf("want != manifestGeneratePaths:\n%s", diff) + } + }) + t.Run("multiple sources", func(t *testing.T) { + app := argocdv1alpha1.Application{ + ObjectMeta: v1meta.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": ".;../manifests1", + }, + }, + Spec: argocdv1alpha1.ApplicationSpec{ + Sources: argocdv1alpha1.ApplicationSources{ + {Path: "/applications/app1"}, + {Path: "/applications/app2"}, + }, + }, + } + manifestGeneratePaths := getManifestGeneratePaths(app) + want := []string{ + "/applications/app1", + "/applications/app2", + "/applications/manifests1", + } + if diff := cmp.Diff(want, manifestGeneratePaths); diff != "" { + t.Errorf("want != manifestGeneratePaths:\n%s", diff) + } + }) }) }