Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

fix creating GitLab comments on unmodified lines #1147

Merged
merged 2 commits into from
Oct 21, 2024
Merged

fix creating GitLab comments on unmodified lines #1147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e0d9a9
getting all GitLab discussions using pagination
alexintech Oct 17, 2024
fc704ae
fixing incorrect request new_line=0,old_line=0 for GitLab in case if …
alexintech Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 45 additions & 9 deletions internal/reporter/gitlab.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,7 @@ func (gl GitLabReporter) Summary(ctx context.Context, dst any, s Summary, errs [

func (gl GitLabReporter) List(ctx context.Context, dst any) ([]ExistingComment, 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))
discs, err := gl.getDiscussions(ctx, mr.mrID)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -313,6 +310,18 @@ func (gl *GitLabReporter) getVersions(ctx context.Context, mrNum int) (*gitlab.M
return vers[0], nil
}

func (gl *GitLabReporter) getDiscussions(ctx context.Context, mrNum int) ([]*gitlab.Discussion, error) {
slog.Debug("Getting the list of merge request discussions", slog.Int("mr", mrNum))
discs, _, err := getGitLabPaginated(func(pageNum int) ([]*gitlab.Discussion, *gitlab.Response, error) {
reqCtx, cancel := context.WithTimeout(ctx, gl.timeout)
defer cancel()
return gl.client.Discussions.ListMergeRequestDiscussions(gl.project, mrNum, &gitlab.ListMergeRequestDiscussionsOptions{
Page: pageNum,
}, gitlab.WithContext(reqCtx))
})
return discs, err
}

func (gl GitLabReporter) generalComment(ctx context.Context, mrNum int, msg string) error {
opt := gitlab.CreateMergeRequestDiscussionOptions{
Body: gitlab.Ptr(msg),
Expand Down Expand Up @@ -350,17 +359,17 @@ func reportToGitLabDiscussion(pending PendingComment, diffs []*gitlab.MergeReque
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)
d.Position.NewLine = gitlab.Ptr(pending.line)
d.Position.OldLine = gitlab.Ptr(pending.line)
case pending.anchor == checks.AnchorBefore:
// Comment on removed line.
d.Position.OldLine = gitlab.Ptr(dl.old)
case ok && !dl.wasModified:
// Commend on unmodified line.
// Comment on unmodified line.
d.Position.NewLine = gitlab.Ptr(dl.new)
d.Position.OldLine = gitlab.Ptr(dl.old)
default:
// Commend on new or modified line.
// Comment on new or modified line.
d.Position.NewLine = gitlab.Ptr(dl.new)
}

Expand All @@ -383,10 +392,37 @@ type diffLine struct {
}

func diffLineFor(lines []diffLine, line int) (diffLine, bool) {
for _, dl := range lines {
if len(lines) == 0 {
return diffLine{}, false
}

for i, dl := range lines {
if dl.new == line {
return dl, true
}
// Calculate unmodified line that does not present in the diff
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is going on here? Why are you looking for unmodified lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff commonly includes some lines before changes and some after. If we have small change in a big alert definition, such as:

- alert: AlertName
      annotations:
        dashboard: url
        description: some long description
        runbook_url: url
        summary: summary note
      expr: up > 0
      for: 10m
      labels:
        severity: critical

For example, if we change only runbook_url, diff got from GitLab API may include lines starting annotations: and ending labels:.

Pint validates the whole alert and may find problems, that it want's to publish on unmodified lines, that doesn't present in the diff (for example, at the end list of labels if some label is required).

In such cases we get an error similar to:

level=DEBUG msg="Got pending comment" reporter=GitLab path=alert.yaml line=23 msg=":warning: **Warning** reported by [pint](https://cloudflare.github.io/pint/) **rule/label** check.\n\n------\n\n`alert_for` label is required.\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/rule/label.html).\n"
level=DEBUG msg="Comment doesn't exist yet and needs to be created" reporter=GitLab path=alert.yaml line=23
level=DEBUG msg="Creating a new merge request discussion" base_sha=d135e0bfc702c6d51f70394119a29d04c9695063 head_sha=8ddd5b4d620d808c1b748f31c121dd3c6c822517 start_sha=d135e0bfc702c6d51f70394119a29d04c9695063 old_path=alert.yaml new_path=alert.yaml old_line=0 new_line=0
level=DEBUG msg="performing request" reporter=GitLab method=POST url=https://gitlab/api/v4/projects/123/merge_requests/35/discussions
level=ERROR msg="Failed to create a new comment" reporter=GitLab path=alert.yaml line=23 err="POST https://gitlab/api/v4/projects/123/merge_requests/35/discussions: 400 {message: 400 Bad request - Note {:line_code=>[\"can't be blank\", \"must be a valid line code\"]}}"

That's because parseDiffLines() does not have dl.new=23, and empty diffLine{}, false returns here. In that case case !ok results to incorrect request with new_line=0 old_line=0

So the fix here checks if line before the first diff line (or before the next diff block) and calculates diffLine{} based on the first diff line information. It just calculates the gap here and adds it to lastLines.old to get old_line value.

The same goes here for cases if line is after the all values in diffLines.

Actually, I suppose, that case !ok should not be called anymore (and test coverage shows that it is not called). But maybe I don't see some corner case and missed it in testCases

if dl.new > line {
lastLines := dl
if i > 0 {
lastLines = lines[i-1]
}
gap := line - lastLines.new
return diffLine{
old: lastLines.old + gap,
new: line,
wasModified: false,
}, true
}
}
// Calculate unmodified line that is greater than the last diff line
lastLines := lines[len(lines)-1]
if line > lastLines.new {
gap := line - lastLines.new
return diffLine{
old: lastLines.old + gap,
new: line,
wasModified: false,
}, true
}
return diffLine{}, false
}
Expand Down
206 changes: 206 additions & 0 deletions internal/reporter/gitlab_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package reporter_test

import (
"context"
"io"
"log/slog"
"net/http"
"net/http/httptest"
"strconv"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -396,3 +398,207 @@ func TestGitLabReporter(t *testing.T) {
})
}
}

func TestGitLabReporterCommentLine(t *testing.T) {
type testCaseT struct {
description string
problemLine int
expectedNewLine int
expectedOldLine int
anchor checks.Anchor
}

p := parser.NewParser(false)
mockRules, _ := p.Parse([]byte(`
- record: target is down
expr: up == 0
- record: sum errors
expr: sum(errors) by (job)
`))

multipleDiffs := `@@ -15,6 +15,7 @@ spec:
annotations:
foo: bar
description: some description
+ runbook_url: https://runbook.url
summary: summary text
expr: up == 0
for: 10m
@@ -141,6 +142,5 @@ spec:
description: some description
summary: some summary
expr: sum(errors) by (job)
- for: 15m
labels:
severity: warning`
multipleDiffs = strings.ReplaceAll(multipleDiffs, "\n", "\\n")
multipleDiffs = strings.ReplaceAll(multipleDiffs, "\t", "\\t")

errorHandler := func(err error) error { return err }

testCases := []testCaseT{
{
description: "comment on new line",
problemLine: 18,
expectedNewLine: 18,
},
{
description: "comment on removed line",
problemLine: 145,
expectedOldLine: 145,
anchor: checks.AnchorBefore,
},
{
description: "unmodified line before existing line in the diff",
problemLine: 10,
expectedNewLine: 10,
expectedOldLine: 10,
},
{
description: "unmodified line that exists in the diff",
problemLine: 16,
expectedNewLine: 16,
expectedOldLine: 16,
},
{
description: "unmodified line after added line and exists in the diff",
problemLine: 19,
expectedNewLine: 19,
expectedOldLine: 18,
},
{
description: "unmodified line after added line and not exists in the diff",
problemLine: 23,
expectedNewLine: 23,
expectedOldLine: 22,
},
{
description: "unmodified line before removed line and exists in the diff",
problemLine: 143,
expectedNewLine: 143,
expectedOldLine: 142,
},
{
description: "unmodified line after removed line and exists in the diff",
problemLine: 145,
expectedNewLine: 145,
expectedOldLine: 145,
},
{
description: "unmodified line after removed line and not exists in the diff",
problemLine: 148,
expectedNewLine: 148,
expectedOldLine: 148,
},
}

for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
slog.SetDefault(slogt.New(t))

srv := httptest.NewServer(getHTTPHandlerForCommentingLines(
tc.expectedNewLine, tc.expectedOldLine, multipleDiffs, t))
defer srv.Close()

r, err := reporter.NewGitLabReporter(
"v0.0.0",
"fakeBranch",
srv.URL,
time.Second,
"fakeToken",
123,
10,
)
if err == nil {
summary := reporter.NewSummary([]reporter.Report{
{
Path: discovery.Path{
Name: "foo.txt",
SymlinkTarget: "foo.txt",
},
ModifiedLines: []int{2},
Rule: mockRules[1],
Problem: checks.Problem{
Lines: parser.LineRange{
First: tc.problemLine,
Last: tc.problemLine,
},
Reporter: "mock",
Text: "syntax error",
Details: "syntax details",
Severity: checks.Fatal,
Anchor: tc.anchor,
},
},
})
err = reporter.Submit(context.Background(), summary, r)
}
require.NoError(t, errorHandler(err))
})
}
}

func getHTTPHandlerForCommentingLines(expectedNewLine, expectedOldLine int, diff string, t *testing.T) http.HandlerFunc {
return 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}]`))
}
case "/api/v4/projects/123/merge_requests/1/diffs":
if r.Method == http.MethodGet {
_, _ = w.Write([]byte(`[{"diff":"` + diff + `","new_path":"foo.txt","old_path":"foo.txt"}]`))
}
case "/api/v4/projects/123/merge_requests/1/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":
if r.Method == http.MethodGet {
_, _ = w.Write([]byte(`[]`))
} else {
body, _ := io.ReadAll(r.Body)
b := strings.TrimSpace(strings.TrimRight(string(body), "\n\t\r"))

str := ""
if expectedNewLine != 0 {
str = `"new_line":` + strconv.Itoa(expectedNewLine)
}
if expectedOldLine != 0 {
if str != "" {
str += ","
}
str += `"old_line":` + strconv.Itoa(expectedOldLine)
}

expected := `{
"body":":stop_sign: **Fatal** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nsyntax error\n\nsyntax details\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock.html).\n",
"position":{
"base_sha":"base",
"head_sha":"head",
"start_sha":"start",
"new_path":"foo.txt",
"old_path":"foo.txt",
"position_type":"text",
` + str + `
}
}`
expected = strings.ReplaceAll(expected, "\n", "")
expected = strings.ReplaceAll(expected, "\t", "")
if b != expected {
t.Errorf("Unexpected comment: %s", b)
t.FailNow()
}
_, _ = w.Write([]byte(`{}`))
}
}
})
}