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

Make pull request a 1st Class Citizen

    XMLWordPrintable

    Details

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

      Description

      Why

      Currently, analyzing a PR calls for a mix of sonar.branch.* and sonar.[provider].* properties, and updates the short-lived branch on which the PR was based. In fact, on the SonarQube side, a PR is currently indistinguishable from a short-lived branch, but they are really different kinds of things. For instance, multiple PRs can be opened from a single feature branch, and PRs have a different life cycle than branches.

      For these reasons, PRs must be 1st Class citizens in SonarQube.

      What

      It is important to note up front that a PR must be analyzable whether or not its short-lived origin branch exists in SonarQube. In fact, our assumption is that we will quickly move internally to a workflow in which most short-lived branches are not present in SQ. This design choice has no impact on the analysis of short-lived branches.

      These things are needed to distinguish PRs as 1st class citizens:

      • new icon
      • slightly different UI. Based on the short-lived branch UI, but with additional/different information
      • a different analysis property "domain"
      • unique properties
      • a different treatment in the branch dropdown

      UI Differences

      In addition to using the PR icon, the header of a PR should present the following pieces of information if available:

      • Merge target (which may or may not be the PR's origin branch)
      • Origin branch name
      • PR title

      Property Namespace

      We'll use sonar.pullrequest.blah.
      These properties will not be provider-specific. So, sonar.pullrequest.id rather than sonar.pullrequest.github.id. (This implies the need to explicitly specify provider.)

      Unique properties

      We'll use the following properties to gather the requisite information:

      • Already done as part of previous hardening sprint (so here just for completeness):
        • sonar.pullrequest.key required - the provider's id for the PR
          • Necessarily passed by the scanner
      • To be done as part of this MMF:
        • sonar.pullrequest.github.token.secured - the authentication token for updating a PR on the repository
          • Must be specified in the Web UI of the project (for security reasons)
        • sonar.pullrequest.provider required - Default: GitHub.
          • Can be specified in the Web UI of the project
        • sonar.pullrequest.github.repository - set at the project level.
          • This is the provider's ID for the repository, e.g. SonarSource/sonarqube
        • sonar.pullrequest.base - Default: master. The name of the SonarQube long-lived branch into which this PR will (might be) merged. This must be specified rather than being harvested from GitHub because there's not necessarily a relationship between the name of the branch in GitHub and on SonarQube.
          • Necessarily passed by the scanner
        • sonar.pullrequest.branch required - the short-lived SonarQube branch on which a PR is based. This may or may not be analyzed in SQ, so we cannot require or make a hard link between the two (so, no use of ids or repo URLs). Instead, we give the user the option to make a soft link by providing the title of the base branch, which we'll display
          • Necessarily passed by the scanner

      We need URL as well, but that's easily calculated for GitHub, our first target.

      Note that many of these properties would be provided implicitly by a truly sophisticated integration. As a baby step, we will gather them explicitly.
      Several properties could be set at analysis time: sonar.pullrequest.provider, sonar.pullrequest.github.repository. As a baby step, we will gather them at project level only. Technical note: similarly to file exclusions, it could be overridden at analysis time. It'd need to be stored in relation to a branch (ex: project_branches database table.).

      Status of issues when merged to the base branch

      When we detect that a new issue on a long lived branch "comes" from an existing one on a PR, we retain the state of that one located on the PR. If the issue is found on both a PR and a short-lived branch, we retain the state of the one found on the PR.

      Behavior when things change on GitHub

      If the name of a PR or its base branch gets changed in GitHub, as a user I expect to see those changes reflected in SonarCloud. Applying those changes when a build occurs is good enough.

      When a PR gets closed or reopened, SonarCloud does not need to do something on its own as part of this MMF. This will be the responsibility of a further integration (other MMF) to make sure that SonarCloud gets notified of such changes (through webhooks) and then acts accordingly.

      Other questions

      • If a PR references another PR or short-living branch, use as the base of that branch (which will be always a long branch)
      • If a PR references a non-existent branch, then fallback on the default branch as the base

      How

      As part of this work, web services that return components and component-related data must naturally be updated to work for pull requests as well.

      The branches dropdown displays PRs under the Long-Lived Branch they're supposed to merge into. They are clearly separated from Short-Lived Branches with a subheading.

      When selected, text next to the dropdown selector indicates both origin and target.

        Attachments

        1. 01_Pull-Requests_V2.png
          01_Pull-Requests_V2.png
          205 kB
        2. 01_Pull-Requests-01.png
          01_Pull-Requests-01.png
          201 kB
        3. 01_Pull-Requests-02.png
          01_Pull-Requests-02.png
          170 kB
        4. screenshot-1.png
          screenshot-1.png
          70 kB

          Issue Links

            Activity

              People

              Assignee:
              fabrice.bellingard Fabrice Bellingard
              Reporter:
              ann.campbell.2 Ann Campbell
              Votes:
              0 Vote for this issue
              Watchers:
              13 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: