Skip to content

Introduce MergeRequest#merge_base_pipeline for CodeQuality artifact comparison to merged results

drew stachon requested to merge merge-base-pipeline into master

What does this MR do?

This merge request adds the ability to reference the pipeline run on the target_sha of a merge request pipeline. The testing group is implementing this solution to attempt cleaning up our own MergeRequest widget, with an eye on making it available for the rest of the merge request to use as well when the pipelines are for merged results.

We'll use the feature flag roll out issue #263724 (closed) to track the deployment of this first for the grouptesting to investigate some merge requests on projects we know to be problematic, and then progressively to a wider audience if no problems arise, and aim to femove the flag in 13.6.

A Diagram

graph TD
  A["Commit A (master)"] --> B["Commit B (master, A+B, BASE_PIPELINE)"]
  B --> C["Commit C (master, A+B+C)"]
  B --> F["Commit F (feature, A+B+F)"]
  C --> D["Commit D (master, A+B+C+D)"]
  F --> G["Commit Z (Ephemeral merge commit, A+B+C+D+F, HEAD_PIPELINE)"]
  D --> E["Commit E (master, A+B+C+D+E)"]
  D--> G

This method exposes a pipeline run on commit D from this diagram, if it exists.

Screenshots

Currently & flag off

Screen_Shot_2020-10-15_at_4.11.23_AM

Notice the second violation Method 'drew' has 19 arguments (exceeds 4 allowed). This method was added to the target branch (master) after the merge request was opened, but remains in the merged result of this merge request.

After switching the flag on

The base_pipeline link has changed, and the artifact is fetched from a job in the more recent pipeline on master, where this violation does exist:

Screen_Shot_2020-10-15_at_4.05.37_AM

The violation no longer appears in this list, because it's in the base and no longer considered introduced.

Screen_Shot_2020-10-15_at_4.10.24_AM

Does this MR meet the acceptance criteria?

I'm specifically concerned about the test coverage of this method. The way the test is constructed and run is completely separated from the creation of pipelines, and in that way makes an assumption about what pipelines will be where and running on which commits for this to "work".

There are a few basic tests here, but the code is also currently written so that only an extra field is added, and nothing will break if the pipeline isn't found or the field happens to be blank. It's the lightest touch implementation I could think of that will still potentially give us a meaningful improvement.

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by drew stachon

Merge request reports