Uploaded image for project: 'Product Roadmaps'
  1. Product Roadmaps
  2. MMF-1305

PR decoration uses new GitHub Check API

    Details

    • Type: MMF
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Labels:

      Description

      Why ?

      Some while ago, GitHub API changed its behavior and introduced a lot of noise in pull requests decoration : previous reviews are not deleted after new analysis are performed.
      This leads to inconsistent displaying of information as the user sees previous reviews that are not relevant anymore and links to SonarQube issues that may not exist anymore.

      On the other hand GitHub released a new Checks API that offers new features to extend user experience in pull request decoration and will allow to bring more information to the user.

      The goal of this MMF is to first fix the issue introduced and then start an ideation phase to decide new features we can do next using this new API.

      What ?

      As first iteration we’d like to keep the same functional scope by fixing the PR decoration using the GitHub Checks API. Even if we'll try to stay iso-functional, the scope will include status, summary of issues, duplication and coverage and updating on issue change.
      Adding inline and not in the diff issues will come in MMF-1535

      That way, we will have a first step to discover and understand the full potential that the new API can offer.

      For this iteration we expect to use Checks API to create a PR review:

      • Using one check run (see open questions below) with the following details:
        • Overall Status
          The check run status (or "conclusion" in github terms) must be computed as of today, i.e. "failure" in case of any bugs, vulnerabilities or open code smells
        • Summary
          Title : Success: congratulations! or Failed : xx Open Issues.
          Text :
          In case of success : There are no issues : Good job
          In case of failure :

          See issue details in SC/SQ (Link to PR issues)
          Handle singular and plural wording
          Coverage & Duplication
          xxx % Coverage , or "No Coverage information" if no coverage data
          yyy % Duplication, or "No Duplication information" if no duplication data
          See coverage & Duplication on SQ/SC (Link to PR measures )
        • Details (In the Detail section)
          Nothing is provided in the Details

      Analyse Status Check box (at the bottom of the conversation page)

      The text of the status is generated automatically by GitHub and will display the title of the Check Run
      The target display would be like this - to be arranged during the sprint in live

      Issue change

      The check run must be re-created when an issue changes (e.g. resolved as won't fix)

      • to re-create list of annotations
      • to update or re-create details section
      • to possibly update the overall status

      One or more Check Runs?

      Do we want to have a single check run both for issues and coverage or two dedicated runs?

      => single run for now, keep in mind the possibility to improve

      Check run reruns

      Github's UI offers the possibility for user to rerun Check Runs.
      We can not offer support for this feature as of today and therefor rerun webhooks will be ignored.

      What about SonarQube

      SonarQube does not support PR decoration using a GitHub App. Therefor, PR decoration based on PR comments and status is the only PR decoration which can used by SonarQube.

      It's been agreed that, if and when support is added to SonarQube for GitHub Apps, then PR decoration on SonarQube with GitHub App will rely on the Check API (and require user to upgrade to GitHub Enterprise 2.14+ (see release notes)

      Dead links

      Because Check Runs are "immutably" attached to a specific commit, then links to SonarCloud in the Check Run of "old" commits might be those of issues which have since then been closed/removed. Therefore, those will be dead link.

      Also, when a PR is purged from SonarQube, all links to SonarCloud on the Check Runs created by the SonarCloud will be dead. (this problem also affects the current PR decoration with comments and status)

      This situation is accepted and we might think later of a way to avoid those dead links later.

      Migration (PR moving from Review comment to Checks)

      When we will deploy the feature and modify the permission of the gitHub app, the administrator of gitHub organisation will receive a permission request that he can accept or not.

      • When the user accepts the upgrade, and when there are pending PR under review:
        • We delete all comments of the reviewer user
        • And we make the status green if it was red
        • And we (annotate-->)decorate with checks
      • When the user has not yet accepted the upgrade:
        • For existing PR under review, we delete all the comments and we add a comment to tell the user to accept the upgrade of the SonarCloud App
        • For any new PR, we add a comment to tell the user to accept the upgrade of the SonarCloud App

      In any case, we should verify that SQ UI and Checks are consistent (same information in Open Issues, Issue types and Coverage & duplication)

      How?

      Update GitHub App permissions

      New checks:write must be requested by the application.

      Automatic vs manual workflow

      Disable automatic creation of check suites

      According to documentation:

      You must have admin permissions in the repository to set preferences for check suites.

      Since it is likely not acceptable to request the SonarCloud app to have admin permissions on repositories, we must integrate with automatic creation of check suites.

      Actions at end of report processing

      Create a check run with status completed

      1. POST /repos/:owner/:repo/check-runs (see https://developer.github.com/v3/checks/runs/#create-a-check-run)
        • name="SonarCloud Analysis report"
        • head_sha=[sha1 of the report]
        • details_url=[URL of PR on SC]
        • external_id=[analysis UUID?]
        • status="completed"
        • started_at=[submission date? analysis date?]
        • conclusion=[success if QG ok/failure if QG ko]
        • completed_at=[current date]
        • output=
          • title="SonarCloud Analysis report"
          • summary="SonarCloud analysis failed/succeeded. See on SonarCloud"
          • text="There is no issue: Goog job " / "The analysis found: ..."
          • annotations=[none]
        • actions=[none]

      (Optional) Store ID of check run in DB attached to the current analysis

      Action when an issue of a PR is updated

      requirement

      SHA1 of report must be stored attached to the analysis created from the processing of that report.

      If Check Run ID is not stored:

      1. get latest run for the SHA1 of the analysis
      2. do nothing if
        • empty response
        • SHA1 does not match the head of PR anymore

      Compute the new text of the Check Run:

      1. compute 1st part of text from new current "QG" status
      2. compute the number of each issues type by quering ES

      Create a check run with status completed

      1. POST /repos/:owner/:repo/check-runs (see https://developer.github.com/v3/checks/runs/#create-a-check-run)
        • name="SonarCloud Analysis report"
        • head_sha=[sha1 of the report]
        • details_url=[URL of PR on SC]
        • external_id=[analysis UUID?]
        • status="completed"
        • conclusion=[success if QG ok/failure if QG ko]
        • completed_at=[current date]
        • output=[same as when creating]
          • title=[same as when creating]
          • summary=[same as when creating]
          • text=[\new text from new "QG"]
        • annotations=[none]

      Decoration PRs with check API: iteration 2

      Check run is created in "in_progress" state at the beginning of the report processing.
      Check run ID is (OPTIONAL: stored in DB attached to the current analysis and) kept in memory for use at the end of report processing.

      Which ever the status of the report processing, the state of the Check Run must be updated to a completed state:

      1. if the task processing failed: state "canceled"
      2. if the task processing failed: state "failure" or "success" depending on the status of the PR's "QG"

      API investigation data

      Those examples are not up to date with the current specification, but they allow to have a global view of what can be done
      Example of click-though: https://invis.io/TAOGYY463RE#/324458614_Conversation
      Example of real PR: https://github.com/stas-vilchik/fake-js/pull/3

      Receive new webhooks about requested checks

      We are interested in the new webhook CheckSuiteEvent (docs), and particularly for requested and rerequested actions.

      rerequested means that a user is requested a re-run of the check. I don't think we currently support that.

      Example of the webhook payload:

      {
        "action": "requested",
        "check_suite": {
          "id": 18070942,
          "node_id": "MDEwOkNoZWNrU3VpdGUxODA3MDk0Mg==",
          "head_branch": "feature-y",
          "head_sha": "514fee5fe45871488cc16e9a0775b03c5266391d",
          "status": "queued",
          "conclusion": null,
          "url": "https://api.github.com/repos/stas-vilchik/fake-js/check-suites/18070942",
          "before": "0000000000000000000000000000000000000000",
          "after": "514fee5fe45871488cc16e9a0775b03c5266391d",
          "pull_requests": [],
          "app": {
            "id": 18705,
            "node_id": "MDM6QXBwMTg3MDU=",
            "owner": {
              "login": "stas-sonarsource-2",
              "id": 43403651,
              "node_id": "MDEyOk9yZ2FuaXphdGlvbjQzNDAzNjUx",
              "avatar_url": "https://avatars2.githubusercontent.com/u/43403651?v=4",
              "gravatar_id": "",
              "url": "https://api.github.com/users/stas-sonarsource-2",
              "html_url": "https://github.com/stas-sonarsource-2",
              "followers_url": "https://api.github.com/users/stas-sonarsource-2/followers",
              "following_url": "https://api.github.com/users/stas-sonarsource-2/following{/other_user}",
              "gists_url": "https://api.github.com/users/stas-sonarsource-2/gists{/gist_id}",
              "starred_url": "https://api.github.com/users/stas-sonarsource-2/starred{/owner}{/repo}",
              "subscriptions_url": "https://api.github.com/users/stas-sonarsource-2/subscriptions",
              "organizations_url": "https://api.github.com/users/stas-sonarsource-2/orgs",
              "repos_url": "https://api.github.com/users/stas-sonarsource-2/repos",
              "events_url": "https://api.github.com/users/stas-sonarsource-2/events{/privacy}",
              "received_events_url": "https://api.github.com/users/stas-sonarsource-2/received_events",
              "type": "Organization",
              "site_admin": false
            },
            "name": "stas-sonarsource-app-2",
            "description": "",
            "external_url": "http://localhost:9000/",
            "html_url": "https://github.com/apps/stas-sonarsource-app-2",
            "created_at": "2018-10-09T07:51:31Z",
            "updated_at": "2018-10-09T07:51:31Z"
          },
          "created_at": "2018-10-09T08:48:59Z",
          "updated_at": "2018-10-09T08:48:59Z",
          "latest_check_runs_count": 0,
          "check_runs_url": "https://api.github.com/repos/stas-vilchik/fake-js/check-suites/18070942/check-runs",
          "head_commit": {
            "id": "514fee5fe45871488cc16e9a0775b03c5266391d",
            "tree_id": "ac9506ebc6a70eb5fadf5e1b08b429f10b00e3e8",
            "message": "update foo.js",
            "timestamp": "2018-10-09T08:48:42Z",
            "author": {
              "name": "Stas Vilchik",
              "email": "stas.vilchik@sonarsource.com"
            },
            "committer": {
              "name": "Stas Vilchik",
              "email": "stas.vilchik@sonarsource.com"
            }
          }
        },
        "repository": {
          "id": 152210958,
          "node_id": "MDEwOlJlcG9zaXRvcnkxNTIyMTA5NTg=",
          "name": "fake-js",
          "full_name": "stas-vilchik/fake-js",
          "private": false,
          "owner": {
            "login": "stas-vilchik",
            "id": 2317341,
            "node_id": "MDQ6VXNlcjIzMTczNDE=",
            "avatar_url": "https://avatars0.githubusercontent.com/u/2317341?v=4",
            "gravatar_id": "",
            "url": "https://api.github.com/users/stas-vilchik",
            "html_url": "https://github.com/stas-vilchik",
            "followers_url": "https://api.github.com/users/stas-vilchik/followers",
            "following_url": "https://api.github.com/users/stas-vilchik/following{/other_user}",
            "gists_url": "https://api.github.com/users/stas-vilchik/gists{/gist_id}",
            "starred_url": "https://api.github.com/users/stas-vilchik/starred{/owner}{/repo}",
            "subscriptions_url": "https://api.github.com/users/stas-vilchik/subscriptions",
            "organizations_url": "https://api.github.com/users/stas-vilchik/orgs",
            "repos_url": "https://api.github.com/users/stas-vilchik/repos",
            "events_url": "https://api.github.com/users/stas-vilchik/events{/privacy}",
            "received_events_url": "https://api.github.com/users/stas-vilchik/received_events",
            "type": "User",
            "site_admin": false
          },
          "html_url": "https://github.com/stas-vilchik/fake-js",
          "description": null,
          "fork": false,
          "url": "https://api.github.com/repos/stas-vilchik/fake-js",
          "forks_url": "https://api.github.com/repos/stas-vilchik/fake-js/forks",
          "keys_url": "https://api.github.com/repos/stas-vilchik/fake-js/keys{/key_id}",
          "collaborators_url": "https://api.github.com/repos/stas-vilchik/fake-js/collaborators{/collaborator}",
          "teams_url": "https://api.github.com/repos/stas-vilchik/fake-js/teams",
          "hooks_url": "https://api.github.com/repos/stas-vilchik/fake-js/hooks",
          "issue_events_url": "https://api.github.com/repos/stas-vilchik/fake-js/issues/events{/number}",
          "events_url": "https://api.github.com/repos/stas-vilchik/fake-js/events",
          "assignees_url": "https://api.github.com/repos/stas-vilchik/fake-js/assignees{/user}",
          "branches_url": "https://api.github.com/repos/stas-vilchik/fake-js/branches{/branch}",
          "tags_url": "https://api.github.com/repos/stas-vilchik/fake-js/tags",
          "blobs_url": "https://api.github.com/repos/stas-vilchik/fake-js/git/blobs{/sha}",
          "git_tags_url": "https://api.github.com/repos/stas-vilchik/fake-js/git/tags{/sha}",
          "git_refs_url": "https://api.github.com/repos/stas-vilchik/fake-js/git/refs{/sha}",
          "trees_url": "https://api.github.com/repos/stas-vilchik/fake-js/git/trees{/sha}",
          "statuses_url": "https://api.github.com/repos/stas-vilchik/fake-js/statuses/{sha}",
          "languages_url": "https://api.github.com/repos/stas-vilchik/fake-js/languages",
          "stargazers_url": "https://api.github.com/repos/stas-vilchik/fake-js/stargazers",
          "contributors_url": "https://api.github.com/repos/stas-vilchik/fake-js/contributors",
          "subscribers_url": "https://api.github.com/repos/stas-vilchik/fake-js/subscribers",
          "subscription_url": "https://api.github.com/repos/stas-vilchik/fake-js/subscription",
          "commits_url": "https://api.github.com/repos/stas-vilchik/fake-js/commits{/sha}",
          "git_commits_url": "https://api.github.com/repos/stas-vilchik/fake-js/git/commits{/sha}",
          "comments_url": "https://api.github.com/repos/stas-vilchik/fake-js/comments{/number}",
          "issue_comment_url": "https://api.github.com/repos/stas-vilchik/fake-js/issues/comments{/number}",
          "contents_url": "https://api.github.com/repos/stas-vilchik/fake-js/contents/{+path}",
          "compare_url": "https://api.github.com/repos/stas-vilchik/fake-js/compare/{base}...{head}",
          "merges_url": "https://api.github.com/repos/stas-vilchik/fake-js/merges",
          "archive_url": "https://api.github.com/repos/stas-vilchik/fake-js/{archive_format}{/ref}",
          "downloads_url": "https://api.github.com/repos/stas-vilchik/fake-js/downloads",
          "issues_url": "https://api.github.com/repos/stas-vilchik/fake-js/issues{/number}",
          "pulls_url": "https://api.github.com/repos/stas-vilchik/fake-js/pulls{/number}",
          "milestones_url": "https://api.github.com/repos/stas-vilchik/fake-js/milestones{/number}",
          "notifications_url": "https://api.github.com/repos/stas-vilchik/fake-js/notifications{?since,all,participating}",
          "labels_url": "https://api.github.com/repos/stas-vilchik/fake-js/labels{/name}",
          "releases_url": "https://api.github.com/repos/stas-vilchik/fake-js/releases{/id}",
          "deployments_url": "https://api.github.com/repos/stas-vilchik/fake-js/deployments",
          "created_at": "2018-10-09T07:55:40Z",
          "updated_at": "2018-10-09T07:56:14Z",
          "pushed_at": "2018-10-09T08:48:58Z",
          "git_url": "git://github.com/stas-vilchik/fake-js.git",
          "ssh_url": "git@github.com:stas-vilchik/fake-js.git",
          "clone_url": "https://github.com/stas-vilchik/fake-js.git",
          "svn_url": "https://github.com/stas-vilchik/fake-js",
          "homepage": null,
          "size": 0,
          "stargazers_count": 0,
          "watchers_count": 0,
          "language": "JavaScript",
          "has_issues": true,
          "has_projects": true,
          "has_downloads": true,
          "has_wiki": true,
          "has_pages": false,
          "forks_count": 0,
          "mirror_url": null,
          "archived": false,
          "open_issues_count": 0,
          "license": null,
          "forks": 0,
          "open_issues": 0,
          "watchers": 0,
          "default_branch": "master"
        },
        "sender": {
          "login": "stas-vilchik",
          "id": 2317341,
          "node_id": "MDQ6VXNlcjIzMTczNDE=",
          "avatar_url": "https://avatars0.githubusercontent.com/u/2317341?v=4",
          "gravatar_id": "",
          "url": "https://api.github.com/users/stas-vilchik",
          "html_url": "https://github.com/stas-vilchik",
          "followers_url": "https://api.github.com/users/stas-vilchik/followers",
          "following_url": "https://api.github.com/users/stas-vilchik/following{/other_user}",
          "gists_url": "https://api.github.com/users/stas-vilchik/gists{/gist_id}",
          "starred_url": "https://api.github.com/users/stas-vilchik/starred{/owner}{/repo}",
          "subscriptions_url": "https://api.github.com/users/stas-vilchik/subscriptions",
          "organizations_url": "https://api.github.com/users/stas-vilchik/orgs",
          "repos_url": "https://api.github.com/users/stas-vilchik/repos",
          "events_url": "https://api.github.com/users/stas-vilchik/events{/privacy}",
          "received_events_url": "https://api.github.com/users/stas-vilchik/received_events",
          "type": "User",
          "site_admin": false
        },
        "installation": {
          "id": 373623,
          "node_id": "MDIzOkludGVncmF0aW9uSW5zdGFsbGF0aW9uMzczNjIz"
        }
      }
      

      Add check run details to the PR

      We can add several check runs (for example, one for issues, one for coverage). They are grouped into a single check suite.

      Each check run includes:

      • name (for example, sonarcloud-issues)
      • status (can be one of queued, in_progress, or completed)
      • output with title (text), summary (markdown) and annotations (each with its own title, message, code position and level)

      Example of payload to create in progress check run:

      {
          "name": "sonarcloud-issues",
          "head_sha": "{{ HEAD_SHA1  }}",
          "status": "in_progress",
          "started_at": "2018-10-09T01:14:52Z"
      }
      

      Or to create a failed check run with annotations:

      {
        "name": "sonarcloud-issues",
        "head_sha": "{{ HEAD_SHA1  }}",
        "status": "completed",
      	"conclusion": "failure",
        "started_at": "2018-10-09T01:14:52Z",
        "completed_at": "2018-10-09T01:19:52Z",
        "output": {
          "title": "SonarCloud Issues Report",
          "summary": "SonarCloud analysis found 2 bugs, 13 code smells and no vulnerabilities.",
          "text": "Some issues cannot be reported inline:\n* Code Smell: Remove this unused import 'java.time.Instant'.\n* Code Smell: Remove this unused import 'java.time.ZoneOffset'.",
          "annotations": [
            {
              "path": "src/foo.js",
              "annotation_level": "failure",
              "title": "Bug",
              "message": "This condition is always false.",
              "start_line": 4,
              "end_line": 4
            },
            {
              "path": "src/pr-file.js",
              "annotation_level": "warning",
              "title": "Code Smell",
              "message": "This variable is unused.",
              "start_line": 1,
              "end_line": 1
            }
          ]
        }
      }
      

      Each check run can be updated or re-created at any point with new status and details. The difference is that updating would keep the old annotations and re-creation would not.

      Looks like GitHub does not allow to add more than 50 annotations at once. We'd need to call the API several times if we reach the limit (see https://platform.github.community/t/checks-api-50-annotation-limit/6138).

      Note for future Annotations integration

      On end of analysis

      Since at most 50 annotations can be added at time in POST or PATCH (see https://platform.github.community/t/checks-api-50-annotation-limit/6138),
      as many as necessary updates of the Check Run should be made

      1. PATCH /repos/:owner/:repo/check-runs/:check_run_id (see https://developer.github.com/v3/checks/runs/#update-a-check-run)
        • output= (We have to provide title and summary in addition to updated fields)
          • title=[same as when creating]
          • summary=[same as when creating]
          • text=[new text from new "QG" + old or new list of issues out of diff]
          • annotations=[at most 50 extra ones]

      On Issue change in the UI

      actions if it's not possible to update the check run to remove annotations

      TO CONFIRM It is not possible to update a Check Run to remove annotations. Therefor

      Same procedure as below, except that a new Check Run should be created for each UI change.
      The previously create check run is needed only to build know the list of all issues generated during the report processing (as part of the UI update, we only know the issues changed and there is no way to compute the list of issues of a given report processing).

      actions if it's possible to update the check run to remove annotations

      If Check Run ID is not stored:

      1. get latest run for the SHA1 of the analysis
      2. do nothing if
        • empty response
        • SHA1 is not part of PR anymore (eg. check pull_requests array in the response?)

      If Check Run ID is stored attached to the analysis

      1. get that Check Run
      2. do nothing if
        • HTTP error 404
        • SHA1 is not part of PR anymore (eg. check pull_requests array in the response?)

      Retrieve the Check Run's annotations (if annotation_count > 0 in previous response)

      1. GET /repos/:owner/:repo/check-runs/:check_run_id/annotations (see https://developer.github.com/v3/checks/runs/#list-annotations-for-a-check-run)
        • can be paginated

      Compute the new annotations of the Check Run from:

      • the issue UUIDs hidden in the annotations
      • the UUIDs of issue(s) closed by the UI change

      Compute the new text of the Check Run:

      1. compute 1st part of text from new current "QG" status
      2. (OPTIONAL, unless already done today) compute a new list of issues out of diff
        • parsing the text, URL and UUID of issues out of diff from text of the existing Check Run
        • the UUIDs of issue(s) closed by the UI change

      Create a check run with status completed

      1. POST /repos/:owner/:repo/check-runs (see https://developer.github.com/v3/checks/runs/#create-a-check-run)
        • name="SonarCloud Analysis report"
        • head_sha=[sha1 of the report]
        • details_url=[URL of PR on SC]
        • external_id=[analysis UUID?]
        • status="completed"
        • conclusion=[success if QG ok/failure if QG ko]
        • completed_at=[current date]
        • output=[same as when creating]
          • title=[same as when creating]
          • summary=[same as when creating]
          • text=[\new text from new "QG" + old or new list of issues out of diff]
        • annotations=[at most 50 of the new annotations]

      Since at most 50 annotations can be added at time in POST or PATCH (see https://platform.github.community/t/checks-api-50-annotation-limit/6138),
      as many as necessary updates of the Check Run should be made

      1. PATCH /repos/:owner/:repo/check-runs/:check_run_id (see https://developer.github.com/v3/checks/runs/#update-a-check-run)
        • output= (We have to provide title and summary in addition to updated fields)
          • title=[same as when creating]
          • summary=[same as when creating]
          • text=[new text from new "QG" + old or new list of issues out of diff]
          • annotations=[at most 50 extra ones]

      Side notes

      To be checked : is the new API reliable? (ie. no more in beta version, see warning at https://developer.github.com/v3/checks/runs/)

      See what's travis is doing : https://github.com/SonarSource/iris/pull/2/checks?check_run_id=23304985
      And Cirrus: https://github.com/SonarSource/sonar-enterprise/pull/788/checks?check_run_id=22366655

      Worth reading:

        Attachments

        1. IssueSummary.jpg
          IssueSummary.jpg
          12 kB
        2. NotInDiffIssues.jpg
          NotInDiffIssues.jpg
          108 kB
        3. SummaryChecks.jpg
          SummaryChecks.jpg
          19 kB

          Issue Links

            Activity

              People

              • Assignee:
                aurelie.boiteux Aurélie Boiteux
                Reporter:
                fabrice.bellingard Fabrice Bellingard
              • Votes:
                2 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: