Make github file annotation levels configurable via zuul return

Change-Id: I4ccc264ce9b05d34e8456821b9cb85a28eaf5ed8

9 files changed, 84 insertions, 19 deletions

diff --git a/doc/source/reference/jobs.rst b/doc/source/reference/jobs.rst
index b39629604..bd4ee2d11 100644
--- a/doc/source/reference/jobs.rst
+++ b/doc/source/reference/jobs.rst
@@ -881,10 +881,12 @@ change, set the **zuul.file_comments** value.  For example:
               path/to/file.py:
                 - line: 42
                   message: "Line too long"
+                  level: info
                 - line: 82
                   message: "Line too short"
                 - line: 119
                   message: "This block is indented too far."
+                  level: warning
                   range:
                     start_line: 117
                     start_character:    0
@@ -893,7 +895,8 @@ change, set the **zuul.file_comments** value.  For example:
 
 Not all reporters currently support line comments (or all of the
 features of line comments); in these cases, reporters will simply
-ignore this data.
+ignore this data. The ``level`` is optional, but if provided must
+be one of ``info``, ``warning``, ``error``.
 
 Zuul will attempt to automatically translate the supplied line numbers
 to the corresponding lines in the original change as written (they may
diff --git a/releasenotes/notes/file-comment-levels-9b26156addc3644f.yaml b/releasenotes/notes/file-comment-levels-9b26156addc3644f.yaml
new file mode 100644
index 000000000..ec5dff2bb
--- /dev/null
+++ b/releasenotes/notes/file-comment-levels-9b26156addc3644f.yaml
@@ -0,0 +1,10 @@
+---
+features:
+  - |
+    The annotation levels for the file comments reported via Github checks
+    API are now configurable in ``zuul_return``. Each file comment entry can
+    provide an optional ``level`` parameter ``[info|warning|error]`` that will
+    be picked up by the Github reporter.
+
+    For more details on how to provide file comments from Zuul, see the
+    documentation of the :ref:`return_values`.
diff --git a/tests/fixtures/config/gerrit-file-comments/git/org_project/playbooks/file-comments.yaml b/tests/fixtures/config/gerrit-file-comments/git/org_project/playbooks/file-comments.yaml
index b281af233..97400bab3 100644
--- a/tests/fixtures/config/gerrit-file-comments/git/org_project/playbooks/file-comments.yaml
+++ b/tests/fixtures/config/gerrit-file-comments/git/org_project/playbooks/file-comments.yaml
@@ -9,6 +9,9 @@
                   message: line too long
                 - line: 82
                   message: line too short
+                - line: 2
+                  message: levels are ignored by gerrit
+                  level: warning
               otherfile.txt:
                 - line: 21
                   message: |
diff --git a/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments-error.yaml b/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments-error.yaml
index a87c1348b..0556ae40e 100644
--- a/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments-error.yaml
+++ b/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments-error.yaml
@@ -19,6 +19,9 @@
                 - line: 7  # No message provided
                 - line: 9999
                   message: Line is not part of the file
+                - line: 3
+                  message: Invalid level will fall back to warning
+                  level: invalid-level
               missingfile.txt:
                 - line: 1
                   message: "Missing file"
diff --git a/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments.yaml b/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments.yaml
index 5838954e9..62f70b1fa 100644
--- a/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments.yaml
+++ b/tests/fixtures/config/github-file-comments/git/org_project/playbooks/file-comments.yaml
@@ -7,8 +7,12 @@
               README:
                 - line: 1
                   message: "Simple line annotation"
+                - line: 2
+                  message: "Line annotation with level"
+                  level: warning
                 - line: 6
                   message: "simple range annotation"
+                  level: info
                   range:
                     start_line: 4
                     start_character: 0
@@ -16,6 +20,7 @@
                     end_character: 10
                 - line: 7
                   message: "Columns must be part of the same line"
+                  level: error
                   range:
                     start_line: 7
                     start_character: 13
diff --git a/tests/unit/test_gerrit.py b/tests/unit/test_gerrit.py
index 5136c8ffe..73f361d22 100644
--- a/tests/unit/test_gerrit.py
+++ b/tests/unit/test_gerrit.py
@@ -246,9 +246,22 @@ class TestFileComments(AnsibleZuulTestCase):
                          'SUCCESS')
         self.assertEqual(self.getJobFromHistory('file-comments-error').result,
                          'SUCCESS')
-        self.assertEqual(len(A.comments), 3)
+        self.assertEqual(len(A.comments), 4)
         comments = sorted(A.comments, key=lambda x: x['line'])
-        self.assertEqual(comments[0],
+        self.assertEqual(
+            comments[0],
+            {
+                "file": "path/to/file.py",
+                "line": 2,
+                "message": "levels are ignored by gerrit",
+                "reviewer": {
+                    "email": "zuul@example.com",
+                    "name": "Zuul",
+                    "username": "jenkins",
+                },
+            },
+        )
+        self.assertEqual(comments[1],
                          {'file': 'otherfile.txt',
                           'line': 21,
                           'message': 'This is a much longer message.\n\n'
@@ -257,7 +270,7 @@ class TestFileComments(AnsibleZuulTestCase):
                                        'name': 'Zuul',
                                        'username': 'jenkins'}}
         )
-        self.assertEqual(comments[1],
+        self.assertEqual(comments[2],
                          {'file': 'path/to/file.py',
                           'line': 42,
                           'message': 'line too long',
@@ -265,7 +278,7 @@ class TestFileComments(AnsibleZuulTestCase):
                                        'name': 'Zuul',
                                        'username': 'jenkins'}}
         )
-        self.assertEqual(comments[2],
+        self.assertEqual(comments[3],
                          {'file': 'path/to/file.py',
                           'line': 82,
                           'message': 'line too short',
diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py
index 8b1488c6d..837b17cdb 100644
--- a/tests/unit/test_github_driver.py
+++ b/tests/unit/test_github_driver.py
@@ -1777,7 +1777,7 @@ class TestCheckRunAnnotations(ZuulGithubAppTestCase, AnsibleZuulTestCase):
         )
 
         annotations = check_run["output"]["annotations"]
-        self.assertEqual(4, len(annotations))
+        self.assertEqual(6, len(annotations))
 
         self.assertEqual(annotations[0], {
             "path": "README",
@@ -1787,19 +1787,27 @@ class TestCheckRunAnnotations(ZuulGithubAppTestCase, AnsibleZuulTestCase):
             "end_line": 1,
         })
 
-        # As the columns are not part of the same line, they are ignored in the
-        # annotation. Otherwise Github will complain about the request.
         self.assertEqual(annotations[1], {
             "path": "README",
             "annotation_level": "warning",
+            "message": "Line annotation with level",
+            "start_line": 2,
+            "end_line": 2,
+        })
+
+        # As the columns are not part of the same line, they are ignored in the
+        # annotation. Otherwise Github will complain about the request.
+        self.assertEqual(annotations[2], {
+            "path": "README",
+            "annotation_level": "notice",
             "message": "simple range annotation",
             "start_line": 4,
             "end_line": 6,
         })
 
-        self.assertEqual(annotations[2], {
+        self.assertEqual(annotations[3], {
             "path": "README",
-            "annotation_level": "warning",
+            "annotation_level": "failure",
             "message": "Columns must be part of the same line",
             "start_line": 7,
             "end_line": 7,
@@ -1810,10 +1818,18 @@ class TestCheckRunAnnotations(ZuulGithubAppTestCase, AnsibleZuulTestCase):
         # From the invalid/error file comments, only the "line out of file"
         # should remain. All others are excluded as they would result in
         # invalid Github requests, making the whole check run update fail.
-        self.assertEqual(annotations[3], {
+        self.assertEqual(annotations[4], {
             "path": "README",
             "annotation_level": "warning",
             "message": "Line is not part of the file",
             "end_line": 9999,
             "start_line": 9999
         })
+
+        self.assertEqual(annotations[5], {
+            "path": "README",
+            "annotation_level": "warning",
+            "message": "Invalid level will fall back to warning",
+            "start_line": 3,
+            "end_line": 3,
+        })
diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py
index 12dcd5202..1bc839113 100644
--- a/zuul/driver/github/githubconnection.py
+++ b/zuul/driver/github/githubconnection.py
@@ -48,6 +48,16 @@ GITHUB_BASE_URL = 'https://api.github.com'
 PREVIEW_JSON_ACCEPT = 'application/vnd.github.machine-man-preview+json'
 PREVIEW_DRAFT_ACCEPT = 'application/vnd.github.shadow-cat-preview+json'
 
+# NOTE (felix): Using log levels for file comments / annotations is IMHO more
+# convenient than the values Github expects. Having in mind that those comments
+# most probably come from various linters, "info", "warning" and "error"
+# should be more general terms than "notice", "warning" and "failure".
+ANNOTATION_LEVELS = {
+    "info": "notice",
+    "warning": "warning",
+    "error": "failure",
+}
+
 
 def _sign_request(body, secret):
     signature = 'sha1=' + hmac.new(
@@ -2011,19 +2021,20 @@ class GithubConnection(BaseConnection):
                         start_column = rng.get("start_character")
                         end_column = rng.get("end_character")
 
-                # TODO (felix): Make annotation_level configurable via
-                # file_comments in zuul_return. Other reporters like Gerrit
-                # might ignore the field if they don't support it.
-                # Accepted values are "notice", "warning", "failure".
-                # A "failure" annotation won't declare the check run as
-                # failure.
+                # Map the level coming from zuul_return to the ones Github
+                # expects. Each Github annotation must provide a level, so
+                # we fall back to "warning" in case no or an invalid level
+                # is provided.
+                annotation_level = ANNOTATION_LEVELS.get(
+                    comment.get("level"), "warning"
+                )
 
                 # A Github check annotation requires at least the following
                 # attributes: "path", "start_line", "end_line", "message" and
                 # "annotation_level"
                 raw_annotation = {
                     "path": fn,
-                    "annotation_level": "warning",
+                    "annotation_level": annotation_level,
                     "message": comment["message"],
                     "start_line": start_line,
                     "end_line": end_line,
diff --git a/zuul/lib/filecomments.py b/zuul/lib/filecomments.py
index a05fc8bf3..fe8399a0d 100644
--- a/zuul/lib/filecomments.py
+++ b/zuul/lib/filecomments.py
@@ -21,7 +21,8 @@ FILE_COMMENT = {
         'start_character': int,
         'end_line': int,
         'end_character': int,
-    }
+    },
+    'level': str,
 }
 
 FILE_COMMENTS = {str: [FILE_COMMENT]}