Resolve "Pipelines for merge request"
What does this MR do?
Adds Pipelines
tab in merge request view
What are the relevant issue numbers?
Closes #18681 (closed)
Screenshots (if relevant)
Merge request reports
Activity
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 inmerge_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 @ayufanAdded 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.
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 usingcommits
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 tomerge_requests.id
so that we could just callmerge_request.pipelines
here because we could haveMergeRequest.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-423915Currently 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 toMerge Request
. Currently it is not easily achievable, because we are creating a Pipeline ongit 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ńśkiFollowing 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
mentioned in issue #19982 (closed)
mentioned in issue #20616 (moved)
Reassigned to @ayufan
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.
Reassigned to @annabeldunstone
cc @grzesiek
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 642 642 diverged_commits_count > 0 643 643 end 644 644 645 def commits_sha 646 commits.map(&:sha) 647 end 648 I agree that this is the most simple change, but this can have quite important impact on performance. Maybe it is worth to ask @yorickpeterse about opinion here?
@grzesiek I'm not aware of a better way of just getting the SHA1s from a merge request. Since commit data is cached in merge requests diffs I think this should be OK.
Thanks @yorickpeterse for taking a look!
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 Does it make sense to use one-liner here? This wont exceed 80 characters I think. Otherwise we should add an empty line after block.
Edited by Grzegorz Bizon
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') Strange, it should be there.
Did you look at latest changes? https://gitlab.com/gitlab-org/gitlab-ce/commit/db3c5a042d2cd03fb64d7f85b130d065dce3eb42
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 I still don't think we tested it well enough. Can we use commits we know we have in merge request, since those are fabricated by the factory (I think we have stubs there, but it is still better than not testing it at all).
Edited by Grzegorz BizonDid you look at latest changes? https://gitlab.com/gitlab-org/gitlab-ce/commit/db3c5a042d2cd03fb64d7f85b130d065dce3eb42
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 Did you look at latest changes? https://gitlab.com/gitlab-org/gitlab-ce/commit/db3c5a042d2cd03fb64d7f85b130d065dce3eb42
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
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
-
db3c5a04...5a4ecb98 - 929 commits from branch
Added 1 commit:
- f0d6b1ff - Hide branch name and status text on mr pipelines; don't use shorter timeago
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
Toggle commit list-
f0d6b1ff...f7e08ad0 - 25 commits from branch
@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) { Instead of
success: (function(_this{ return function(data){ // code goes here. } }(this));
Edited by Jacob Schatz
Reassigned to @annabeldunstone
@annabeldunstone impressive. Nice work.
Added 1 commit:
- e5f9fdf9 - Refactor merge_request-tabs
Reassigned to @jschatz1
Reassigned to @ayufan
Reassigned to @grzesiek
Reassigned to @ayufan
Reassigned to @grzesiek
Added 1 commit:
- 62e2989b - Use have_no_link to test presence of button
Reassigned to @jschatz1
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.
mentioned in commit eaf8ae2e
mentioned in merge request !5854 (merged)
@rdavila you're right. Opened MR here: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5854
@annabeldunstone thanks!
@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.
mentioned in commit 9e7231bd
mentioned in merge request gitlab-com/www-gitlab-com!2910 (merged)
mentioned in commit e2cc5b98
mentioned in issue #21889 (closed)
Mentioned in commit pfjason/gitlab-ce@e2cc5b98