Skip to content
Snippets Groups Projects

Resolve "Pipelines for merge request"

Merged Annabel Gray requested to merge 18681-pipelines-merge-request into master
13 unresolved threads

What does this MR do?

Adds Pipelines tab in merge request view

What are the relevant issue numbers?

Closes #18681 (closed)

Screenshots (if relevant)

Screen_Shot_2016-08-16_at_3.22.41_PM

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Annabel Gray Added 1 commit:

    Added 1 commit:

    • 7987f183 - Display all pipelines for Merge Request
  • I'm getting a little bit lost here and not sure I'm actually grabbing the right info. What should the query be to grab all pipelines for a merge request? I used Ci::Pipeline.where(ref: @merge_request.source_branch) but I see that in merge_request.rb, pipeline is already defined and returning a single pipeline? Now I'm not so sure where to go from here. Could someone please take a look to see if I'm heading in the right direction? cc @zj @ayufan

  • Annabel Gray Added 1 commit:

    Added 1 commit:

    • 0d6d7f6e - Display all pipelines for Merge Request
  • @annabeldunstone I believe there is only one pipeline for a project, which itself has multiple environments and deploys etc etc. So in a view @merge_request.pipeline might just be what you need.

    But I just started yesterday on the CI stuff, so I could be misinterpreting the code right now. But it does make sense. :)

136 136 end
137 137 end
138 138
139 def pipelines
140 @pipelines = Ci::Pipeline.where(ref: @merge_request.source_branch)
  • I'm not sure that this will work as expected. If you open a merge request on branch some-branch, then add some commits, merge, remove source branch, open a new merge request with the same branch name, you will still see pipelines for the old merge request in the new one. I'm not sure if this is acceptable, what do you think @markpundsack?

    Not to mention that you do not use project scope here, which obviously will not work well and will introduce security vulnerability.

  • There is also another problem here, that we should keep in mind. We do not want to use pipelines from target project, because there will be none. We need pipelines from the source project.

  • So the bottom line here is that we need following information to get pipelines: source project, branch, and probably SHA of each of commit that is present in MR, which may be a little problematic in terms of implementation and performance (fortunately we store commits in MergeRequestDiff), we may also want to explore different way of getting collection of pipelines for merge request as well.

  • Thanks for the comments @grzesiek. As I mentioned yesterday, I've pretty much hit the extent of my rails knowledge. I am not sure how to fix these security vulnerabilities; it would make a lot more sense for a backend dev to pick this up. If there's frontend work to be done after that, please let me know!

  • @grzesiek Maybe something like this: But yeah, we should probably add a foreign key from pipelines to merge request, otherwise different MR could be getting the same pipelines!

    @annabeldunstone I think we need to decide what to do before implementing, but I guess you could just assume that @pipelines would get the proper data before we have a proper implementation. Or I guess it's already done?

    diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
    index acdb5ba..d6128b3 100644
    --- a/app/controllers/projects/merge_requests_controller.rb
    +++ b/app/controllers/projects/merge_requests_controller.rb
    @@ -137,7 +137,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       end
     
       def pipelines
    -    @pipelines = Ci::Pipeline.where(ref: @merge_request.source_branch)
    +    @pipelines = @merge_request.all_pipelines
     
         respond_to do |format|
           format.html do
    diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
    index 471e32f..44d0ab4 100644
    --- a/app/models/merge_request.rb
    +++ b/app/models/merge_request.rb
    @@ -643,7 +643,14 @@ class MergeRequest < ActiveRecord::Base
       end
     
       def pipeline
    -    @pipeline ||= source_project.pipeline(diff_head_sha, source_branch) if diff_head_sha && source_project
    +    @pipeline ||= all_pipelines.first
    +  end
    +
    +  def all_pipelines
    +    @all_pipelines ||= if diff_head_sha && source_project
    +                         source_project.pipelines.order(id: :desc).
    +                           where(sha: commits.map(&:sha), ref: source_branch)
    +                       end
       end
     
       def merge_commit
  • So the bottom line here is that we need following information to get pipelines: source project, branch, and probably SHA of each of commit that is present in MR, which may be a little problematic in terms of implementation and performance (fortunately we store commits in MergeRequestDiff), we may also want to explore different way of getting collection of pipelines for merge request as well.

    It would be a shame if it comes down to this, but the result is exactly what the user would want. Well, someone that rebases might still lose pipelines that once ran for a given MR but no longer apply because the SHA no longer exists in the diff. But outside of that, this covers it. I just wonder if there is some smaller MVP that would get most of the value with less complication? The only thing I can think of is to show only the last pipeline run, which we already have, or perhaps pipelines for the exact SHA of the latest commit in the MR.

    Then again, maybe traversing the diff for commits isn't that bad. Is it something that could be done in a single SQL query?

    At any rate, perhaps @godfat is right, and @annabeldunstone should go forward assuming @pipelines exists.

  • @godfat Do you want to work on that with @annabeldunstone? I think that @godfat's solutions is good, I would probably leave def pipeline untouched. We have to do some benchmarks on that, because using commits method, that may touch repository, may have some performance implications. For now, I don't see better approach than using commits from diffs.

  • @grzesiek Sure. You're right about leaving def pipeline alone. On the other hand, don't we really consider about adding a new column for this? Matching SHA might be tricky as @markpundsack pointed out about force push.

  • @godfat What column do you propose other than the one related to Commit (#15619 (moved))?

    Edited by Grzegorz Bizon
  • @grzesiek I am not very sure if they're related from a quick glimpse. What I am thinking is adding a ci_commits.merge_request_id (pipelines) pointing to merge_requests.id so that we could just call merge_request.pipelines here because we could have MergeRequest.has_many :pipelines

  • @godfat I wonder if pipeline can have more then one merge request.

  • @godfat But making connection between pipeline and merge request on database level can have a lot of sense I think. What do you think @ayufan @tmaczukin? But I would rather do that in the separate table in database, but it need some deeper thoughts.

    Edited by Grzegorz Bizon
  • @grzesiek From my understanding, pipelines are created either from the web page, and in this case they're not associated with any merge requests, only a particular branch or a particular tag. Or they are created from push events, and in this case they could still have no corresponding merge request, or a merge request for a particular branch. I think since we don't allow people to create a merge request for the same branch if there's already one active, a pipeline could not be associated with two merge requests -- it's always associated to the only active one.

    Edited by username-removed-423915
  • If we would possibly allow having two active merge requests for the same branch, then I think it would be a many-to-many relation. But I doubt if we should do that because it's confusing, and I can't think of a use case for that.

  • Currently we don't track that specific Pipeline were created for a merge request. We don't know that. Ideally we should create a separate pipeline just for Merge Requests since it makes sense to have different set of jobs to be executed on Merge Requests as stated here (https://gitlab.com/gitlab-org/gitlab-ce/issues/15310).

    However, we are still not there. We only plan to work on that, but it's not yet possible.

    We will implement that by adding a column to Pipeline to link it to Merge Request. Currently it is not easily achievable, because we are creating a Pipeline on git push hook. In typical workflow the Merge Request is created after executing the hook.

    My take on this is to go with @godfat proposal. It will not work for git push --force. When we make a first class support for Pipelines on Merge Requests we will modify that part to show all pipelines that were ever created for this merge request.

    Edited by Kamil Trzcińśki
  • @grzesiek

    Following your note: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5485/diffs#note_13384092

    It's important that we load pipelines lazily. So it doesn't have a big implications on performance fortunately.

  • I'll finish the Backend part of this Merge Request following a @godfat proposal here.

    Edited by Kamil Trzcińśki
  • Please register or sign in to reply
  • mentioned in issue #19982 (closed)

  • mentioned in issue #20616 (moved)

  • Reassigned to @ayufan

  • Kamil Trzcińśki Added 2 commits:

    Added 2 commits:

    • cae0fa7c - Properly select a list of Pipelines for a Merge Requests
    • aa8cb00b - Add a feature tests to check if a view can be rendered properly
  • @annabeldunstone

    I updated a MR with a proper list of pipelines that should be shown. I also added a test to validate model methods and a feature test to verify that a feature can be used and a pipelines are shown.

  • 642 642 diverged_commits_count > 0
    643 643 end
    644 644
    645 def commits_sha
    646 commits.map(&:sha)
    647 end
    648
    645 649 def pipeline
    • Should we change this method name to latest_pipeline and use pipelines instead of all_pipelines? I think that formed method needs prefix more than later since we plan to introduce pipelines for merge requests.

    • Not sure if it makes sense. It would make sense if we were to change also: project.pipeline(ref) where we ten to use a pipeline instead of latest_pipeline. I'm kind of OK with using a verb all_pipelines, it's explicit enough.

    • Please register or sign in to reply
  • 642 642 diverged_commits_count > 0
    643 643 end
    644 644
    645 def commits_sha
    646 commits.map(&:sha)
    647 end
    648
  • 53 53 Commits
    54 54 %span.badge= @commits_count
    55 55 - if @pipeline
    56 %li.pipelines-tab
    57 = link_to pipelines_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: '#pipelines', action: 'pipelines', toggle: 'tab'} do
  • 12 login_as user
    13 end
    14
    15 context 'with pipelines' do
    16 let!(:pipeline) do
    17 create(:ci_empty_pipeline,
    18 project: merge_request.source_project,
    19 ref: merge_request.source_branch,
    20 sha: merge_request.diff_head_sha)
    21 end
    22
    23 before do
    24 visit namespace_project_merge_request_path(project.namespace, project, merge_request)
    25 end
    26
    27 scenario 'does click a pipeline tab and sees a list of pipelines' do
  • 15 context 'with pipelines' do
    16 let!(:pipeline) do
    17 create(:ci_empty_pipeline,
    18 project: merge_request.source_project,
    19 ref: merge_request.source_branch,
    20 sha: merge_request.diff_head_sha)
    21 end
    22
    23 before do
    24 visit namespace_project_merge_request_path(project.namespace, project, merge_request)
    25 end
    26
    27 scenario 'does click a pipeline tab and sees a list of pipelines' do
    28 page.within('.merge-request-tabs') do
    29 click_link('Pipelines')
    30 end
  • 27 scenario 'does click a pipeline tab and sees a list of pipelines' do
    28 page.within('.merge-request-tabs') do
    29 click_link('Pipelines')
    30 end
    31 wait_for_ajax
    32
    33 expect(page).to have_selector('.pipeline-actions')
    34 end
    35 end
    36
    37 context 'without pipelines' do
    38 before do
    39 visit namespace_project_merge_request_path(project.namespace, project, merge_request)
    40 end
    41
    42 scenario 'does not find a pipeline link' do
  • 29 click_link('Pipelines')
    30 end
    31 wait_for_ajax
    32
    33 expect(page).to have_selector('.pipeline-actions')
    34 end
    35 end
    36
    37 context 'without pipelines' do
    38 before do
    39 visit namespace_project_merge_request_path(project.namespace, project, merge_request)
    40 end
    41
    42 scenario 'does not find a pipeline link' do
    43 page.within('.merge-request-tabs') do
    44 expect(page).not_to have_link('Pipelines')
  • 419 419 subject { create :merge_request, :simple }
    420 420 end
    421 421
    422 describe '#commits_sha' do
    423 let(:commit0) { double('commit0', sha: 'sha1') }
    424 let(:commit1) { double('commit1', sha: 'sha2') }
    425 let(:commit2) { double('commit2', sha: 'sha3') }
    426
    427 before do
    428 allow(subject).to receive(:commits).and_return([commit0, commit1, commit2])
    429 end
    430
    431 it 'returns sha of commits' do
    432 expect(subject.commits_sha).to contain_exactly('sha1', 'sha2', 'sha3')
    433 end
    434 end
  • 443 457 end
    444 458 end
    445 459
    460 describe '#all_pipelines' do
    461 let(:commit0) { double('commit0', sha: 'sha1') }
    462 let(:commit1) { double('commit1', sha: 'sha2') }
    463 let(:commit2) { double('commit2', sha: 'sha3') }
    464 let!(:pipeline) { create(:ci_empty_pipeline, project: subject.source_project, sha: 'sha1', ref: subject.source_branch) }
    465 let!(:pipeline2) { create(:ci_empty_pipeline, project: subject.source_project, sha: 'sha1', ref: subject.source_branch) }
    466 let!(:pipeline3) { create(:ci_empty_pipeline, project: subject.source_project, sha: 'sha2', ref: subject.source_branch) }
    467 let!(:pipeline4) { create(:ci_empty_pipeline, project: subject.target_project, sha: 'sha1', ref: subject.target_branch) }
    468
    469 before do
    470 allow(subject).to receive(:commits).and_return([commit0, commit1, commit2])
    471 end
  • We need to put this on hold before https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5467 is merged. There are some important changes in how MergeRequestDiff collects commits and changes, that can affect this merge request.

  • As discussed with @ayufan - since !5467 (merged) is scheduled for 8.12 and we are not completely sure what will change there (we will need to take a look now, though), we should improve specs in this merge request, and carry on with currently implementation.

  • Added 1 commit:

    • db3c5a04 - Use merge_request_diff instead of doubles for testing pipelines for Merge Requests
  • Kamil Trzcińśki Added 931 commits:

    Added 931 commits:

    • db3c5a04...5a4ecb98 - 929 commits from branch master
    • a2bf638e - Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ce into 18681-pipelines-merge-request
    • bcb3937d - Update fixtures to make development testing easier
  • Annabel Gray Added 1 commit:

    Added 1 commit:

    • f0d6b1ff - Hide branch name and status text on mr pipelines; don't use shorter timeago
  • Annabel Gray Added 28 commits:

    Added 28 commits:

    • f0d6b1ff...f7e08ad0 - 25 commits from branch master
    • f9883dee - Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ce into 18681-pipelines-merge-request
    • dc42d52a - Add pipelines badge to MR tab
    • 30d578f6 - Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ce into 18681-pipelines-merge-request
  • Annabel Gray Unmarked this merge request as a Work In Progress

    Unmarked this merge request as a Work In Progress

  • @grzesiek can you review?

  • Reassigned to @jschatz1

  • 177 182 });
    178 183 };
    179 184
    185 MergeRequestTabs.prototype.loadPipelines = function(source) {
    186 if (this.pipelinesLoaded) {
    187 return;
    188 }
    189 return this._get({
    190 url: source + ".json",
    191 success: (function(_this) {
    192 return function(data) {
    193 document.querySelector("div#pipelines").innerHTML = data.html;
  • 177 182 });
    178 183 };
    179 184
    185 MergeRequestTabs.prototype.loadPipelines = function(source) {
    186 if (this.pipelinesLoaded) {
    187 return;
    188 }
    189 return this._get({
  • 177 182 });
    178 183 };
    179 184
    185 MergeRequestTabs.prototype.loadPipelines = function(source) {
    186 if (this.pipelinesLoaded) {
    187 return;
    188 }
    189 return this._get({
    190 url: source + ".json",
    191 success: (function(_this) {
  • @annabeldunstone impressive. Nice work.

  • Annabel Gray Added 1 commit:

    Added 1 commit:

    • e5f9fdf9 - Refactor merge_request-tabs
  • Reassigned to @jschatz1

  • Reassigned to @ayufan

  • Reassigned to @grzesiek

  • Annabel Gray Added ~149423 label

    Added ~149423 label

  • Reassigned to @ayufan

  • Reassigned to @grzesiek

  • Added 1 commit:

    • 62e2989b - Use have_no_link to test presence of button
  • Reassigned to @jschatz1

  • Jacob Schatz Status changed to merged

    Status changed to merged

  • Jacob Schatz mentioned in commit eefb2582

    mentioned in commit eefb2582

  • @annabeldunstone @ayufan I think would be great to have a CHANGELOG entry for this feature, I'm going to pick it into stable in the meantime, please send a new MR with the CHANGELOG entry if required.

  • Picked into 8-11-stable, will go into 8.11.0-rc3.

  • Rubén Dávila Removed ~149423 label

    Removed ~149423 label

  • Jacob Schatz mentioned in commit eaf8ae2e

    mentioned in commit eaf8ae2e

  • Annabel Gray mentioned in merge request !5854 (merged)

    mentioned in merge request !5854 (merged)

  • Jacob Schatz Added ~149423 label

    Added ~149423 label

  • Rubén Dávila Removed ~149423 label

    Removed ~149423 label

  • @jschatz1 this was already picked into stable, please let me know if something is missing, I'm going to merge the MR related to the CHANGELOG entry.

  • Rubén Dávila mentioned in commit 9e7231bd

    mentioned in commit 9e7231bd

  • Rubén Dávila mentioned in commit e2cc5b98

    mentioned in commit e2cc5b98

  • mentioned in issue #21889 (closed)

  • Please register or sign in to reply
    Loading