Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix creating GitLab comments on unmodified lines #1147

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexintech
Copy link

This pull request fixes some issues with GitLab Reporter:

  • get all existing comments using pagination
  • fixing incorrect request new_line=0,old_line=0 if comment should be created on unmodified line that doesn't exist in the diff

Added extra tests for different cases for creating comments in GitLab.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.20%. Comparing base (17371fe) to head (3cef775).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
internal/reporter/gitlab.go 94.11% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1147      +/-   ##
==========================================
+ Coverage   95.13%   95.20%   +0.07%     
==========================================
  Files         103      103              
  Lines       10350    10377      +27     
==========================================
+ Hits         9846     9879      +33     
+ Misses        351      346       -5     
+ Partials      153      152       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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
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

@prymitive
Copy link
Collaborator

  • get all existing comments using pagination

Can you split this into a dedicated commit so it's easier to review changes in this PR please?

@alexintech
Copy link
Author

  • get all existing comments using pagination

Can you split this into a dedicated commit so it's easier to review changes in this PR please?

sure, done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants