Show indicator to Pipelines for merge train

What does this MR do?

Updates the text of the pipeline widget on a merge request's page.

This new text simplifies the current implementation (which has an unwieldy number of v-if in the Vue template) and also better indicates the pipeline's purpose.

Current implementation

(The current implementation has many possible permutations; only one scenario is shown in the screenshot above.)

UX proposal:

Note: the conditions will be evaluated in the order specified below.

Pipeline type	Pipeline texts in MR	Evaluation in FE
Pipelines for merge train	Merge train pipeline # 123 pending for adcd1234	pipeline.flags.merge_train_pipeline === true
Pipelines for merged results	Merged result pipeline # 123 pending for adcd1234	pipeline.flags.merge_request_pipeline === true
Pipelines for merge requests (detached)	Detached merge request pipeline # 123 pending for adcd1234	pipeline.flags.detached_merge_request_pipeline === true
Branch pipelines	Pipeline # 123 pending for adcd1234 on feature-branch	pipeline.ref.branch === true
Tag pipelines	Pipeline # 123 pending for adcd12341	pipeline.ref.tag === true

¹It is not currently possible to create a merge request for a tag, so this scenario is N/A at the moment. However, this feature has been proposed, so this merge request attempts to handle this possibility gracefully.

Screenshots

Merge train pipeline:

Merged result pipeline:

Detached merge request pipeline:

Branch pipeline:

Tag pipeline: N/A (tag pipelines are not yet implemented by GitLab)

Related issues

• https://gitlab.com/gitlab-org/gitlab-ee/issues/11222

This MR has a CE backport

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32025

changed milestone to %12.1

added Category:Continuous Delivery Category:Continuous Integration Deliverable DogfoodingRebuild in GitLab GitLab Premium Product Vision FY20 UX backend customer devopsrelease [DEPRECATED] direction frontend internal customer maturitylovable typefeature + 1 deleted label

@nfriend Can you take a look at the FE implementation?

@cstasik @rverissimo @nfriend This piece of functionalities might not be able to be shipped as 12.1 because it's quite late to start working on. However, given this indicator is very helpful for demonstration, let's try to finish by 12.1 for now.

assigned to @nfriend

changed title from Show indicator for Pipeline for merge train to Show indicator to Pipelines for merge train

changed the description

Locally confirmed this is functional.

	1 Warning
	This merge request changed files with disabled eslint rules. Please consider fixing them.

Disabled eslint rules

The following files have disabled eslint rules. Please consider fixing them:

• app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.vue

Run the following command for more details

node_modules/.bin/eslint --report-unused-disable-directives --no-inline-config \
  'app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.vue'

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.

Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.

Category	Reviewer	Maintainer
frontend	Mark Florian (@markrian)	Kushal Pandya (@kushalpandya)
backend	Oswaldo Ferreira (@oswaldo)	Ash McKenzie (@ashmckenzie)
test for spec/features/*	No reviewer available	No maintainer available

Generated by Danger

added 1 deleted label

Labeled ~"Pick into 12.1" as this change should be included into the 12.1 package. The demonstration video has included this change https://www.youtube.com/watch?v=D4qCqXgZkHQ.

@rverissimo This particular UI is super complex - there's lots of conditions that change what text is displayed. To help me wrap my head around it, I put together a quick flowchart:

With this info in mind, I just want to confirm that the mockup in the description is still accurate? I have a feeling this message should include some of the other information shown above, (i.e., the commit SHA, abcd1234), but I could be wrong.

/cc @dosuken123

changed the description

@dosuken123 Since the new proposal is a much bigger change, do you think it still makes sense to ~"Pick into 12.1"? Or should we just move this to %12.2?

resolved all threads

changed milestone to %12.2

added missed:12.1 label

mentioned in issue #13045 (closed)

added Enterprise Edition label

@rverissimo What do you think of changing the text of "pipelines for merged results" from this:

Pipeline type	Pipeline texts in MR	Evaluation in FE
Pipelines for merged results	Merge request pipeline # 123 pending for adcd1234	flags.merge_request_pipeline == true

to this?

Pipeline type	Pipeline texts in MR	Evaluation in FE
Pipelines for merged results	Merged result pipeline # 123 pending for adcd1234	flags.merge_request_pipeline == true

I think this might be more clear and differentiate a pipeline for a merged result from a pipeline for a merge request.

/cc @dosuken123

@dosuken123 Is there ever a time when a pipeline will not have commit info associated with it? This check in the existing code seems to imply this is possible:

https://gitlab.com/gitlab-org/gitlab-ee/blob/288e38acfdac4099aa874349c1859751288fe579/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_pipeline.vue#L71

hasCommitInfo() {
  return this.pipeline.commit && Object.keys(this.pipeline.commit).length > 0;
},

but I can't think of a scenario in which this info wouldn't be available.

If we don't think this will ever happen, I might remove this guard to simplify the code.

added 3886 commits

• 0a655467...ee0c9b87 - 3885 commits from branch master
• d173e0f5 - WIP [skip ci]

Compare with previous version

marked as a Work In Progress from d173e0f5

resolved all threads

changed the description

added 266 commits

• d173e0f5...409d3764 - 265 commits from branch master
• 2e2b6ede - WIP [skip ci]

Compare with previous version

@dosuken123 I reworked some of the logic for each scenario and updated the table in the description - can you have a look and let me know if this looks correct?

For reference, your original proposal can be found in this comment: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/14664#note_194783427

mentioned in issue #13631 (closed)

mentioned in issue #13643 (closed)

resolved all threads

added 126 commits

• 2e2b6ede...5bb340bf - 125 commits from branch master
• 3de3d10b - WIP: testing pipeline

Compare with previous version

unmarked as a Work In Progress

marked as a Work In Progress

added 171 commits

• 3de3d10b...dc049f17 - 169 commits from branch master
• c4c8b624 - Make MR pipeline widget text more descriptive (EE)
• 626ef9ef - Make MR pipeline widget text more descriptive (CE)

Compare with previous version

unmarked as a Work In Progress

changed the description

CE backport: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32025

added 144 commits

• 626ef9ef...a9bee91d - 142 commits from branch master
• 314500e0 - Make MR pipeline widget text more descriptive (EE)
• 332798c4 - Make MR pipeline widget text more descriptive (CE)

Compare with previous version

added 2 commits

• d17df191 - Make MR pipeline widget text more descriptive (EE)
• f40bd42b - Make MR pipeline widget text more descriptive (CE)

Compare with previous version

changed the description

@rverissimo Can you do a UX review on this change?

I've updated the description of this MR with screenshots for each scenario. If you'd like to reproduce this locally or in a review app, let me know, and I can give you steps (although I'll warn you it's a decent amount of work to reproduce each scenario).

assigned to @rverissimo

@splattael You've been Reviewer Roulette'd; care to take a look at the backend portions of this MR?

assigned to @splattael

@nkipling You've been chosen by Reviewer Roulette as a frontend reviewer for this change; care to take a look?

assigned to @nkipling

@dosuken123 All of the scenarios listed in this MR's description are covered by RSpec tests except for merge train pipelines. Do you know where would be the best place to add a test for this case?

For reference, this is the kind of test I want: https://gitlab.com/gitlab-org/gitlab-ee/blob/f40bd42b087e4dfe73099a19c0feecc7292a5a95/spec/features/merge_request/user_sees_merge_widget_spec.rb#L195

it 'shows head pipeline information' do
  within '.ci-widget-content' do
    expect(page).to have_content("Detached merge request pipeline ##{pipeline.id} pending for #{pipeline.short_sha}")
  end
end

added 2 commits

• e4fb4489 - Make MR pipeline widget text more descriptive (EE)
• c4f8c076 - Make MR pipeline widget text more descriptive (CE)

Compare with previous version

unassigned @rverissimo

@nfriend Looks good to me, just one minor comment because I secretly have a thing for switch statements.

unassigned @nkipling

unassigned @splattael

assigned to @dosuken123

mentioned in issue #13799 (closed)

changed milestone to %12.3

added missed:12.2 label

mentioned in issue #13991 (closed)

added 671 commits

• c4f8c076...bca3a0f4 - 668 commits from branch master
• e48c344a - Make MR pipeline widget text more descriptive (EE)
• 6baf9a98 - Make MR pipeline widget text more descriptive (CE)
• 45a1844a - Pulling pipeline type calculation into backend

Compare with previous version

added 1 commit

• 8ee82c2a - Pulling pipeline type calculation into backend

Compare with previous version

@nfriend I worked a bit in this branch and quickly realized that this MR was getting bigger and bigger. Thus, I decoupled BE change to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32323 for keeping this MR as small as possible.

I'll try to merge it asap. With the BE change, FE no longer needs to calculate pipeline type or pipeline type label anymore. Please refer pipeline.type and pipeline.details.type_label keys instead. So until then, this MR can be considered as blocked state.

To make FE implementation aligns with new BE change, I tweaked a bit FE code but I probably made a mistake. I'd appreciate if you'd fix it if any.

@nfriend https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32323 has been merged, thus this MR should be unblocked now. Thank you for your patience.

After I had a discussion with BE maintainers, the final schema is a bit different from the original proposal. Please have a look at the MR description https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32323. We don't have the centralized parameter pipeline.type though, the current schema should be enough for resolving our problem to solve. Please let me know if anything different from your expectation.

added 1140 commits

• 8ee82c2a...37a2ce2f - 1138 commits from branch master
• 7e0f7abf - Make MR pipeline widget text more descriptive (EE)
• 5dde126e - Make MR pipeline widget text more descriptive (CE)

Compare with previous version

added 1 commit

• 1186b476 - Add feature spec and fix backend logic

Compare with previous version

@tkuah This MR contains a very small BE changes. Would you mind reviewing and signs-off if no objections? Thanks.

assigned to @tkuah and unassigned @dosuken123

approved this merge request

@dosuken123 LGTM - CE backport has diverged from this MR so causes ee-specific-lines-check to fail - https://gitlab.com/gitlab-org/gitlab-ee/-/jobs/288154635. Can you please address this ?

assigned to @dosuken123

unassigned @tkuah

I'll rebase both this MR and the CE backport on master to resolve the ee-specific-lines-check failure

added 394 commits

• 1186b476...8ad69eaa - 391 commits from branch master
• c3669dc2 - Make MR pipeline widget text more descriptive (EE)
• 338d7bf4 - Make MR pipeline widget text more descriptive (CE)
• 541a5a79 - Add feature spec and fix backend logic

Compare with previous version

@nfriend Thank you for backporting! It seems there are nothing I have to do in this MR anymore. I'll unassign myself, but please let me know if you need my hand.

unassigned @dosuken123

added 1 commit

• 612c5a0d - Add feature spec and fix backend logic

Compare with previous version

approved this merge request

@kushalpandya Can you review/merge as frontend maintainer? Thanks.

assigned to @kushalpandya and unassigned @nfriend

Thanks @nfriend, left a few suggestions.

unassigned @kushalpandya

assigned to @nfriend

added 377 commits

• 612c5a0d...6cf60b77 - 375 commits from branch master
• ff361258 - Make MR pipeline widget text more descriptive (EE)
• 5a0b9823 - Make MR pipeline widget text more descriptive (CE)

Compare with previous version

added 127 commits

• 5a0b9823...4a57c020 - 125 commits from branch master
• 66aae5f1 - Make MR pipeline widget text more descriptive (EE)
• 4ab6c4e1 - Make MR pipeline widget text more descriptive (CE)

Compare with previous version

approved this merge request

Thanks for the review, @kushalpandya! I've made a few updates based on your suggestions - care to take another look?

assigned to @kushalpandya

approved this merge request

Thanks @nfriend, frontend is good to go. Feel free to assign to backend maintainer for final review and merge.

unassigned @kushalpandya

assigned to @kushalpandya and @tkuah

resolved all threads

merged

mentioned in commit 0b28bd79

Merged! Thanks @nfriend @dosuken123 !

mentioned in merge request !18250 (merged)

removed 1 deleted label