Show All Pipelines for Merge Requests from All merge_request_diffs

Dimitryi added Merge Request Versions: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5467

We added Pipelines for merge request which shows Pipelines only for latest version: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5485

We should verify and extend our implementation to show all past versions.

mentioned in issue #21277 (closed)

Reassigned to @godfat

I'll take over from here :)

@ayufan So it seems I don't really understand what's missing right now. I think we do show all pipelines, and they should be correct. What should we verify against? I thought we're talking about pipelines missing due to rebase (or force push in general), but versioning doesn't seem to be related to rebase?

@godfat

I think that we should pipelines only from latest Merge Request Version, but I may be wrong :)

@ayufan Oh, I see. I'll verify the code then. To my understanding I think it's not related though. We should be showing all pipelines from all commits in a MR (well, not for force pushes though), and that versioning didn't change the commits. I checked my local database and I think it's still showing everything.

@ayufan ok, as far as I could tell, it should be showing all pipelines (without pipelines which were discarded via force pushes). So no action we need to take here. The reasoning is:

• We're showing pipelines from the latest commits from latest merge_request_diff (via: has_one :merge_request_diff, -> { order('merge_request_diffs.id DESC') })
• The latest merge_request_diff should be created via a push in GitPushService, which would invoke Project#update_merge_requests which would use MergeRequests::RefreshService, which would call MergeRequest#reload_diff which would call MergeRequest#create_merge_request_diff, which would grab the commits between MergeRequest#safe_start_commit_sha and MergeRequest#head_commit_sha, which should contain all the commits we want to show.
• This makes perfect sense because we're defaulting to latest merge_request_diff, and which should contain all the changes in this merge request.

Please verify my reasoning :) (and perhaps check the codes accordingly)

Reassigned to @ayufan

I'm asking about these force pushes, since we run this pipelines in the past for this Merge Request, shouldn't we show them anyway?

@ayufan Yes, of course we want to show them, but we lose them even without versioning. Oh, or I guess you're saying that now we have versioning, so we could show all of them now? I am a bit unsure that if we just straightforwardly show them, would the ordering of pipelines being confusing? Because now we're not just showing old pipelines, we're also showing pipelines for non-existing commits, mixing together.

I am a bit unsure that if we just straightforwardly show them, would the ordering of pipelines being confusing

We would show them ordered by date.

Because now we're not just showing old pipelines

Right now we are only showing latests pipelines for latest version.

We're also showing pipelines for non-existing commits, mixing together.

Yes, but we would also know that this commit got removed from that branch, so we could show that this is a pipeline that is for older version of Merge Request.

@ayufan

We would show them ordered by date.

Presumably this means id, and yes I think we're doing that right now.

Right now we are only showing latests pipelines for latest version.

Yes, but latest version should contain everything. I believe everything is like before. Showing pipelines from commits which were already forcefully pushed is a new thing, and I think to do this we'll need to update database schema.

Yes, but we would also know that this commit got removed from that branch, so we could show that this is a pipeline that is for older version of Merge Request.

True. I guess that indication would be good enough.

Yes, but latest version should contain everything. I believe everything is like before. Showing pipelines from commits which were already forcefully pushed is a new thing, and I think to do this we'll need to update database schema.

I assumed that not since we already have all previous versions with all SHAs stored in database.

Umm, if we're walking through all the merge requests diffs, then I think we could retrieve them. Not sure if this would perform well though, I feel it could be a bit heavy but I could try.

Let's see how intensive it is :)

Changed title: Verify correctness of Pipelines for Merge Requests → Show All Pipelines for Merge Requests from All merge_request_diffs

I'll put this in lower priority for now, because I think the issue I mentioned in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5525#note_15134024 is more important and this is not a bug.

Reassigned to @godfat

Oops, I forgot to assign back.

Mentioned in merge request !6414 (merged)

Added pipeline label

Status changed to closed by merge request !6414 (merged)

Mentioned in commit 529b17d1

Mentioned in commit 33bb5ddf

mentioned in merge request !14785