From 71ad2f2d34187dfa8e92e3475108c68b0e2b82ae Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 17 May 2024 18:02:53 +0100 Subject: [PATCH 1/2] Add gitlab reporter --- .github/spellcheck/wordlist.txt | 4 +- cmd/pint/ci.go | 29 +- cmd/pint/scan.go | 10 - docs/changelog.md | 4 + docs/configuration.md | 42 ++- docs/index.md | 8 +- go.mod | 5 +- go.sum | 7 +- internal/config/config.go | 22 +- internal/config/repository.go | 59 ++++ internal/reporter/bitbucket.go | 30 +- internal/reporter/bitbucket_api.go | 67 ----- internal/reporter/comments.go | 148 +++++++++ internal/reporter/github.go | 2 +- internal/reporter/gitlab.go | 466 +++++++++++++++++++++++++++++ internal/reporter/gitlab_test.go | 134 +++++++++ internal/reporter/reporter.go | 87 ++++++ 17 files changed, 1001 insertions(+), 123 deletions(-) create mode 100644 internal/reporter/comments.go create mode 100644 internal/reporter/gitlab.go create mode 100644 internal/reporter/gitlab_test.go diff --git a/.github/spellcheck/wordlist.txt b/.github/spellcheck/wordlist.txt index 143c8799..d7ea08a2 100644 --- a/.github/spellcheck/wordlist.txt +++ b/.github/spellcheck/wordlist.txt @@ -2,9 +2,9 @@ APIs automaxprocs BitBucket bool -ci changelog Changelog +ci CLI cloudflare Cloudflare @@ -15,6 +15,8 @@ dir durations endraw github +gitlab +GitLab GOGC golang GOMAXPROCS diff --git a/cmd/pint/ci.go b/cmd/pint/ci.go index e3b96eeb..a90fbf8c 100644 --- a/cmd/pint/ci.go +++ b/cmd/pint/ci.go @@ -151,6 +151,28 @@ func actionCI(c *cli.Context) error { reps = append(reps, br) } + if meta.cfg.Repository != nil && meta.cfg.Repository.GitLab != nil { + token, ok := os.LookupEnv("GITLAB_AUTH_TOKEN") + if !ok { + return fmt.Errorf("GITLAB_AUTH_TOKEN env variable is required when reporting to GitLab") + } + + timeout, _ := time.ParseDuration(meta.cfg.Repository.GitLab.Timeout) + var gl reporter.GitLabReporter + if gl, err = reporter.NewGitLabReporter( + version, + currentBranch, + meta.cfg.Repository.GitLab.URI, + timeout, + token, + meta.cfg.Repository.GitLab.Project, + meta.cfg.Repository.GitLab.MaxComments, + ); err != nil { + return err + } + reps = append(reps, gl) + } + meta.cfg.Repository = detectRepository(meta.cfg.Repository) if meta.cfg.Repository != nil && meta.cfg.Repository.GitHub != nil { token, ok := os.LookupEnv("GITHUB_AUTH_TOKEN") @@ -204,8 +226,11 @@ func actionCI(c *cli.Context) error { slog.Info("Problems found", logSeverityCounters(bySeverity)...) } - if err := submitReports(reps, summary); err != nil { - return fmt.Errorf("submitting reports: %w", err) + for _, rep := range reps { + err = rep.Submit(summary) + if err != nil { + return fmt.Errorf("submitting reports: %w", err) + } } if problemsFound { diff --git a/cmd/pint/scan.go b/cmd/pint/scan.go index 523f2d89..1a9b03a8 100644 --- a/cmd/pint/scan.go +++ b/cmd/pint/scan.go @@ -298,13 +298,3 @@ This usually means that it's missing some required fields.`, checkIterationChecksDone.Inc() } } - -func submitReports(reps []reporter.Reporter, summary reporter.Summary) (err error) { - for _, rep := range reps { - err = rep.Submit(summary) - if err != nil { - return err - } - } - return nil -} diff --git a/docs/changelog.md b/docs/changelog.md index d40c9d53..7eada285 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,6 +2,10 @@ ## v0.59.0 +### Added + +- Added GitLab support when running `pint ci`. + ### Changed - Reduced severity of problems reported by [promql/counter](checks/promql/counter.md) due diff --git a/docs/configuration.md b/docs/configuration.md index 8ba879db..5c9cde2d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -140,7 +140,11 @@ ci { Configure supported code hosting repository, used for reporting PR checks from CI back to the repository, to be displayed in the PR UI. -Currently it only supports [BitBucket](https://bitbucket.org/) and [GitHub](https://github.com/). +Currently supported platforms are: + +- [BitBucket](https://bitbucket.org) +- [GitHub](https://github.com) +- [GitLab](https://gitlab.com) **NOTE**: BitBucket integration requires `BITBUCKET_AUTH_TOKEN` environment variable to be set. It should contain a personal access token used to authenticate with the API. @@ -148,6 +152,9 @@ to be set. It should contain a personal access token used to authenticate with t **NOTE**: GitHub integration requires `GITHUB_AUTH_TOKEN` environment variable to be set to a personal access key that can access your repository. +**NOTE**: GitLab integration requires `GITLAB_AUTH_TOKEN` environment variable +to be set to a personal access key that can access your repository. + **NOTE** The pull request number must be known to pint so it can add comments if it detects any problems. If pint is run as part of GitHub actions workflow, then this number will be detected from `GITHUB_REF` environment variable. For other use cases, the `GITHUB_PULL_REQUEST_NUMBER` environment variable must be set @@ -155,6 +162,18 @@ with the pull request number. Syntax: +```js +repository { + bitbucket { ... } + github { ... } + gitlab { ... } +} +``` + +### BitBucket options + +Syntax: + ```js repository { bitbucket { @@ -175,6 +194,8 @@ repository { - `bitbucket:maxComments` - the maximum number of comments pint can create on a single pull request. Default is 50. +### GitHub options + ```js repository { github { @@ -206,6 +227,25 @@ Most GitHub settings can be detected from environment variables that are set ins environment. The only exception is `GITHUB_AUTH_TOKEN` environment variable that must be set manually. +### GitLab options + +```js +repository { + gitlab { + uri = "https://..." + timeout = "1m" + project = "..." + maxComments = 50 + } +} +``` + +- `gitlab:uri` - optional base URI for GitLab API calls when using self hosted setup. + You don't need to set it if you use repositories hosted on [gitlab.com](https://gitlab.com/). +- `gitlab:timeout` - timeout to be used for API requests, defaults to 1 minute. +- `gitlab:project` - ID of the GitLab repository. +- `gitlab:maxComments` - the maximum number of comments pint can create on a single pull request. Default is 50. + ## Prometheus servers Some checks work by querying a running Prometheus instance to verify if diff --git a/docs/index.md b/docs/index.md index 6b7047d0..a3e4779f 100644 --- a/docs/index.md +++ b/docs/index.md @@ -55,9 +55,11 @@ Running `pint ci` doesn't require any configuration but it's recommended to add with `ci` section containing at least the `include` option. This will ensure that pint validates only Prometheus rules and ignores other files. -Results can optionally be reported using -[BitBucket API](https://developer.atlassian.com/server/bitbucket/rest/) -or [GitHub API](https://docs.github.com/en/rest) to generate a report with any found issues. +Results can optionally be reported as comments on a pull request when using one of supported platforms: + +- [BitBucket API](https://developer.atlassian.com/server/bitbucket/rest/) +- [GitHub API](https://docs.github.com/en/rest) +- [GitLab API](https://docs.gitlab.com/ee/api/rest/) Exit code will be one (1) if any issues were detected with severity `Bug` or higher. This permits running `pint` in your CI system whilst at the same you will get detailed reports on your source control system. diff --git a/go.mod b/go.mod index 69d2e961..200def80 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/fatih/color v1.17.0 github.com/gkampitakis/go-snaps v0.5.4 github.com/google/go-cmp v0.6.0 - github.com/google/go-github/v60 v60.0.0 + github.com/google/go-github/v62 v62.0.0 github.com/hashicorp/hcl/v2 v2.20.1 github.com/klauspost/compress v1.17.8 github.com/neilotoole/slogt v1.1.0 @@ -19,6 +19,7 @@ require ( github.com/rogpeppe/go-internal v1.12.0 github.com/stretchr/testify v1.9.0 github.com/urfave/cli/v2 v2.27.2 + github.com/xanzy/go-gitlab v0.105.0 github.com/zclconf/go-cty v1.14.4 go.uber.org/atomic v1.11.0 go.uber.org/automaxprocs v1.5.3 @@ -57,6 +58,8 @@ require ( github.com/google/go-querystring v1.1.0 // indirect github.com/google/uuid v1.6.0 // indirect github.com/grafana/regexp v0.0.0-20221122212121-6b5c0a4cb7fd // indirect + github.com/hashicorp/go-cleanhttp v0.5.2 // indirect + github.com/hashicorp/go-retryablehttp v0.7.4 // indirect github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/jpillora/backoff v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect diff --git a/go.sum b/go.sum index 63d4f2d0..44238df5 100644 --- a/go.sum +++ b/go.sum @@ -212,8 +212,8 @@ github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= -github.com/google/go-github/v60 v60.0.0 h1:oLG98PsLauFvvu4D/YPxq374jhSxFYdzQGNCyONLfn8= -github.com/google/go-github/v60 v60.0.0/go.mod h1:ByhX2dP9XT9o/ll2yXAu2VD8l5eNVg8hD4Cr0S/LmQk= +github.com/google/go-github/v62 v62.0.0 h1:/6mGCaRywZz9MuHyw9gD1CwsbmBX8GWsbFkwMmHdhl4= +github.com/google/go-github/v62 v62.0.0/go.mod h1:EMxeUqGJq2xRu9DYBMwel/mr7kZrzUOfQmmpYrZn2a4= github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= @@ -247,6 +247,7 @@ github.com/hashicorp/errwrap v1.1.0 h1:OxrOeh75EUXMY8TBjag2fzXGZ40LB6IKw45YeGUDY github.com/hashicorp/errwrap v1.1.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ= github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48= +github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrjA0H7acj2lQ= github.com/hashicorp/go-hclog v1.5.0 h1:bI2ocEMgcVlz55Oj1xZNBsVi900c7II+fWDyV9o+13c= github.com/hashicorp/go-hclog v1.5.0/go.mod h1:W4Qnvbt70Wk/zYJryRzDRU/4r0kIg0PVHBcfoyhpF5M= github.com/hashicorp/go-immutable-radix v1.3.1 h1:DKHmCUm2hRBK510BaiZlwvpD40f8bJFeZnpfm2KLowc= @@ -432,6 +433,8 @@ github.com/urfave/cli/v2 v2.27.2 h1:6e0H+AkS+zDckwPCUrZkKX38mRaau4nL2uipkJpbkcI= github.com/urfave/cli/v2 v2.27.2/go.mod h1:g0+79LmHHATl7DAcHO99smiR/T7uGLw84w8Y42x+4eM= github.com/vultr/govultr/v2 v2.17.2 h1:gej/rwr91Puc/tgh+j33p/BLR16UrIPnSr+AIwYWZQs= github.com/vultr/govultr/v2 v2.17.2/go.mod h1:ZFOKGWmgjytfyjeyAdhQlSWwTjh2ig+X49cAp50dzXI= +github.com/xanzy/go-gitlab v0.105.0 h1:3nyLq0ESez0crcaM19o5S//SvezOQguuIHZ3wgX64hM= +github.com/xanzy/go-gitlab v0.105.0/go.mod h1:ETg8tcj4OhrB84UEgeE8dSuV/0h4BBL1uOV/qK0vlyI= github.com/xrash/smetrics v0.0.0-20240312152122-5f08fbb34913 h1:+qGGcbkzsfDQNPPe9UDgpxAWQrhbbBXOYJFQDq/dtJw= github.com/xrash/smetrics v0.0.0-20240312152122-5f08fbb34913/go.mod h1:4aEEwZQutDLsQv2Deui4iYQ6DWTxR14g6m8Wv88+Xqk= github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= diff --git a/internal/config/config.go b/internal/config/config.go index 132d2e88..755b887d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -258,26 +258,8 @@ func Load(path string, failOnMissing bool) (cfg Config, err error) { } } - if cfg.Repository != nil && cfg.Repository.BitBucket != nil { - if cfg.Repository.BitBucket.Timeout == "" { - cfg.Repository.BitBucket.Timeout = time.Minute.String() - } - if cfg.Repository.BitBucket.MaxComments == 0 { - cfg.Repository.BitBucket.MaxComments = 50 - } - if err = cfg.Repository.BitBucket.validate(); err != nil { - return cfg, err - } - } - - if cfg.Repository != nil && cfg.Repository.GitHub != nil { - if cfg.Repository.GitHub.Timeout == "" { - cfg.Repository.GitHub.Timeout = time.Minute.String() - } - if cfg.Repository.GitHub.MaxComments == 0 { - cfg.Repository.GitHub.MaxComments = 50 - } - if err = cfg.Repository.GitHub.validate(); err != nil { + if cfg.Repository != nil { + if err = cfg.Repository.validate(); err != nil { return cfg, err } } diff --git a/internal/config/repository.go b/internal/config/repository.go index e2f06a3a..71ec0d8b 100644 --- a/internal/config/repository.go +++ b/internal/config/repository.go @@ -5,6 +5,7 @@ import ( "net/url" "os" "strings" + "time" ) type BitBucket struct { @@ -84,7 +85,65 @@ func (gh GitHub) validate() error { return nil } +type GitLab struct { + URI string `hcl:"uri,optional"` + Timeout string `hcl:"timeout,optional"` + Project int `hcl:"project"` + MaxComments int `hcl:"maxComments,optional"` +} + +func (gl GitLab) validate() error { + if gl.Project <= 0 { + return fmt.Errorf("project must be set") + } + if gl.MaxComments < 0 { + return fmt.Errorf("maxComments cannot be negative") + } + return nil +} + type Repository struct { BitBucket *BitBucket `hcl:"bitbucket,block" json:"bitbucket,omitempty"` GitHub *GitHub `hcl:"github,block" json:"github,omitempty"` + GitLab *GitLab `hcl:"gitlab,block" json:"gitlab,omitempty"` +} + +func (r *Repository) validate() (err error) { + if r.BitBucket != nil { + if r.BitBucket.Timeout == "" { + r.BitBucket.Timeout = time.Minute.String() + } + if r.BitBucket.MaxComments == 0 { + r.BitBucket.MaxComments = 50 + } + if err = r.BitBucket.validate(); err != nil { + return err + } + } + + if r.GitHub != nil { + if r.GitHub.Timeout == "" { + r.GitHub.Timeout = time.Minute.String() + } + if r.GitHub.MaxComments == 0 { + r.GitHub.MaxComments = 50 + } + if err = r.GitHub.validate(); err != nil { + return err + } + } + + if r.GitLab != nil { + if r.GitLab.Timeout == "" { + r.GitLab.Timeout = time.Minute.String() + } + if r.GitLab.MaxComments == 0 { + r.GitLab.MaxComments = 50 + } + if err = r.GitLab.validate(); err != nil { + return err + } + } + + return nil } diff --git a/internal/reporter/bitbucket.go b/internal/reporter/bitbucket.go index 0e632d1d..e38db1cf 100644 --- a/internal/reporter/bitbucket.go +++ b/internal/reporter/bitbucket.go @@ -37,28 +37,28 @@ type BitBucketReporter struct { gitCmd git.CommandRunner } -func (r BitBucketReporter) Submit(summary Summary) (err error) { +func (bb BitBucketReporter) Submit(summary Summary) (err error) { var headCommit string - if headCommit, err = git.HeadCommit(r.gitCmd); err != nil { + if headCommit, err = git.HeadCommit(bb.gitCmd); err != nil { return fmt.Errorf("failed to get HEAD commit: %w", err) } slog.Info("Got HEAD commit from git", slog.String("commit", headCommit)) - if err = r.api.deleteReport(headCommit); err != nil { + if err = bb.api.deleteReport(headCommit); err != nil { slog.Error("Failed to delete old BitBucket report", slog.Any("err", err)) } - if err = r.api.createReport(summary, headCommit); err != nil { + if err = bb.api.createReport(summary, headCommit); err != nil { return fmt.Errorf("failed to create BitBucket report: %w", err) } var headBranch string - if headBranch, err = git.CurrentBranch(r.gitCmd); err != nil { + if headBranch, err = git.CurrentBranch(bb.gitCmd); err != nil { return fmt.Errorf("failed to get current branch: %w", err) } var pr *bitBucketPR - if pr, err = r.api.findPullRequestForBranch(headBranch, headCommit); err != nil { + if pr, err = bb.api.findPullRequestForBranch(headBranch, headCommit); err != nil { return fmt.Errorf("failed to get open pull requests from BitBucket: %w", err) } @@ -74,31 +74,31 @@ func (r BitBucketReporter) Submit(summary Summary) (err error) { slog.Info("Getting pull request changes from BitBucket") var changes *bitBucketPRChanges - if changes, err = r.api.getPullRequestChanges(pr); err != nil { + if changes, err = bb.api.getPullRequestChanges(pr); err != nil { return fmt.Errorf("failed to get pull request changes from BitBucket: %w", err) } slog.Debug("Got modified files from BitBucket", slog.Any("files", changes.pathModifiedLines)) var existingComments []bitBucketComment - if existingComments, err = r.api.getPullRequestComments(pr); err != nil { + if existingComments, err = bb.api.getPullRequestComments(pr); err != nil { return fmt.Errorf("failed to get pull request comments from BitBucket: %w", err) } slog.Info("Got existing pull request comments from BitBucket", slog.Int("count", len(existingComments))) - pendingComments := r.api.makeComments(summary, changes) + pendingComments := bb.api.makeComments(summary, changes) slog.Info("Generated comments to add to BitBucket", slog.Int("count", len(pendingComments))) - pendingComments = r.api.limitComments(pendingComments) + pendingComments = bb.api.limitComments(pendingComments) slog.Info("Will add comments to BitBucket", slog.Int("count", len(pendingComments)), - slog.Int("limit", r.api.maxComments), + slog.Int("limit", bb.api.maxComments), ) slog.Info("Deleting stale comments from BitBucket") - r.api.pruneComments(pr, existingComments, pendingComments) + bb.api.pruneComments(pr, existingComments, pendingComments) slog.Info("Adding missing comments to BitBucket") - if err = r.api.addComments(pr, existingComments, pendingComments); err != nil { + if err = bb.api.addComments(pr, existingComments, pendingComments); err != nil { return fmt.Errorf("failed to create BitBucket pull request comments: %w", err) } @@ -109,11 +109,11 @@ func (r BitBucketReporter) Submit(summary Summary) (err error) { slog.String("commit", headCommit), ) - if err = r.api.deleteAnnotations(headCommit); err != nil { + if err = bb.api.deleteAnnotations(headCommit); err != nil { return fmt.Errorf("failed to delete existing BitBucket code insight annotations: %w", err) } - if err = r.api.createAnnotations(summary, headCommit); err != nil { + if err = bb.api.createAnnotations(summary, headCommit); err != nil { return fmt.Errorf("failed to create BitBucket code insight annotations: %w", err) } } diff --git a/internal/reporter/bitbucket_api.go b/internal/reporter/bitbucket_api.go index e4c9d239..fe8d7ded 100644 --- a/internal/reporter/bitbucket_api.go +++ b/internal/reporter/bitbucket_api.go @@ -865,70 +865,3 @@ func reportToAnnotation(report Report) BitBucketAnnotation { Link: fmt.Sprintf("https://cloudflare.github.io/pint/checks/%s.html", report.Problem.Reporter), } } - -func dedupReports(src []Report) (dst [][]Report) { - for _, report := range src { - index := -1 - for i, d := range dst { - if d[0].Problem.Severity != report.Problem.Severity { - continue - } - if d[0].Problem.Reporter != report.Problem.Reporter { - continue - } - if d[0].Path.SymlinkTarget != report.Path.SymlinkTarget { - continue - } - if d[0].Problem.Lines.First != report.Problem.Lines.First { - continue - } - if d[0].Problem.Lines.Last != report.Problem.Lines.Last { - continue - } - if d[0].Problem.Anchor != report.Problem.Anchor { - continue - } - index = i - break - } - if index < 0 { - dst = append(dst, []Report{report}) - continue - } - // Skip this report if we have exact same message already - if dst[index][0].Problem.Text == report.Problem.Text && dst[index][0].Problem.Details == report.Problem.Details { - continue - } - dst[index] = append(dst[index], report) - } - return dst -} - -func identicalDetails(src []Report) bool { - if len(src) <= 1 { - return false - } - var details string - for _, report := range src { - if details == "" { - details = report.Problem.Details - continue - } - if details != report.Problem.Details { - return false - } - } - return true -} - -func problemIcon(s checks.Severity) string { - // nolint:exhaustive - switch s { - case checks.Warning: - return ":warning:" - case checks.Information: - return ":information_source:" - default: - return ":stop_sign:" - } -} diff --git a/internal/reporter/comments.go b/internal/reporter/comments.go new file mode 100644 index 00000000..ca0e1529 --- /dev/null +++ b/internal/reporter/comments.go @@ -0,0 +1,148 @@ +package reporter + +import ( + "context" + "strings" + + "github.com/cloudflare/pint/internal/checks" +) + +type PendingCommentV2 struct { + path string + text string + line int + anchor checks.Anchor +} + +type ExistingCommentV2 struct { + meta any + path string + text string + line int +} + +type Commenter interface { + Describe() string + Destinations(context.Context) ([]any, error) + Summary(context.Context, any, Summary) error + List(context.Context, any) ([]ExistingCommentV2, error) + Create(context.Context, any, PendingCommentV2) error + Delete(context.Context, any, ExistingCommentV2) error + CanCreate(int) bool + IsEqual(ExistingCommentV2, PendingCommentV2) bool +} + +func makeComments(summary Summary) (comments []PendingCommentV2) { + var buf strings.Builder + for _, reports := range dedupReports(summary.reports) { + mergeDetails := identicalDetails(reports) + + buf.Reset() + + buf.WriteString(problemIcon(reports[0].Problem.Severity)) + buf.WriteString(" **") + buf.WriteString(reports[0].Problem.Severity.String()) + buf.WriteString("** reported by [pint](https://cloudflare.github.io/pint/) **") + buf.WriteString(reports[0].Problem.Reporter) + buf.WriteString("** check.\n\n") + for _, report := range reports { + buf.WriteString("------\n\n") + buf.WriteString(report.Problem.Text) + buf.WriteString("\n\n") + if !mergeDetails && report.Problem.Details != "" { + buf.WriteString(report.Problem.Details) + buf.WriteString("\n\n") + } + if report.Path.SymlinkTarget != report.Path.Name { + buf.WriteString(":leftwards_arrow_with_hook: This problem was detected on a symlinked file ") + buf.WriteRune('`') + buf.WriteString(report.Path.Name) + buf.WriteString("`.\n\n") + } + } + if mergeDetails && reports[0].Problem.Details != "" { + buf.WriteString("------\n\n") + buf.WriteString(reports[0].Problem.Details) + buf.WriteString("\n\n") + } + buf.WriteString("------\n\n") + buf.WriteString(":information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/") + buf.WriteString(reports[0].Problem.Reporter) + buf.WriteString(".html).\n") + + comments = append(comments, PendingCommentV2{ + anchor: reports[0].Problem.Anchor, + path: reports[0].Path.SymlinkTarget, + line: reports[0].Problem.Lines.Last, + text: buf.String(), + }) + } + return comments +} + +func dedupReports(src []Report) (dst [][]Report) { + for _, report := range src { + index := -1 + for i, d := range dst { + if d[0].Problem.Severity != report.Problem.Severity { + continue + } + if d[0].Problem.Reporter != report.Problem.Reporter { + continue + } + if d[0].Path.SymlinkTarget != report.Path.SymlinkTarget { + continue + } + if d[0].Problem.Lines.First != report.Problem.Lines.First { + continue + } + if d[0].Problem.Lines.Last != report.Problem.Lines.Last { + continue + } + if d[0].Problem.Anchor != report.Problem.Anchor { + continue + } + index = i + break + } + if index < 0 { + dst = append(dst, []Report{report}) + continue + } + // Skip this report if we have exact same message already + if dst[index][0].Problem.Text == report.Problem.Text && dst[index][0].Problem.Details == report.Problem.Details { + continue + } + dst[index] = append(dst[index], report) + } + return dst +} + +func identicalDetails(src []Report) bool { + if len(src) <= 1 { + return false + } + var details string + for _, report := range src { + if details == "" { + details = report.Problem.Details + continue + } + if details != report.Problem.Details { + return false + } + } + return true +} + +func problemIcon(s checks.Severity) string { + // nolint:exhaustive + switch s { + case checks.Warning: + return ":warning:" + case checks.Information: + return ":information_source:" + default: + return ":stop_sign:" + } +} diff --git a/internal/reporter/github.go b/internal/reporter/github.go index 6154e448..25b4e9cf 100644 --- a/internal/reporter/github.go +++ b/internal/reporter/github.go @@ -9,7 +9,7 @@ import ( "strings" "time" - "github.com/google/go-github/v60/github" + "github.com/google/go-github/v62/github" "golang.org/x/oauth2" "github.com/cloudflare/pint/internal/checks" diff --git a/internal/reporter/gitlab.go b/internal/reporter/gitlab.go new file mode 100644 index 00000000..c2c39e3a --- /dev/null +++ b/internal/reporter/gitlab.go @@ -0,0 +1,466 @@ +package reporter + +import ( + "bufio" + "context" + "errors" + "fmt" + "log/slog" + "regexp" + "strconv" + "strings" + "time" + + "github.com/xanzy/go-gitlab" + + "github.com/cloudflare/pint/internal/checks" + "github.com/cloudflare/pint/internal/output" +) + +type gitlabLogger struct{} + +func (l gitlabLogger) Error(msg string, keysAndValues ...any) { + slog.Error(msg, keysAndValuesToAttrs(keysAndValues...)...) +} + +func (l gitlabLogger) Info(msg string, keysAndValues ...any) { + slog.Info(msg, keysAndValuesToAttrs(keysAndValues...)...) +} + +func (l gitlabLogger) Debug(msg string, keysAndValues ...any) { + slog.Debug(msg, keysAndValuesToAttrs(keysAndValues...)...) +} + +func (l gitlabLogger) Warn(msg string, keysAndValues ...any) { + slog.Warn(msg, keysAndValuesToAttrs(keysAndValues...)...) +} + +func keysAndValuesToAttrs(keysAndValues ...any) (attrs []any) { + attrs = append(attrs, slog.String("reporter", "GitLab")) + var key string + var ok bool + for i, elem := range keysAndValues { + switch { + case i%2 == 0: + key, ok = elem.(string) + case ok && key != "": + attrs = append(attrs, slog.Any(key, elem)) + } + } + return attrs +} + +type gitlabMR struct { + version *gitlab.MergeRequestDiffVersion + diffs []*gitlab.MergeRequestDiff + userID int + mrID int +} + +type gitlabComment struct { + discussionID string + baseSHA string + headSHA string + startSHA string + noteID int +} + +type GitLabReporter struct { + client *gitlab.Client + version string + branch string + timeout time.Duration + project int + maxComments int +} + +func NewGitLabReporter(version, branch, uri string, timeout time.Duration, token string, project, maxComments int) (_ GitLabReporter, err error) { + slog.Info( + "Will report problems to GitLab", + slog.String("uri", uri), + slog.String("timeout", output.HumanizeDuration(timeout)), + slog.String("branch", branch), + slog.Int("project", project), + slog.Int("maxComments", maxComments), + ) + gl := GitLabReporter{ + timeout: timeout, + version: version, + branch: branch, + project: project, + maxComments: maxComments, + } + + options := []gitlab.ClientOptionFunc{ + gitlab.WithCustomLeveledLogger(gitlabLogger{}), + } + if uri != "" { + options = append(options, gitlab.WithBaseURL(uri+"/api/v4")) + } + + gl.client, err = gitlab.NewClient(token, options...) + if err != nil { + return gl, fmt.Errorf("failed to create a new GitLab client: %w", err) + } + return gl, nil +} + +func (gl GitLabReporter) Describe() string { + return "GitLab" +} + +func (gl GitLabReporter) Destinations(ctx context.Context) ([]any, error) { + userID, err := gl.getUserID(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GitLab user ID: %w", err) + } + + ids, err := gl.getMRs(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GitLab merge request details: %w", err) + } + + dsts := make([]any, 0, len(ids)) + for _, id := range ids { + slog.Info("Found open GitLab merge request", slog.String("branch", gl.branch), slog.Int("id", id)) + dst := gitlabMR{ + userID: userID, + mrID: id, + } + if dst.version, err = gl.getVersions(ctx, id); err != nil { + return nil, fmt.Errorf("failed to get GitLab merge request versions: %w", err) + } + if dst.diffs, err = gl.getDiffs(ctx, id); err != nil { + return nil, fmt.Errorf("failed to get GitLab merge request changes: %w", err) + } + dsts = append(dsts, dst) + } + return dsts, nil +} + +func (gl GitLabReporter) Summary(ctx context.Context, dst any, s Summary) error { + mr := dst.(gitlabMR) + if gl.maxComments > 0 && len(s.reports) > gl.maxComments { + if err := gl.tooManyComments(ctx, mr.mrID, len(s.reports)); err != nil { + return err + } + } + return nil +} + +func (gl GitLabReporter) List(ctx context.Context, dst any) ([]ExistingCommentV2, error) { + mr := dst.(gitlabMR) + slog.Debug("Getting the list of merge request discussions", slog.Int("mr", mr.mrID)) + reqCtx, cancel := context.WithTimeout(ctx, gl.timeout) + defer cancel() + discs, _, err := gl.client.Discussions.ListMergeRequestDiscussions(gl.project, mr.mrID, nil, gitlab.WithContext(reqCtx)) + if err != nil { + return nil, err + } + comments := make([]ExistingCommentV2, 0, len(discs)) + for _, disc := range discs { + var c ExistingCommentV2 + for _, note := range disc.Notes { + if note.System { + goto NEXT + } + if note.Author.ID != mr.userID { + goto NEXT + } + if note.Position == nil { + goto NEXT + } + if note.Position.NewPath != "" { + c.path = note.Position.NewPath + } else { + c.path = note.Position.OldPath + } + if note.Position.NewLine > 0 { + c.line = note.Position.NewLine + } else { + c.line = note.Position.OldLine + } + c.text = note.Body + c.meta = gitlabComment{ + discussionID: disc.ID, + noteID: note.ID, + baseSHA: note.Position.BaseSHA, + headSHA: note.Position.HeadSHA, + startSHA: note.Position.StartSHA, + } + break + } + comments = append(comments, c) + NEXT: + } + return comments, nil +} + +func (gl GitLabReporter) Create(ctx context.Context, dst any, comment PendingCommentV2) error { + mr := dst.(gitlabMR) + opt := reportToGitLabDiscussion(comment, mr.diffs, mr.version) + if opt == nil { + return nil + } + slog.Debug("Creating a new merge request discussion", loggifyDiscussion(opt)...) + reqCtx, cancel := context.WithTimeout(ctx, gl.timeout) + defer cancel() + _, _, err := gl.client.Discussions.CreateMergeRequestDiscussion(gl.project, mr.mrID, opt, gitlab.WithContext(reqCtx)) + return err +} + +func (gl GitLabReporter) Delete(ctx context.Context, dst any, comment ExistingCommentV2) error { + mr := dst.(gitlabMR) + c := comment.meta.(gitlabComment) + slog.Debug("Deleting stale merge request discussion note", + slog.String("discussion", c.discussionID), + slog.Int("note", c.noteID), + ) + reqCtx, cancel := context.WithTimeout(ctx, gl.timeout) + defer cancel() + + _, err := gl.client.Discussions.DeleteMergeRequestDiscussionNote( + gl.project, + mr.mrID, + c.discussionID, + c.noteID, + gitlab.WithContext(reqCtx), + ) + return err +} + +func (gl GitLabReporter) IsEqual(existing ExistingCommentV2, pending PendingCommentV2) bool { + if existing.path != pending.path { + return false + } + if existing.line != pending.line { + return false + } + return strings.Trim(existing.text, "\n") == strings.Trim(pending.text, "\n") +} + +func (gl GitLabReporter) CanCreate(done int) bool { + return done < gl.maxComments +} + +func (gl *GitLabReporter) getUserID(ctx context.Context) (int, error) { + slog.Debug("Getting current GitLab user details") + ctx, cancel := context.WithTimeout(ctx, gl.timeout) + defer cancel() + user, _, err := gl.client.Users.CurrentUser(gitlab.WithContext(ctx)) + if err != nil { + return 0, err + } + return user.ID, nil +} + +func (gl *GitLabReporter) getMRs(ctx context.Context) (ids []int, err error) { + slog.Debug("Finding merge requests for current branch", slog.String("branch", gl.branch)) + mrs, _, err := getGitLabPaginated(func(pageNum int) ([]*gitlab.MergeRequest, *gitlab.Response, error) { + reqCtx, cancel := context.WithTimeout(ctx, gl.timeout) + defer cancel() + return gl.client.MergeRequests.ListProjectMergeRequests(gl.project, &gitlab.ListProjectMergeRequestsOptions{ + State: gitlab.Ptr("opened"), + SourceBranch: gitlab.Ptr(gl.branch), + ListOptions: gitlab.ListOptions{Page: pageNum}, + }, gitlab.WithContext(reqCtx)) + }) + if err != nil { + return nil, err + } + for _, mr := range mrs { + ids = append(ids, mr.IID) + } + return ids, nil +} + +func (gl *GitLabReporter) getDiffs(ctx context.Context, mrNum int) ([]*gitlab.MergeRequestDiff, error) { + slog.Debug("Getting the list of merge request diffs", slog.Int("mr", mrNum)) + diffs, _, err := getGitLabPaginated(func(pageNum int) ([]*gitlab.MergeRequestDiff, *gitlab.Response, error) { + reqCtx, cancel := context.WithTimeout(ctx, gl.timeout) + defer cancel() + return gl.client.MergeRequests.ListMergeRequestDiffs(gl.project, mrNum, &gitlab.ListMergeRequestDiffsOptions{ + ListOptions: gitlab.ListOptions{Page: pageNum}, + }, gitlab.WithContext(reqCtx)) + }) + return diffs, err +} + +func (gl *GitLabReporter) getVersions(ctx context.Context, mrNum int) (*gitlab.MergeRequestDiffVersion, error) { + slog.Debug("Getting the list of merge request versions", slog.Int("mr", mrNum)) + vers, _, err := getGitLabPaginated(func(pageNum int) ([]*gitlab.MergeRequestDiffVersion, *gitlab.Response, error) { + reqCtx, cancel := context.WithTimeout(ctx, gl.timeout) + defer cancel() + return gl.client.MergeRequests.GetMergeRequestDiffVersions(gl.project, mrNum, &gitlab.GetMergeRequestDiffVersionsOptions{ + Page: pageNum, + }, gitlab.WithContext(reqCtx)) + }) + if err != nil { + return nil, err + } + if len(vers) == 0 { + return nil, errors.New("no merge request versions found") + } + return vers[0], nil +} + +func (gl GitLabReporter) tooManyComments(ctx context.Context, mrNum, nrComments int) error { + msg := fmt.Sprintf(`This pint run would create %d comment(s), which is more than %d limit configured for pint. + %d comments were skipped and won't be visibile on this PR.`, nrComments, gl.maxComments, nrComments-gl.maxComments) + opt := gitlab.CreateMergeRequestDiscussionOptions{ + Body: gitlab.Ptr(msg), + } + + slog.Debug("Creating a PR comment", slog.String("body", msg)) + + reqCtx, cancel := context.WithTimeout(ctx, gl.timeout) + defer cancel() + + _, _, err := gl.client.Discussions.CreateMergeRequestDiscussion(gl.project, mrNum, &opt, gitlab.WithContext(reqCtx)) + return err +} + +func reportToGitLabDiscussion(pending PendingCommentV2, diffs []*gitlab.MergeRequestDiff, ver *gitlab.MergeRequestDiffVersion) *gitlab.CreateMergeRequestDiscussionOptions { + diff := getDiffForPath(diffs, pending.path) + if diff == nil { + slog.Debug("Skipping report for path with no GitLab diff", + slog.String("path", pending.path), + ) + return nil + } + + d := gitlab.CreateMergeRequestDiscussionOptions{ + Body: gitlab.Ptr(pending.text), + Position: &gitlab.PositionOptions{ + PositionType: gitlab.Ptr("text"), + BaseSHA: gitlab.Ptr(ver.BaseCommitSHA), + HeadSHA: gitlab.Ptr(ver.HeadCommitSHA), + StartSHA: gitlab.Ptr(ver.StartCommitSHA), + OldPath: gitlab.Ptr(diff.OldPath), + NewPath: gitlab.Ptr(diff.NewPath), + }, + } + + dl, ok := diffLineFor(parseDiffLines(diff), pending.line) + switch { + case !ok: + // No diffLine for this line, most likely unmodified ?. + d.Position.NewLine = gitlab.Ptr(dl.new) + d.Position.OldLine = gitlab.Ptr(dl.old) + case pending.anchor == checks.AnchorBefore: + // Comment on removed line. + d.Position.OldLine = gitlab.Ptr(dl.old) + case ok && !dl.wasModified: + // Commend on unmodified line. + d.Position.NewLine = gitlab.Ptr(dl.new) + d.Position.OldLine = gitlab.Ptr(dl.old) + default: + // Commend on new or modified line. + d.Position.NewLine = gitlab.Ptr(dl.new) + } + + return &d +} + +func getDiffForPath(diffs []*gitlab.MergeRequestDiff, path string) *gitlab.MergeRequestDiff { + for _, change := range diffs { + if change.NewPath == path { + return change + } + } + return nil +} + +type diffLine struct { + old int + new int + wasModified bool +} + +func diffLineFor(lines []diffLine, line int) (diffLine, bool) { + for _, dl := range lines { + if dl.new == line { + return dl, true + } + } + return diffLine{}, false +} + +var diffRe = regexp.MustCompile(`@@ \-(\d+),(\d+) \+(\d+),(\d+) @@`) + +func parseDiffLines(diff *gitlab.MergeRequestDiff) (lines []diffLine) { + var oldLine, newLine int + + sc := bufio.NewScanner(strings.NewReader(diff.Diff)) + for sc.Scan() { + line := sc.Text() + switch { + case strings.HasPrefix(line, "@@"): + matches := diffRe.FindStringSubmatch(line) + if len(matches) == 5 { + oldLine, _ = strconv.Atoi(matches[1]) + newLine, _ = strconv.Atoi(matches[3]) + } + case strings.HasPrefix(line, "-"): + oldLine++ + case strings.HasPrefix(line, "+"): + lines = append(lines, diffLine{old: oldLine, new: newLine, wasModified: true}) + newLine++ + default: + lines = append(lines, diffLine{old: oldLine, new: newLine, wasModified: false}) + oldLine++ + newLine++ + } + } + + return lines +} + +func getGitLabPaginated[T any](searchFunc func(pageNum int) ([]T, *gitlab.Response, error)) ([]T, *gitlab.Response, error) { + items := []T{} + pageNum := 1 + for { + tempItems, response, err := searchFunc(pageNum) + if err != nil { + return nil, response, err + } + items = append(items, tempItems...) + if response.NextPage == 0 { + break + } + pageNum = response.NextPage + } + return items, nil, nil +} + +func loggifyDiscussion(opt *gitlab.CreateMergeRequestDiscussionOptions) (attrs []any) { + if opt.Position == nil { + return nil + } + if opt.Position.BaseSHA != nil { + attrs = append(attrs, slog.String("base_sha", *opt.Position.BaseSHA)) + } + if opt.Position.HeadSHA != nil { + attrs = append(attrs, slog.String("head_sha", *opt.Position.HeadSHA)) + } + if opt.Position.StartSHA != nil { + attrs = append(attrs, slog.String("start_sha", *opt.Position.StartSHA)) + } + if opt.Position.OldPath != nil { + attrs = append(attrs, slog.String("old_path", *opt.Position.OldPath)) + } + if opt.Position.NewPath != nil { + attrs = append(attrs, slog.String("new_path", *opt.Position.NewPath)) + } + if opt.Position.OldLine != nil { + attrs = append(attrs, slog.Int("old_line", *opt.Position.OldLine)) + } + if opt.Position.NewLine != nil { + attrs = append(attrs, slog.Int("new_line", *opt.Position.NewLine)) + } + return attrs +} + +func (gl GitLabReporter) Submit(summary Summary) (err error) { + return Submit(context.Background(), summary, gl) +} diff --git a/internal/reporter/gitlab_test.go b/internal/reporter/gitlab_test.go new file mode 100644 index 00000000..332e9807 --- /dev/null +++ b/internal/reporter/gitlab_test.go @@ -0,0 +1,134 @@ +package reporter_test + +import ( + "context" + "fmt" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/neilotoole/slogt" + + "github.com/cloudflare/pint/internal/checks" + "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/parser" + "github.com/cloudflare/pint/internal/reporter" +) + +func TestGitLabReporter(t *testing.T) { + type errorCheck func(err error) error + + type testCaseT struct { + httpHandler http.Handler + errorHandler errorCheck + + description string + branch string + token string + + reports []reporter.Report + timeout time.Duration + project int + maxComments int + } + + p := parser.NewParser() + mockRules, _ := p.Parse([]byte(` +- record: target is down + expr: up == 0 +- record: sum errors + expr: sum(errors) by (job) +`)) + + testCases := []testCaseT{ + { + description: "returns an error on non-200 HTTP response", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 0, + reports: []reporter.Report{ + { + Path: discovery.Path{ + SymlinkTarget: "foo.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2}, + Rule: mockRules[0], + Problem: checks.Problem{}, + }, + }, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(400) + _, _ = w.Write([]byte("Bad Request")) + }), + errorHandler: func(err error) error { + if err != nil && strings.HasPrefix(err.Error(), "failed to get GitLab user ID:") { + return nil + } + return fmt.Errorf("wrong error: %w", err) + }, + }, + { + description: "returns an error on HTTP response timeout", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 0, + reports: []reporter.Report{ + { + Path: discovery.Path{ + SymlinkTarget: "foo.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2}, + Rule: mockRules[0], + Problem: checks.Problem{}, + }, + }, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + time.Sleep(time.Second * 2) + w.WriteHeader(400) + _, _ = w.Write([]byte("Bad Request")) + }), + errorHandler: func(err error) error { + if err != nil && strings.HasSuffix(err.Error(), "context deadline exceeded") { + return nil + } + return fmt.Errorf("wrong error: %w", err) + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + slog.SetDefault(slogt.New(t)) + + srv := httptest.NewServer(tc.httpHandler) + defer srv.Close() + + r, err := reporter.NewGitLabReporter( + "v0.0.0", + tc.branch, + srv.URL, + tc.timeout, + tc.token, + tc.project, + tc.maxComments, + ) + if err == nil { + summary := reporter.NewSummary(tc.reports) + err = reporter.Submit(context.Background(), summary, r) + } + if e := tc.errorHandler(err); e != nil { + t.Errorf("error check failure: %s", e) + return + } + }) + } +} diff --git a/internal/reporter/reporter.go b/internal/reporter/reporter.go index 8d0d44ef..07a923c1 100644 --- a/internal/reporter/reporter.go +++ b/internal/reporter/reporter.go @@ -1,11 +1,14 @@ package reporter import ( + "context" + "log/slog" "sort" "time" "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/output" "github.com/cloudflare/pint/internal/parser" ) @@ -123,3 +126,87 @@ func (s Summary) CountBySeverity() map[checks.Severity]int { type Reporter interface { Submit(Summary) error } + +func Submit(ctx context.Context, s Summary, c Commenter) error { + dsts, err := c.Destinations(ctx) + if err != nil { + return err + } + + for _, dst := range dsts { + if err = updateDestination(ctx, s, c, dst); err != nil { + return err + } + } + + return nil +} + +func updateDestination(ctx context.Context, s Summary, c Commenter, dst any) error { + slog.Info("Creating report summary", + slog.String("reporter", c.Describe()), + slog.Int("reports", len(s.reports)), + slog.Int("online", int(s.OnlineChecks)), + slog.Int("offline", int(s.OnlineChecks)), + slog.String("duration", output.HumanizeDuration(s.Duration)), + slog.Int("entries", s.TotalEntries), + slog.Int("checked", int(s.CheckedEntries)), + ) + if err := c.Summary(ctx, dst, s); err != nil { + return err + } + + slog.Info("Listing existing comments", slog.String("reporter", c.Describe())) + existingComments, err := c.List(ctx, dst) + if err != nil { + return err + } + + var created int + pendingComments := makeComments(s) + for _, pending := range pendingComments { + for _, existing := range existingComments { + if c.IsEqual(existing, pending) { + slog.Debug("Comment already exists", + slog.String("reporter", c.Describe()), + slog.String("path", pending.path), + slog.Int("line", pending.line), + ) + goto NEXTCreate + } + } + if !c.CanCreate(created) { + goto NEXTCreate + } + if err := c.Create(ctx, dst, pending); err != nil { + slog.Error("Failed to create a new comment", + slog.String("reporter", c.Describe()), + slog.String("path", pending.path), + slog.Int("line", pending.line), + slog.Any("err", err), + ) + return err + } + created++ + NEXTCreate: + } + + for _, existing := range existingComments { + for _, pending := range pendingComments { + if c.IsEqual(existing, pending) { + goto NEXTDelete + } + } + if err := c.Delete(ctx, dst, existing); err != nil { + slog.Error("Failed to delete a stale comment", + slog.String("reporter", c.Describe()), + slog.String("path", existing.path), + slog.Int("line", existing.line), + slog.Any("err", err), + ) + } + NEXTDelete: + } + + return nil +} From 7d4b6f3c8756d470ec1295ce93ec45335b255811 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Thu, 23 May 2024 16:07:03 +0100 Subject: [PATCH 2/2] Add tests --- .golangci.yml | 14 - internal/reporter/comments.go | 15 +- internal/reporter/comments_test.go | 771 +++++++++++++++++++++++++++++ internal/reporter/gitlab.go | 36 +- internal/reporter/gitlab_test.go | 199 +++++++- internal/reporter/reporter.go | 47 +- 6 files changed, 1018 insertions(+), 64 deletions(-) create mode 100644 internal/reporter/comments_test.go diff --git a/.golangci.yml b/.golangci.yml index 4dde71fa..61903c51 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -2,12 +2,8 @@ run: timeout: 5m tests: true -output: - sort-results: true - linters: enable: - - depguard - gofumpt - goimports - revive @@ -32,16 +28,6 @@ issues: max-same-issues: 0 linters-settings: - depguard: - rules: - test: - files: - - "$test" - deny: - - pkg: "github.com/stretchr/testify/assert" - desc: "Use github.com/stretchr/testify/require instead of github.com/stretchr/testify/assert" - - pkg: "io/ioutil" - desc: "Use corresponding 'os' or 'io' functions instead." goimports: local-prefixes: github.com/cloudflare/pint gofumpt: diff --git a/internal/reporter/comments.go b/internal/reporter/comments.go index ca0e1529..ca0237c2 100644 --- a/internal/reporter/comments.go +++ b/internal/reporter/comments.go @@ -2,6 +2,7 @@ package reporter import ( "context" + "slices" "strings" "github.com/cloudflare/pint/internal/checks" @@ -24,11 +25,11 @@ type ExistingCommentV2 struct { type Commenter interface { Describe() string Destinations(context.Context) ([]any, error) - Summary(context.Context, any, Summary) error + Summary(context.Context, any, Summary, []error) error List(context.Context, any) ([]ExistingCommentV2, error) Create(context.Context, any, PendingCommentV2) error Delete(context.Context, any, ExistingCommentV2) error - CanCreate(int) bool + CanCreate(int) (bool, error) IsEqual(ExistingCommentV2, PendingCommentV2) bool } @@ -70,10 +71,18 @@ func makeComments(summary Summary) (comments []PendingCommentV2) { buf.WriteString(reports[0].Problem.Reporter) buf.WriteString(".html).\n") + line := reports[0].Problem.Lines.Last + for i := reports[0].Problem.Lines.Last; i >= reports[0].Problem.Lines.First; i-- { + if slices.Contains(reports[0].ModifiedLines, i) { + line = i + break + } + } + comments = append(comments, PendingCommentV2{ anchor: reports[0].Problem.Anchor, path: reports[0].Path.SymlinkTarget, - line: reports[0].Problem.Lines.Last, + line: line, text: buf.String(), }) } diff --git a/internal/reporter/comments_test.go b/internal/reporter/comments_test.go new file mode 100644 index 00000000..99f16e1d --- /dev/null +++ b/internal/reporter/comments_test.go @@ -0,0 +1,771 @@ +package reporter + +import ( + "context" + "errors" + "fmt" + "log/slog" + "testing" + + "github.com/neilotoole/slogt" + "github.com/stretchr/testify/require" + + "github.com/cloudflare/pint/internal/checks" + "github.com/cloudflare/pint/internal/discovery" + "github.com/cloudflare/pint/internal/parser" +) + +var ( + errSummary = errors.New("Summary() error") + errList = errors.New("List() error") + errCanCreate = errors.New("CanCreate() error") + errCreate = errors.New("Create() error") + errDelete = errors.New("Delete() error") +) + +type testCommenter struct { + destinations func(context.Context) ([]any, error) + summary func(context.Context, any, Summary, []error) error + list func(context.Context, any) ([]ExistingCommentV2, error) + create func(context.Context, any, PendingCommentV2) error + delete func(context.Context, any, ExistingCommentV2) error + canCreate func(int) (bool, error) + isEqual func(ExistingCommentV2, PendingCommentV2) bool +} + +func (tc testCommenter) Describe() string { + return "testCommenter" +} + +func (tc testCommenter) Destinations(ctx context.Context) ([]any, error) { + return tc.destinations(ctx) +} + +func (tc testCommenter) Summary(ctx context.Context, dst any, s Summary, errs []error) error { + return tc.summary(ctx, dst, s, errs) +} + +func (tc testCommenter) List(ctx context.Context, dst any) ([]ExistingCommentV2, error) { + return tc.list(ctx, dst) +} + +func (tc testCommenter) Create(ctx context.Context, dst any, comment PendingCommentV2) error { + return tc.create(ctx, dst, comment) +} + +func (tc testCommenter) Delete(ctx context.Context, dst any, comment ExistingCommentV2) error { + return tc.delete(ctx, dst, comment) +} + +func (tc testCommenter) CanCreate(n int) (bool, error) { + return tc.canCreate(n) +} + +func (tc testCommenter) IsEqual(e ExistingCommentV2, p PendingCommentV2) bool { + return tc.isEqual(e, p) +} + +func TestCommenter(t *testing.T) { + p := parser.NewParser() + mockRules, _ := p.Parse([]byte(` +- record: target is down + expr: up == 0 +- record: sum errors + expr: sum(errors) by (job) +`)) + + fooReport := Report{ + Path: discovery.Path{ + SymlinkTarget: "foo.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2}, + Rule: mockRules[0], + Problem: checks.Problem{ + Reporter: "foo", + Text: "foo error", + Details: "foo details", + Lines: parser.LineRange{First: 1, Last: 3}, + Severity: checks.Fatal, + Anchor: checks.AnchorAfter, + }, + } + fooComment := ExistingCommentV2{ + path: "foo.txt", + line: 2, + text: `:stop_sign: **Fatal** reported by [pint](https://cloudflare.github.io/pint/) **foo** check. + +------ + +foo error + +foo details + +------ + +:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/foo.html). +`, + meta: nil, + } + + barReport := Report{ + Path: discovery.Path{ + SymlinkTarget: "bar.txt", + Name: "bar.txt", + }, + ModifiedLines: []int{1}, + Rule: mockRules[0], + Problem: checks.Problem{ + Reporter: "bar", + Text: "bar warning", + Details: "", + Lines: parser.LineRange{First: 1, Last: 1}, + Severity: checks.Warning, + Anchor: checks.AnchorBefore, + }, + } + barComment := ExistingCommentV2{ + path: "bar.txt", + line: 1, + text: `:warning: **Warning** reported by [pint](https://cloudflare.github.io/pint/) **bar** check. + +------ + +bar warning + +------ + +:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/bar.html). +`, + meta: nil, + } + + type testCaseT struct { + commenter Commenter + checkErr func(t *testing.T, err error) + description string + reports []Report + } + + testCases := []testCaseT{ + { + description: "no-op when there are no destinations", + reports: []Report{}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{}, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "stops if List() fails", + reports: []Report{}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return nil, errList + }, + }, + checkErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, errList) + }, + }, + { + description: "no-op when all comments already exist", + reports: []Report{fooReport, barReport}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return []ExistingCommentV2{fooComment, barComment}, nil + }, + create: func(_ context.Context, _ any, p PendingCommentV2) error { + return fmt.Errorf("shouldn't try to create %s:%d", p.path, p.line) + }, + delete: func(_ context.Context, _ any, e ExistingCommentV2) error { + return fmt.Errorf("shouldn't try to delete %s:%d", e.path, e.line) + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return true, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "creates missing comments", + reports: []Report{fooReport, barReport}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return []ExistingCommentV2{fooComment}, nil + }, + create: func(_ context.Context, _ any, p PendingCommentV2) error { + if p.path == barComment.path && p.line == barComment.line && p.text == barComment.text { + return nil + } + return fmt.Errorf("shouldn't try to create %s:%d", p.path, p.line) + }, + delete: func(_ context.Context, _ any, e ExistingCommentV2) error { + return fmt.Errorf("shouldn't try to delete %s:%d", e.path, e.line) + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return true, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "skips creating comments when CanCreate() is false", + reports: []Report{barReport, fooReport}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return nil, nil + }, + create: func(_ context.Context, _ any, p PendingCommentV2) error { + if p.path == barComment.path && p.line == barComment.line && p.text == barComment.text { + return nil + } + return fmt.Errorf("shouldn't try to create %s:%d", p.path, p.line) + }, + delete: func(_ context.Context, _ any, e ExistingCommentV2) error { + return fmt.Errorf("shouldn't try to delete %s:%d", e.path, e.line) + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(n int) (bool, error) { + return n == 0, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "CanCreate() fails", + reports: []Report{barReport, fooReport}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 2 { + return fmt.Errorf("Expected errCanCreate in errs, got %v", errs) + } + if !errors.Is(errs[0], errCanCreate) { + return fmt.Errorf("Expected errCanCreate in errs, got %w", errs[0]) + } + if !errors.Is(errs[1], errCanCreate) { + return fmt.Errorf("Expected errCanCreate in errs, got %w", errs[1]) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return nil, nil + }, + create: func(_ context.Context, _ any, p PendingCommentV2) error { + if p.path == barComment.path && p.line == barComment.line && p.text == barComment.text { + return nil + } + return fmt.Errorf("shouldn't try to create %s:%d", p.path, p.line) + }, + delete: func(_ context.Context, _ any, e ExistingCommentV2) error { + return fmt.Errorf("shouldn't try to delete %s:%d", e.path, e.line) + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return false, errCanCreate + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "Create() fails", + reports: []Report{barReport, fooReport}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return nil, nil + }, + create: func(_ context.Context, _ any, p PendingCommentV2) error { + if p.path == barComment.path && p.line == barComment.line && p.text == barComment.text { + return nil + } + return errCreate + }, + delete: func(_ context.Context, _ any, e ExistingCommentV2) error { + return fmt.Errorf("shouldn't try to delete %s:%d", e.path, e.line) + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return true, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, errCreate) + }, + }, + { + description: "Create() works", + reports: []Report{barReport, fooReport}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return nil, nil + }, + create: func(_ context.Context, _ any, p PendingCommentV2) error { + if p.path == barComment.path && p.line == barComment.line && p.text == barComment.text { + return nil + } + if p.path == fooComment.path && p.line == fooComment.line && p.text == fooComment.text { + return nil + } + return fmt.Errorf("unexpected comment at %s:%d: %s", p.path, p.line, p.text) + }, + delete: func(_ context.Context, _ any, e ExistingCommentV2) error { + return fmt.Errorf("shouldn't try to delete %s:%d", e.path, e.line) + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return true, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "Create() merges details", + reports: []Report{ + { + Path: discovery.Path{ + SymlinkTarget: "bar.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2, 3, 4, 5}, + Rule: mockRules[0], + Problem: checks.Problem{ + Reporter: "foo", + Text: "foo error 1", + Details: "foo details", + Lines: parser.LineRange{First: 1, Last: 3}, + Severity: checks.Bug, + Anchor: checks.AnchorAfter, + }, + }, + { + Path: discovery.Path{ + SymlinkTarget: "bar.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2, 3, 4, 5}, + Rule: mockRules[0], + Problem: checks.Problem{ + Reporter: "foo", + Text: "foo error 2", + Details: "foo details", + Lines: parser.LineRange{First: 1, Last: 3}, + Severity: checks.Bug, + Anchor: checks.AnchorAfter, + }, + }, + }, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return nil, nil + }, + create: func(_ context.Context, _ any, p PendingCommentV2) error { + if p.path != "bar.txt" { + return fmt.Errorf("wrong path: %s", p.path) + } + if p.line != 3 { + return fmt.Errorf("wrong line: %d", p.line) + } + if p.text != `:stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **foo** check. + +------ + +foo error 1 + +:leftwards_arrow_with_hook: This problem was detected on a symlinked file `+"`foo.txt`"+`. + +------ + +foo error 2 + +:leftwards_arrow_with_hook: This problem was detected on a symlinked file `+"`foo.txt`"+`. + +------ + +foo details + +------ + +:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/foo.html). +` { + return fmt.Errorf("wrong text: %s", p.text) + } + return nil + }, + delete: func(_ context.Context, _ any, e ExistingCommentV2) error { + return fmt.Errorf("shouldn't try to delete %s:%d", e.path, e.line) + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return true, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "Create() reports symlink", + reports: []Report{ + { + Path: discovery.Path{ + SymlinkTarget: "bar.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2, 3, 4, 5}, + Rule: mockRules[0], + Problem: checks.Problem{ + Reporter: "foo", + Text: "foo error", + Details: "foo details", + Lines: parser.LineRange{First: 1, Last: 3}, + Severity: checks.Bug, + Anchor: checks.AnchorAfter, + }, + }, + }, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return nil, nil + }, + create: func(_ context.Context, _ any, p PendingCommentV2) error { + if p.path != "bar.txt" { + return fmt.Errorf("wrong path: %s", p.path) + } + if p.line != 3 { + return fmt.Errorf("wrong line: %d", p.line) + } + if p.text != `:stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **foo** check. + +------ + +foo error + +foo details + +:leftwards_arrow_with_hook: This problem was detected on a symlinked file `+"`foo.txt`"+`. + +------ + +:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/foo.html). +` { + return fmt.Errorf("wrong text: %s", p.text) + } + return nil + }, + delete: func(_ context.Context, _ any, e ExistingCommentV2) error { + return fmt.Errorf("shouldn't try to delete %s:%d", e.path, e.line) + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return true, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "Create() doesn't merge details with different line range", + reports: []Report{ + { + Path: discovery.Path{ + SymlinkTarget: "foo.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2, 3, 4, 5}, + Rule: mockRules[0], + Problem: checks.Problem{ + Reporter: "foo", + Text: "foo error 1", + Details: "foo details", + Lines: parser.LineRange{First: 1, Last: 3}, + Severity: checks.Bug, + Anchor: checks.AnchorAfter, + }, + }, + { + Path: discovery.Path{ + SymlinkTarget: "foo.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2, 3, 4, 5}, + Rule: mockRules[0], + Problem: checks.Problem{ + Reporter: "foo", + Text: "foo error 2", + Details: "foo details", + Lines: parser.LineRange{First: 2, Last: 2}, + Severity: checks.Bug, + Anchor: checks.AnchorAfter, + }, + }, + }, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return nil, nil + }, + create: func(_ context.Context, _ any, p PendingCommentV2) error { + if p.path != "foo.txt" { + return fmt.Errorf("wrong path: %s", p.path) + } + if p.line != 2 && p.line != 3 { + return fmt.Errorf("wrong line: %d", p.line) + } + if p.line == 3 && p.text != `:stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **foo** check. + +------ + +foo error 1 + +foo details + +------ + +:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/foo.html). +` { + return fmt.Errorf("wrong text on first report: %s", p.text) + } + if p.line == 2 && p.text != `:stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **foo** check. + +------ + +foo error 2 + +foo details + +------ + +:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/foo.html). +` { + return fmt.Errorf("wrong text on second report: %s", p.text) + } + return nil + }, + delete: func(_ context.Context, _ any, e ExistingCommentV2) error { + return fmt.Errorf("shouldn't try to delete %s:%d", e.path, e.line) + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return true, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "Delete() fails", + reports: []Report{barReport}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 1 { + return fmt.Errorf("Expected errDelete in errs, got %v", errs) + } + if !errors.Is(errs[0], errDelete) { + return fmt.Errorf("Expected errDelete in errs, got %w", errs[0]) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return []ExistingCommentV2{fooComment}, nil + }, + create: func(_ context.Context, _ any, _ PendingCommentV2) error { + return nil + }, + delete: func(_ context.Context, _ any, _ ExistingCommentV2) error { + return errDelete + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return true, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "Delete() works", + reports: []Report{barReport}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, errs []error) error { + if len(errs) != 0 { + return fmt.Errorf("Expected empty errs, got %v", errs) + } + return nil + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return []ExistingCommentV2{fooComment}, nil + }, + create: func(_ context.Context, _ any, _ PendingCommentV2) error { + return nil + }, + delete: func(_ context.Context, _ any, _ ExistingCommentV2) error { + return nil + }, + isEqual: func(e ExistingCommentV2, p PendingCommentV2) bool { + return e.path == p.path && e.line == p.line && e.text == p.text + }, + canCreate: func(_ int) (bool, error) { + return true, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + { + description: "Summary() fails", + reports: []Report{}, + commenter: testCommenter{ + destinations: func(_ context.Context) ([]any, error) { + return []any{1}, nil + }, + summary: func(_ context.Context, _ any, _ Summary, _ []error) error { + return errSummary + }, + list: func(_ context.Context, _ any) ([]ExistingCommentV2, error) { + return nil, nil + }, + }, + checkErr: func(t *testing.T, err error) { + require.ErrorIs(t, err, errSummary) + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + slog.SetDefault(slogt.New(t)) + + summary := NewSummary(tc.reports) + tc.checkErr(t, Submit(context.Background(), summary, tc.commenter)) + }) + } +} diff --git a/internal/reporter/gitlab.go b/internal/reporter/gitlab.go index c2c39e3a..7f3729c8 100644 --- a/internal/reporter/gitlab.go +++ b/internal/reporter/gitlab.go @@ -138,10 +138,24 @@ func (gl GitLabReporter) Destinations(ctx context.Context) ([]any, error) { return dsts, nil } -func (gl GitLabReporter) Summary(ctx context.Context, dst any, s Summary) error { +func (gl GitLabReporter) Summary(ctx context.Context, dst any, s Summary, errs []error) error { mr := dst.(gitlabMR) if gl.maxComments > 0 && len(s.reports) > gl.maxComments { - if err := gl.tooManyComments(ctx, mr.mrID, len(s.reports)); err != nil { + if err := gl.generalComment(ctx, mr.mrID, tooManyCommentsMsg(len(s.reports), gl.maxComments)); err != nil { + return err + } + } + if len(errs) > 0 { + var buf strings.Builder + buf.WriteString("There were some errors when pint was reporting problems via GitLab APIs.\n") + buf.WriteString("Some review comments might be outdated or missing.\n") + buf.WriteString("List of all errors:\n\n") + for _, err := range errs { + buf.WriteString("- `") + buf.WriteString(err.Error()) + buf.WriteString("`\n") + } + if err := gl.generalComment(ctx, mr.mrID, buf.String()); err != nil { return err } } @@ -239,8 +253,11 @@ func (gl GitLabReporter) IsEqual(existing ExistingCommentV2, pending PendingComm return strings.Trim(existing.text, "\n") == strings.Trim(pending.text, "\n") } -func (gl GitLabReporter) CanCreate(done int) bool { - return done < gl.maxComments +func (gl GitLabReporter) CanCreate(done int) (bool, error) { + if done >= gl.maxComments { + return false, fmt.Errorf(tooManyCommentsMsg(done, gl.maxComments)) + } + return true, nil } func (gl *GitLabReporter) getUserID(ctx context.Context) (int, error) { @@ -304,18 +321,14 @@ func (gl *GitLabReporter) getVersions(ctx context.Context, mrNum int) (*gitlab.M return vers[0], nil } -func (gl GitLabReporter) tooManyComments(ctx context.Context, mrNum, nrComments int) error { - msg := fmt.Sprintf(`This pint run would create %d comment(s), which is more than %d limit configured for pint. - %d comments were skipped and won't be visibile on this PR.`, nrComments, gl.maxComments, nrComments-gl.maxComments) +func (gl GitLabReporter) generalComment(ctx context.Context, mrNum int, msg string) error { opt := gitlab.CreateMergeRequestDiscussionOptions{ Body: gitlab.Ptr(msg), } - slog.Debug("Creating a PR comment", slog.String("body", msg)) reqCtx, cancel := context.WithTimeout(ctx, gl.timeout) defer cancel() - _, _, err := gl.client.Discussions.CreateMergeRequestDiscussion(gl.project, mrNum, &opt, gitlab.WithContext(reqCtx)) return err } @@ -464,3 +477,8 @@ func loggifyDiscussion(opt *gitlab.CreateMergeRequestDiscussionOptions) (attrs [ func (gl GitLabReporter) Submit(summary Summary) (err error) { return Submit(context.Background(), summary, gl) } + +func tooManyCommentsMsg(nr, max int) string { + return fmt.Sprintf(`This pint run would create %d comment(s), which is more than the limit configured for pint (%d). + %d comment(s) were skipped and won't be visibile on this PR.`, nr, max, nr-max) +} diff --git a/internal/reporter/gitlab_test.go b/internal/reporter/gitlab_test.go index 332e9807..82053ebf 100644 --- a/internal/reporter/gitlab_test.go +++ b/internal/reporter/gitlab_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/neilotoole/slogt" + "github.com/stretchr/testify/require" "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/discovery" @@ -18,6 +19,19 @@ import ( "github.com/cloudflare/pint/internal/reporter" ) +func TestGitLabReporterBadBaseURI(t *testing.T) { + _, err := reporter.NewGitLabReporter( + "v0.0.0", + "branch", + "%gh&%ij", + time.Second, + "token", + 123, + 0, + ) + require.Error(t, err) +} + func TestGitLabReporter(t *testing.T) { type errorCheck func(err error) error @@ -43,6 +57,24 @@ func TestGitLabReporter(t *testing.T) { expr: sum(errors) by (job) `)) + fooReport := reporter.Report{ + Path: discovery.Path{ + SymlinkTarget: "foo.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2}, + Rule: mockRules[0], + Problem: checks.Problem{ + Reporter: "foo", + Text: "foo error", + Details: "foo details", + Lines: parser.LineRange{First: 1, Last: 3}, + Severity: checks.Fatal, + Anchor: checks.AnchorAfter, + }, + } + fooDiff := `@@ -1,4 +1,6 @@\n- record: target is down\n- expr: up == 0\n+ expr: up == 1\n+ labels:\n+ foo: bar\n- record: sum errors\nexpr: sum(errors) by (job)\n` + testCases := []testCaseT{ { description: "returns an error on non-200 HTTP response", @@ -50,18 +82,8 @@ func TestGitLabReporter(t *testing.T) { token: "fakeToken", timeout: time.Second, project: 123, - maxComments: 0, - reports: []reporter.Report{ - { - Path: discovery.Path{ - SymlinkTarget: "foo.txt", - Name: "foo.txt", - }, - ModifiedLines: []int{2}, - Rule: mockRules[0], - Problem: checks.Problem{}, - }, - }, + maxComments: 50, + reports: []reporter.Report{fooReport}, httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(400) _, _ = w.Write([]byte("Bad Request")) @@ -79,18 +101,8 @@ func TestGitLabReporter(t *testing.T) { token: "fakeToken", timeout: time.Second, project: 123, - maxComments: 0, - reports: []reporter.Report{ - { - Path: discovery.Path{ - SymlinkTarget: "foo.txt", - Name: "foo.txt", - }, - ModifiedLines: []int{2}, - Rule: mockRules[0], - Problem: checks.Problem{}, - }, - }, + maxComments: 50, + reports: []reporter.Report{fooReport}, httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { time.Sleep(time.Second * 2) w.WriteHeader(400) @@ -103,6 +115,145 @@ func TestGitLabReporter(t *testing.T) { return fmt.Errorf("wrong error: %w", err) }, }, + { + description: "returns an error on non-json body", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 50, + reports: []reporter.Report{fooReport}, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(200) + _, _ = w.Write([]byte("OK")) + }), + errorHandler: func(err error) error { + if err != nil && strings.HasPrefix(err.Error(), "failed to get GitLab user ID:") { + return nil + } + return fmt.Errorf("wrong error: %w", err) + }, + }, + { + description: "returns an error on empty JSON body", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 50, + reports: []reporter.Report{fooReport}, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(200) + _, _ = w.Write([]byte("{}")) + }), + errorHandler: func(err error) error { + if err != nil && strings.HasPrefix(err.Error(), "failed to get GitLab merge request details: json: cannot unmarshal object into Go value of type") { + return nil + } + return fmt.Errorf("wrong error: %w", err) + }, + }, + { + description: "empty list of merge requests", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 50, + reports: []reporter.Report{fooReport}, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + switch r.URL.Path { + case "/api/v4/user": + _, _ = w.Write([]byte(`{"id": 123}`)) + default: + _, _ = w.Write([]byte("[]")) + } + }), + errorHandler: func(err error) error { + return err + }, + }, + { + description: "multiple merge requests", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 50, + reports: []reporter.Report{fooReport}, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + switch r.URL.Path { + case "/api/v4/user": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"id": 123}`)) + } + case "/api/v4/projects/123/merge_requests": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[{"iid":1},{"iid":2},{"iid":5}]`)) + } + case "/api/v4/projects/123/merge_requests/1/diffs", "/api/v4/projects/123/merge_requests/2/diffs", "/api/v4/projects/123/merge_requests/5/diffs": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[{"diff":"` + fooDiff + `","new_path":"foo.txt","old_path":"foo.txt"}]`)) + } + case "/api/v4/projects/123/merge_requests/1/versions", "/api/v4/projects/123/merge_requests/2/versions", "/api/v4/projects/123/merge_requests/5/versions": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[ +{"id": 2,"head_commit_sha": "head","base_commit_sha": "base","start_commit_sha": "start"}, +{"id": 1,"head_commit_sha": "head","base_commit_sha": "base","start_commit_sha": "start"} +]`)) + } + case "/api/v4/projects/123/merge_requests/1/discussions", "/api/v4/projects/123/merge_requests/2/discussions": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[ +{"id":"100","notes":[ + {"id":101, + "system":true, + "author":{"id":123}, + "body":"system message" + } +]}, +{"id":"200","system":false,"notes":[ + {"id":201, + "system":false, + "author":{"id":321}, + "position":{"base_sha": "base","start_sha": "start","head_sha": "head","old_path": "foo.txt","new_path": "foo.txt","position_type": "text","new_line": 2}, + "body":"different user" + } +]}, +{"id":"300","system":false,"notes":[ + {"id":301, + "system":false, + "author":{"id":123}, + "position":{"base_sha": "base","start_sha": "start","head_sha": "head","old_path": "foo.txt","new_path": "foo.txt","position_type": "text","new_line": 2}, + "body":"stale comment" + } +]}, +{"id":"400","system":false,"notes":[ + {"id":401, + "system":false, + "author":{"id":123}, + "position":{"base_sha": "base","start_sha": "start","head_sha": "head","old_path": "foo.txt","new_path": "foo.txt","position_type": "text","new_line": 1}, + "body":"stale comment on unmodified line" + } +]} +]`)) + } else { + _, _ = w.Write([]byte(`{}`)) + } + case "/api/v4/projects/123/merge_requests/5/discussions": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[]`)) + } else { + _, _ = w.Write([]byte(`{}`)) + } + } + }), + errorHandler: func(err error) error { + return err + }, + }, } for _, tc := range testCases { diff --git a/internal/reporter/reporter.go b/internal/reporter/reporter.go index 07a923c1..0ffcff12 100644 --- a/internal/reporter/reporter.go +++ b/internal/reporter/reporter.go @@ -143,19 +143,6 @@ func Submit(ctx context.Context, s Summary, c Commenter) error { } func updateDestination(ctx context.Context, s Summary, c Commenter, dst any) error { - slog.Info("Creating report summary", - slog.String("reporter", c.Describe()), - slog.Int("reports", len(s.reports)), - slog.Int("online", int(s.OnlineChecks)), - slog.Int("offline", int(s.OnlineChecks)), - slog.String("duration", output.HumanizeDuration(s.Duration)), - slog.Int("entries", s.TotalEntries), - slog.Int("checked", int(s.CheckedEntries)), - ) - if err := c.Summary(ctx, dst, s); err != nil { - return err - } - slog.Info("Listing existing comments", slog.String("reporter", c.Describe())) existingComments, err := c.List(ctx, dst) if err != nil { @@ -163,6 +150,8 @@ func updateDestination(ctx context.Context, s Summary, c Commenter, dst any) err } var created int + var ok bool + var errs []error pendingComments := makeComments(s) for _, pending := range pendingComments { for _, existing := range existingComments { @@ -175,9 +164,25 @@ func updateDestination(ctx context.Context, s Summary, c Commenter, dst any) err goto NEXTCreate } } - if !c.CanCreate(created) { + slog.Debug("Comment doesn't exist yet", + slog.String("reporter", c.Describe()), + slog.String("path", pending.path), + slog.Int("line", pending.line), + ) + + ok, err = c.CanCreate(created) + if err != nil { + errs = append(errs, err) + } + if !ok { + slog.Debug("Cannot create new comment", + slog.String("reporter", c.Describe()), + slog.String("path", pending.path), + slog.Int("line", pending.line), + ) goto NEXTCreate } + if err := c.Create(ctx, dst, pending); err != nil { slog.Error("Failed to create a new comment", slog.String("reporter", c.Describe()), @@ -204,9 +209,23 @@ func updateDestination(ctx context.Context, s Summary, c Commenter, dst any) err slog.Int("line", existing.line), slog.Any("err", err), ) + errs = append(errs, err) } NEXTDelete: } + slog.Info("Creating report summary", + slog.String("reporter", c.Describe()), + slog.Int("reports", len(s.reports)), + slog.Int("online", int(s.OnlineChecks)), + slog.Int("offline", int(s.OnlineChecks)), + slog.String("duration", output.HumanizeDuration(s.Duration)), + slog.Int("entries", s.TotalEntries), + slog.Int("checked", int(s.CheckedEntries)), + ) + if err := c.Summary(ctx, dst, s, errs); err != nil { + return err + } + return nil }