From b6ec12ceca58b12d974d46d0579742f4d3cdb9d7 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 22 Jan 2020 21:08:48 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .../behaviors/markdown/constants.js | 3 + .../behaviors/markdown/marks/math.js | 3 +- .../behaviors/markdown/nodes/audio.js | 54 +-------- .../behaviors/markdown/nodes/image.js | 3 +- .../markdown/nodes/ordered_task_list.js | 3 +- .../behaviors/markdown/nodes/playable.js | 73 ++++++++++++ .../behaviors/markdown/nodes/reference.js | 3 +- .../markdown/nodes/table_header_row.js | 3 +- .../markdown/nodes/table_of_contents.js | 5 +- .../behaviors/markdown/nodes/task_list.js | 3 +- .../markdown/nodes/task_list_item.js | 3 +- .../behaviors/markdown/nodes/video.js | 56 +-------- app/helpers/system_note_helper.rb | 1 + app/models/merge_request.rb | 3 + app/models/system_note_metadata.rb | 2 +- app/services/commits/cherry_pick_service.rb | 19 ++- .../link_merge_requests_service.rb | 9 ++ .../system_notes/merge_requests_service.rb | 11 ++ .../bvl-remove-request-deadline-ff.yml | 5 + doc/integration/saml.md | 7 ++ lib/gitlab/request_context.rb | 1 - spec/factories/notes.rb | 5 + spec/lib/gitlab/request_context_spec.rb | 12 -- spec/models/merge_request_spec.rb | 10 ++ spec/requests/api/deployments_spec.rb | 63 +++++++++- .../commits/cherry_pick_service_spec.rb | 85 ++++++++++++++ .../link_merge_requests_service_spec.rb | 109 ++++++++++++++++-- .../merge_requests_service_spec.rb | 21 ++++ spec/support/helpers/graphql_helpers.rb | 5 + 29 files changed, 446 insertions(+), 134 deletions(-) create mode 100644 app/assets/javascripts/behaviors/markdown/constants.js create mode 100644 app/assets/javascripts/behaviors/markdown/nodes/playable.js create mode 100644 changelogs/unreleased/bvl-remove-request-deadline-ff.yml create mode 100644 spec/services/commits/cherry_pick_service_spec.rb diff --git a/app/assets/javascripts/behaviors/markdown/constants.js b/app/assets/javascripts/behaviors/markdown/constants.js new file mode 100644 index 00000000000..b4545d6c6c6 --- /dev/null +++ b/app/assets/javascripts/behaviors/markdown/constants.js @@ -0,0 +1,3 @@ +// https://prosemirror.net/docs/ref/#model.ParseRule.priority +export const DEFAULT_PARSE_RULE_PRIORITY = 50; +export const HIGHER_PARSE_RULE_PRIORITY = 1 + DEFAULT_PARSE_RULE_PRIORITY; diff --git a/app/assets/javascripts/behaviors/markdown/marks/math.js b/app/assets/javascripts/behaviors/markdown/marks/math.js index e582fb18f15..04441d5d710 100644 --- a/app/assets/javascripts/behaviors/markdown/marks/math.js +++ b/app/assets/javascripts/behaviors/markdown/marks/math.js @@ -2,6 +2,7 @@ import { Mark } from 'tiptap'; import { defaultMarkdownSerializer } from 'prosemirror-markdown'; +import { HIGHER_PARSE_RULE_PRIORITY } from '../constants'; // Transforms generated HTML back to GFM for Banzai::Filter::MathFilter export default class MathMark extends Mark { @@ -15,7 +16,7 @@ export default class MathMark extends Mark { // Matches HTML generated by Banzai::Filter::MathFilter { tag: 'code.code.math[data-math-style=inline]', - priority: 51, + priority: HIGHER_PARSE_RULE_PRIORITY, }, // Matches HTML after being transformed by app/assets/javascripts/behaviors/markdown/render_math.js { diff --git a/app/assets/javascripts/behaviors/markdown/nodes/audio.js b/app/assets/javascripts/behaviors/markdown/nodes/audio.js index 48ac408cf24..146349b118c 100644 --- a/app/assets/javascripts/behaviors/markdown/nodes/audio.js +++ b/app/assets/javascripts/behaviors/markdown/nodes/audio.js @@ -1,53 +1,9 @@ -/* eslint-disable class-methods-use-this */ - -import { Node } from 'tiptap'; -import { defaultMarkdownSerializer } from 'prosemirror-markdown'; +import Playable from './playable'; // Transforms generated HTML back to GFM for Banzai::Filter::AudioLinkFilter -export default class Audio extends Node { - get name() { - return 'audio'; - } - - get schema() { - return { - attrs: { - src: {}, - alt: { - default: null, - }, - }, - group: 'block', - draggable: true, - parseDOM: [ - { - tag: '.audio-container', - skip: true, - }, - { - tag: '.audio-container p', - priority: 51, - ignore: true, - }, - { - tag: 'audio[src]', - getAttrs: el => ({ src: el.getAttribute('src'), alt: el.dataset.title }), - }, - ], - toDOM: node => [ - 'audio', - { - src: node.attrs.src, - controls: true, - 'data-setup': '{}', - 'data-title': node.attrs.alt, - }, - ], - }; - } - - toMarkdown(state, node) { - defaultMarkdownSerializer.nodes.image(state, node); - state.closeBlock(node); +export default class Audio extends Playable { + constructor() { + super(); + this.mediaType = 'audio'; } } diff --git a/app/assets/javascripts/behaviors/markdown/nodes/image.js b/app/assets/javascripts/behaviors/markdown/nodes/image.js index e839396330e..b1983eebe15 100644 --- a/app/assets/javascripts/behaviors/markdown/nodes/image.js +++ b/app/assets/javascripts/behaviors/markdown/nodes/image.js @@ -3,6 +3,7 @@ import { Image as BaseImage } from 'tiptap-extensions'; import { defaultMarkdownSerializer } from 'prosemirror-markdown'; import { placeholderImage } from '~/lazy_loader'; +import { HIGHER_PARSE_RULE_PRIORITY } from '../constants'; export default class Image extends BaseImage { get schema() { @@ -23,7 +24,7 @@ export default class Image extends BaseImage { // Matches HTML generated by Banzai::Filter::ImageLinkFilter { tag: 'a.no-attachment-icon', - priority: 51, + priority: HIGHER_PARSE_RULE_PRIORITY, skip: true, }, // Matches HTML generated by Banzai::Filter::ImageLazyLoadFilter diff --git a/app/assets/javascripts/behaviors/markdown/nodes/ordered_task_list.js b/app/assets/javascripts/behaviors/markdown/nodes/ordered_task_list.js index 25c4976a1bc..a28d7be3758 100644 --- a/app/assets/javascripts/behaviors/markdown/nodes/ordered_task_list.js +++ b/app/assets/javascripts/behaviors/markdown/nodes/ordered_task_list.js @@ -1,6 +1,7 @@ /* eslint-disable class-methods-use-this */ import { Node } from 'tiptap'; +import { HIGHER_PARSE_RULE_PRIORITY } from '../constants'; // Transforms generated HTML back to GFM for Banzai::Filter::TaskListFilter export default class OrderedTaskList extends Node { @@ -14,7 +15,7 @@ export default class OrderedTaskList extends Node { content: '(task_list_item|list_item)+', parseDOM: [ { - priority: 51, + priority: HIGHER_PARSE_RULE_PRIORITY, tag: 'ol.task-list', }, ], diff --git a/app/assets/javascripts/behaviors/markdown/nodes/playable.js b/app/assets/javascripts/behaviors/markdown/nodes/playable.js new file mode 100644 index 00000000000..08539df1242 --- /dev/null +++ b/app/assets/javascripts/behaviors/markdown/nodes/playable.js @@ -0,0 +1,73 @@ +/* eslint-disable class-methods-use-this */ +/* eslint-disable @gitlab/i18n/no-non-i18n-strings */ + +import { Node } from 'tiptap'; +import { defaultMarkdownSerializer } from 'prosemirror-markdown'; +import { HIGHER_PARSE_RULE_PRIORITY } from '../constants'; + +/** + * Abstract base class for playable media, like video and audio. + * Must not be instantiated directly. Subclasses must set + * the `mediaType` property in their constructors. + * @abstract + */ +export default class Playable extends Node { + constructor() { + super(); + this.mediaType = ''; + this.extraElementAttrs = {}; + } + + get name() { + return this.mediaType; + } + + get schema() { + const attrs = { + src: {}, + alt: { + default: null, + }, + }; + + const parseDOM = [ + { + tag: `.${this.mediaType}-container`, + skip: true, + }, + { + tag: `.${this.mediaType}-container p`, + priority: HIGHER_PARSE_RULE_PRIORITY, + ignore: true, + }, + { + tag: `${this.mediaType}[src]`, + getAttrs: el => ({ src: el.getAttribute('src'), alt: el.dataset.title }), + }, + ]; + + const toDOM = node => [ + this.mediaType, + { + src: node.attrs.src, + controls: true, + 'data-setup': '{}', + 'data-title': node.attrs.alt, + ...this.extraElementAttrs, + }, + ]; + + return { + attrs, + group: 'block', + draggable: true, + parseDOM, + toDOM, + }; + } + + toMarkdown(state, node) { + defaultMarkdownSerializer.nodes.image(state, node); + state.closeBlock(node); + } +} diff --git a/app/assets/javascripts/behaviors/markdown/nodes/reference.js b/app/assets/javascripts/behaviors/markdown/nodes/reference.js index 5d6bbeca833..aa724798da6 100644 --- a/app/assets/javascripts/behaviors/markdown/nodes/reference.js +++ b/app/assets/javascripts/behaviors/markdown/nodes/reference.js @@ -1,6 +1,7 @@ /* eslint-disable class-methods-use-this */ import { Node } from 'tiptap'; +import { HIGHER_PARSE_RULE_PRIORITY } from '../constants'; // Transforms generated HTML back to GFM for Banzai::Filter::ReferenceFilter and subclasses export default class Reference extends Node { @@ -23,7 +24,7 @@ export default class Reference extends Node { parseDOM: [ { tag: 'a.gfm:not([data-link=true])', - priority: 51, + priority: HIGHER_PARSE_RULE_PRIORITY, getAttrs: el => ({ className: el.className, referenceType: el.dataset.referenceType, diff --git a/app/assets/javascripts/behaviors/markdown/nodes/table_header_row.js b/app/assets/javascripts/behaviors/markdown/nodes/table_header_row.js index e7eee636402..6e3c16f0a08 100644 --- a/app/assets/javascripts/behaviors/markdown/nodes/table_header_row.js +++ b/app/assets/javascripts/behaviors/markdown/nodes/table_header_row.js @@ -1,6 +1,7 @@ /* eslint-disable class-methods-use-this */ import TableRow from './table_row'; +import { HIGHER_PARSE_RULE_PRIORITY } from '../constants'; const CENTER_ALIGN = 'center'; @@ -16,7 +17,7 @@ export default class TableHeaderRow extends TableRow { parseDOM: [ { tag: 'thead tr', - priority: 51, + priority: HIGHER_PARSE_RULE_PRIORITY, }, ], toDOM: () => ['tr', 0], diff --git a/app/assets/javascripts/behaviors/markdown/nodes/table_of_contents.js b/app/assets/javascripts/behaviors/markdown/nodes/table_of_contents.js index 9a2e2c03213..db9072acc3a 100644 --- a/app/assets/javascripts/behaviors/markdown/nodes/table_of_contents.js +++ b/app/assets/javascripts/behaviors/markdown/nodes/table_of_contents.js @@ -2,6 +2,7 @@ import { Node } from 'tiptap'; import { __ } from '~/locale'; +import { HIGHER_PARSE_RULE_PRIORITY } from '../constants'; // Transforms generated HTML back to GFM for Banzai::Filter::TableOfContentsFilter export default class TableOfContents extends Node { @@ -16,11 +17,11 @@ export default class TableOfContents extends Node { parseDOM: [ { tag: 'ul.section-nav', - priority: 51, + priority: HIGHER_PARSE_RULE_PRIORITY, }, { tag: 'p.table-of-contents', - priority: 51, + priority: HIGHER_PARSE_RULE_PRIORITY, }, ], toDOM: () => ['p', { class: 'table-of-contents' }, __('Table of Contents')], diff --git a/app/assets/javascripts/behaviors/markdown/nodes/task_list.js b/app/assets/javascripts/behaviors/markdown/nodes/task_list.js index ab33bc21502..35ba2eb0674 100644 --- a/app/assets/javascripts/behaviors/markdown/nodes/task_list.js +++ b/app/assets/javascripts/behaviors/markdown/nodes/task_list.js @@ -1,6 +1,7 @@ /* eslint-disable class-methods-use-this */ import { Node } from 'tiptap'; +import { HIGHER_PARSE_RULE_PRIORITY } from '../constants'; // Transforms generated HTML back to GFM for Banzai::Filter::TaskListFilter export default class TaskList extends Node { @@ -14,7 +15,7 @@ export default class TaskList extends Node { content: '(task_list_item|list_item)+', parseDOM: [ { - priority: 51, + priority: HIGHER_PARSE_RULE_PRIORITY, tag: 'ul.task-list', }, ], diff --git a/app/assets/javascripts/behaviors/markdown/nodes/task_list_item.js b/app/assets/javascripts/behaviors/markdown/nodes/task_list_item.js index d0ee7333d5e..7bb56b4c406 100644 --- a/app/assets/javascripts/behaviors/markdown/nodes/task_list_item.js +++ b/app/assets/javascripts/behaviors/markdown/nodes/task_list_item.js @@ -1,6 +1,7 @@ /* eslint-disable class-methods-use-this */ import { Node } from 'tiptap'; +import { HIGHER_PARSE_RULE_PRIORITY } from '../constants'; // Transforms generated HTML back to GFM for Banzai::Filter::TaskListFilter export default class TaskListItem extends Node { @@ -20,7 +21,7 @@ export default class TaskListItem extends Node { content: 'paragraph block*', parseDOM: [ { - priority: 51, + priority: HIGHER_PARSE_RULE_PRIORITY, tag: 'li.task-list-item', getAttrs: el => { const checkbox = el.querySelector('input[type=checkbox].task-list-item-checkbox'); diff --git a/app/assets/javascripts/behaviors/markdown/nodes/video.js b/app/assets/javascripts/behaviors/markdown/nodes/video.js index 516f983397d..68085c2c416 100644 --- a/app/assets/javascripts/behaviors/markdown/nodes/video.js +++ b/app/assets/javascripts/behaviors/markdown/nodes/video.js @@ -1,54 +1,10 @@ -/* eslint-disable class-methods-use-this */ - -import { Node } from 'tiptap'; -import { defaultMarkdownSerializer } from 'prosemirror-markdown'; +import Playable from './playable'; // Transforms generated HTML back to GFM for Banzai::Filter::VideoLinkFilter -export default class Video extends Node { - get name() { - return 'video'; - } - - get schema() { - return { - attrs: { - src: {}, - alt: { - default: null, - }, - }, - group: 'block', - draggable: true, - parseDOM: [ - { - tag: '.video-container', - skip: true, - }, - { - tag: '.video-container p', - priority: 51, - ignore: true, - }, - { - tag: 'video[src]', - getAttrs: el => ({ src: el.getAttribute('src'), alt: el.dataset.title }), - }, - ], - toDOM: node => [ - 'video', - { - src: node.attrs.src, - width: '400', - controls: true, - 'data-setup': '{}', - 'data-title': node.attrs.alt, - }, - ], - }; - } - - toMarkdown(state, node) { - defaultMarkdownSerializer.nodes.image(state, node); - state.closeBlock(node); +export default class Video extends Playable { + constructor() { + super(); + this.mediaType = 'video'; + this.extraElementAttrs = { width: '400' }; } } diff --git a/app/helpers/system_note_helper.rb b/app/helpers/system_note_helper.rb index 51cbe93513d..05d698a6d99 100644 --- a/app/helpers/system_note_helper.rb +++ b/app/helpers/system_note_helper.rb @@ -2,6 +2,7 @@ module SystemNoteHelper ICON_NAMES_BY_ACTION = { + 'cherry_pick' => 'link', 'commit' => 'commit', 'description' => 'pencil-square', 'merge' => 'git-merge', diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 4ed73d979ce..7042bc7074f 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -228,6 +228,9 @@ class MergeRequest < ApplicationRecord scope :by_merge_commit_sha, -> (sha) do where(merge_commit_sha: sha) end + scope :by_cherry_pick_sha, -> (sha) do + joins(:notes).where(notes: { commit_id: sha }) + end scope :join_project, -> { joins(:target_project) } scope :references_project, -> { references(:target_project) } scope :with_api_entity_associations, -> { diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 5a44ee7211b..6324636db1e 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -17,7 +17,7 @@ class SystemNoteMetadata < ApplicationRecord commit description merge confidential visible label assignee cross_reference title time_tracking branch milestone discussion task moved opened closed merged duplicate locked unlocked - outdated tag due_date pinned_embed + outdated tag due_date pinned_embed cherry_pick ].freeze validates :note, presence: true diff --git a/app/services/commits/cherry_pick_service.rb b/app/services/commits/cherry_pick_service.rb index 4c5b15b2f95..91a18909e22 100644 --- a/app/services/commits/cherry_pick_service.rb +++ b/app/services/commits/cherry_pick_service.rb @@ -3,7 +3,24 @@ module Commits class CherryPickService < ChangeService def create_commit! - commit_change(:cherry_pick) + commit_change(:cherry_pick).tap do |sha| + track_mr_picking(sha) + end + end + + private + + def track_mr_picking(pick_sha) + return unless Feature.enabled?(:track_mr_picking, project) + + merge_request = project.merge_requests.by_merge_commit_sha(@commit.sha).first + return unless merge_request + + ::SystemNotes::MergeRequestsService.new( + noteable: merge_request, + project: project, + author: current_user + ).picked_into_branch(@branch_name, pick_sha) end end end diff --git a/app/services/deployments/link_merge_requests_service.rb b/app/services/deployments/link_merge_requests_service.rb index a1d6d50bbb4..67a2230350d 100644 --- a/app/services/deployments/link_merge_requests_service.rb +++ b/app/services/deployments/link_merge_requests_service.rb @@ -38,6 +38,8 @@ module Deployments .commits_between(from, to) .map(&:id) + track_mr_picking = Feature.enabled?(:track_mr_picking, project) + # For some projects the list of commits to deploy may be very large. To # ensure we do not end up running SQL queries with thousands of WHERE IN # values, we run one query per a certain number of commits. @@ -50,6 +52,13 @@ module Deployments project.merge_requests.merged.by_merge_commit_sha(slice) deployment.link_merge_requests(merge_requests) + + next unless track_mr_picking + + picked_merge_requests = + project.merge_requests.by_cherry_pick_sha(slice) + + deployment.link_merge_requests(picked_merge_requests) end end diff --git a/app/services/system_notes/merge_requests_service.rb b/app/services/system_notes/merge_requests_service.rb index 1d17f0ded57..a26fc0f7d35 100644 --- a/app/services/system_notes/merge_requests_service.rb +++ b/app/services/system_notes/merge_requests_service.rb @@ -139,6 +139,17 @@ module SystemNotes create_note(NoteSummary.new(noteable, project, author, body, action: 'merge')) end + + def picked_into_branch(branch_name, pick_commit) + link = url_helpers.project_tree_path(project, branch_name) + + body = "picked this merge request into branch [`#{branch_name}`](#{link}) with commit #{pick_commit}" + + summary = NoteSummary.new(noteable, project, author, body, action: 'cherry_pick') + summary.note[:commit_id] = pick_commit + + create_note(summary) + end end end diff --git a/changelogs/unreleased/bvl-remove-request-deadline-ff.yml b/changelogs/unreleased/bvl-remove-request-deadline-ff.yml new file mode 100644 index 00000000000..33b33b47e00 --- /dev/null +++ b/changelogs/unreleased/bvl-remove-request-deadline-ff.yml @@ -0,0 +1,5 @@ +--- +title: Don't allow Gitaly calls to exceed the worker timeout set for unicorn or puma +merge_request: 23510 +author: +type: performance diff --git a/doc/integration/saml.md b/doc/integration/saml.md index 11e768194bc..ce1c23f77af 100644 --- a/doc/integration/saml.md +++ b/doc/integration/saml.md @@ -545,6 +545,13 @@ GitLab will sign the request with the provided private key. GitLab will include ## Troubleshooting +### GitLab+SAML Testing Environments + +If you need to troubleshoot, below is a complete GitLab+SAML testing environment using docker compose: +https://gitlab.com/gitlab-com/support/toolbox/replication/tree/master/compose_files + +If you only need a SAML provider for testing, below is quick start guide to start a Docker container with a plug and play SAML 2.0 Identity Provider (IdP): https://docs.gitlab.com/ee/administration/troubleshooting/test_environments.html#saml + ### 500 error after login If you see a "500 error" in GitLab when you are redirected back from the SAML sign in page, diff --git a/lib/gitlab/request_context.rb b/lib/gitlab/request_context.rb index 214670cac28..9da6732796a 100644 --- a/lib/gitlab/request_context.rb +++ b/lib/gitlab/request_context.rb @@ -18,7 +18,6 @@ module Gitlab def request_deadline strong_memoize(:request_deadline) do next unless request_start_time - next unless Feature.enabled?(:request_deadline) request_start_time + max_request_duration_seconds end diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 11fc5060cf0..ec89a5904f5 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -29,6 +29,11 @@ FactoryBot.define do end end + factory :track_mr_picking_note, traits: [:on_merge_request, :system] do + association :system_note_metadata, action: 'cherry_pick' + commit_id { RepoHelpers.sample_commit.id } + end + factory :discussion_note_on_issue, traits: [:on_issue], class: 'DiscussionNote' factory :discussion_note_on_commit, traits: [:on_commit], class: 'DiscussionNote' diff --git a/spec/lib/gitlab/request_context_spec.rb b/spec/lib/gitlab/request_context_spec.rb index 5785dbfd850..7e2e05c9f1b 100644 --- a/spec/lib/gitlab/request_context_spec.rb +++ b/spec/lib/gitlab/request_context_spec.rb @@ -24,18 +24,6 @@ describe Gitlab::RequestContext, :request_store do expect(subject.request_deadline).to be_nil end - - it 'only checks the feature once per request-instance' do - expect(Feature).to receive(:enabled?).with(:request_deadline).once - - 2.times { subject.request_deadline } - end - - it 'returns nil when the feature is disabled' do - stub_feature_flags(request_deadline: false) - - expect(subject.request_deadline).to be_nil - end end describe '#ensure_request_deadline_not_exceeded!' do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 9f256719248..2fd1a87e5f1 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -327,6 +327,16 @@ describe MergeRequest do end end + describe '.by_cherry_pick_sha' do + it 'returns merge requests that match the given merge commit' do + note = create(:track_mr_picking_note, commit_id: '456abc') + + create(:track_mr_picking_note, commit_id: '456def') + + expect(described_class.by_cherry_pick_sha('456abc')).to eq([note.noteable]) + end + end + describe '.in_projects' do it 'returns the merge requests for a set of projects' do expect(described_class.in_projects(Project.all)).to eq([subject]) diff --git a/spec/requests/api/deployments_spec.rb b/spec/requests/api/deployments_spec.rb index d8fc234cbae..151f67061eb 100644 --- a/spec/requests/api/deployments_spec.rb +++ b/spec/requests/api/deployments_spec.rb @@ -122,7 +122,29 @@ describe API::Deployments do describe 'POST /projects/:id/deployments' do let!(:project) { create(:project, :repository) } - let(:sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' } + # * ddd0f15ae83993f5cb66a927a28673882e99100b (HEAD -> master, origin/master, origin/HEAD) Merge branch 'po-fix-test-en + # |\ + # | * 2d1db523e11e777e49377cfb22d368deec3f0793 Correct test_env.rb path for adding branch + # |/ + # * 1e292f8fedd741b75372e19097c76d327140c312 Merge branch 'cherry-pick-ce369011' into 'master' + + let_it_be(:sha) { 'ddd0f15ae83993f5cb66a927a28673882e99100b' } + let_it_be(:first_deployment_sha) { '1e292f8fedd741b75372e19097c76d327140c312' } + + before do + # Creating the first deployment is an edge-case that is already covered by unit testing, + # here we want to see the behavior of a running system so we create a first deployment + post( + api("/projects/#{project.id}/deployments", user), + params: { + environment: 'production', + sha: first_deployment_sha, + ref: 'master', + tag: false, + status: 'success' + } + ) + end context 'as a maintainer' do it 'creates a new deployment' do @@ -163,6 +185,7 @@ describe API::Deployments do mr = create( :merge_request, :merged, + merge_commit_sha: sha, target_project: project, source_project: project, target_branch: 'master', @@ -215,6 +238,7 @@ describe API::Deployments do mr = create( :merge_request, :merged, + merge_commit_sha: sha, target_project: project, source_project: project, target_branch: 'master', @@ -236,6 +260,43 @@ describe API::Deployments do expect(deploy.merge_requests).to eq([mr]) end + + it 'links any picked merge requests to the deployment', :sidekiq_inline do + mr = create( + :merge_request, + :merged, + merge_commit_sha: sha, + target_project: project, + source_project: project, + target_branch: 'master', + source_branch: 'foo' + ) + + # we branch from the previous deployment and cherry-pick mr into the new branch + branch = project.repository.add_branch(developer, 'stable', first_deployment_sha) + expect(branch).not_to be_nil + + result = ::Commits::CherryPickService + .new(project, developer, commit: mr.merge_commit, start_branch: 'stable', branch_name: 'stable') + .execute + expect(result[:status]).to eq(:success), result[:message] + + pick_sha = result[:result] + + post( + api("/projects/#{project.id}/deployments", developer), + params: { + environment: 'production', + sha: pick_sha, + ref: 'stable', + tag: false, + status: 'success' + } + ) + + deploy = project.deployments.last + expect(deploy.merge_requests).to eq([mr]) + end end context 'as non member' do diff --git a/spec/services/commits/cherry_pick_service_spec.rb b/spec/services/commits/cherry_pick_service_spec.rb new file mode 100644 index 00000000000..ead1932c2d1 --- /dev/null +++ b/spec/services/commits/cherry_pick_service_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Commits::CherryPickService do + let(:project) { create(:project, :repository) } + # * ddd0f15ae83993f5cb66a927a28673882e99100b (HEAD -> master, origin/master, origin/HEAD) Merge branch 'po-fix-test-en + # |\ + # | * 2d1db523e11e777e49377cfb22d368deec3f0793 Correct test_env.rb path for adding branch + # |/ + # * 1e292f8fedd741b75372e19097c76d327140c312 Merge branch 'cherry-pick-ce369011' into 'master' + + let_it_be(:merge_commit_sha) { 'ddd0f15ae83993f5cb66a927a28673882e99100b' } + let_it_be(:merge_base_sha) { '1e292f8fedd741b75372e19097c76d327140c312' } + let_it_be(:branch_name) { 'stable' } + + let(:repository) { project.repository } + let(:commit) { project.commit } + let(:user) { create(:user) } + + before do + project.add_maintainer(user) + + repository.add_branch(user, branch_name, merge_base_sha) + end + + def cherry_pick(sha, branch_name) + commit = project.commit(sha) + + described_class.new( + project, + user, + commit: commit, + start_branch: branch_name, + branch_name: branch_name + ).execute + end + + describe '#execute' do + shared_examples 'successful cherry-pick' do + it 'picks the commit into the branch' do + result = cherry_pick(merge_commit_sha, branch_name) + expect(result[:status]).to eq(:success), result[:message] + + head = repository.find_branch(branch_name).target + expect(head).not_to eq(merge_base_sha) + end + end + + it_behaves_like 'successful cherry-pick' + + context 'when picking a merge-request' do + let!(:merge_request) { create(:merge_request, :simple, :merged, author: user, source_project: project, merge_commit_sha: merge_commit_sha) } + + it_behaves_like 'successful cherry-pick' + + it 'adds a system note' do + result = cherry_pick(merge_commit_sha, branch_name) + + mr_notes = find_cherry_pick_notes(merge_request) + expect(mr_notes.length).to eq(1) + expect(mr_notes[0].commit_id).to eq(result[:result]) + end + + context 'when :track_mr_picking feature flag is disabled' do + before do + stub_feature_flags(track_mr_picking: false) + end + + it 'does not add system notes' do + expect do + cherry_pick(merge_commit_sha, branch_name) + end.not_to change { Note.count } + end + end + end + + def find_cherry_pick_notes(noteable) + noteable + .notes + .joins(:system_note_metadata) + .where(system_note_metadata: { action: 'cherry_pick' }) + end + end +end diff --git a/spec/services/deployments/link_merge_requests_service_spec.rb b/spec/services/deployments/link_merge_requests_service_spec.rb index 307fe22a192..605f2cfdc51 100644 --- a/spec/services/deployments/link_merge_requests_service_spec.rb +++ b/spec/services/deployments/link_merge_requests_service_spec.rb @@ -5,6 +5,19 @@ require 'spec_helper' describe Deployments::LinkMergeRequestsService do let(:project) { create(:project, :repository) } + # * ddd0f15 Merge branch 'po-fix-test-env-path' into 'master' + # |\ + # | * 2d1db52 Correct test_env.rb path for adding branch + # |/ + # * 1e292f8 Merge branch 'cherry-pick-ce369011' into 'master' + # |\ + # | * c1c67ab Add file with a _flattable_ path + # |/ + # * 7975be0 Merge branch 'rd-add-file-larger-than-1-mb' into 'master' + let_it_be(:first_deployment_sha) { '7975be0116940bf2ad4321f79d02a55c5f7779aa' } + let_it_be(:mr1_merge_commit_sha) { '1e292f8fedd741b75372e19097c76d327140c312' } + let_it_be(:mr2_merge_commit_sha) { 'ddd0f15ae83993f5cb66a927a28673882e99100b' } + describe '#execute' do context 'when the deployment is for a review environment' do it 'does nothing' do @@ -25,7 +38,7 @@ describe Deployments::LinkMergeRequestsService do :deployment, :success, project: project, - sha: '7975be0116940bf2ad4321f79d02a55c5f7779aa' + sha: first_deployment_sha ) deploy2 = create( @@ -33,17 +46,14 @@ describe Deployments::LinkMergeRequestsService do :success, project: deploy1.project, environment: deploy1.environment, - sha: 'ddd0f15ae83993f5cb66a927a28673882e99100b' + sha: mr2_merge_commit_sha ) service = described_class.new(deploy2) expect(service) .to receive(:link_merge_requests_for_range) - .with( - '7975be0116940bf2ad4321f79d02a55c5f7779aa', - 'ddd0f15ae83993f5cb66a927a28673882e99100b' - ) + .with(first_deployment_sha, mr2_merge_commit_sha) service.execute end @@ -70,7 +80,7 @@ describe Deployments::LinkMergeRequestsService do mr1 = create( :merge_request, :merged, - merge_commit_sha: '1e292f8fedd741b75372e19097c76d327140c312', + merge_commit_sha: mr1_merge_commit_sha, source_project: project, target_project: project ) @@ -78,18 +88,97 @@ describe Deployments::LinkMergeRequestsService do mr2 = create( :merge_request, :merged, - merge_commit_sha: '2d1db523e11e777e49377cfb22d368deec3f0793', + merge_commit_sha: mr2_merge_commit_sha, source_project: project, target_project: project ) described_class.new(deploy).link_merge_requests_for_range( - '7975be0116940bf2ad4321f79d02a55c5f7779aa', - 'ddd0f15ae83993f5cb66a927a28673882e99100b' + first_deployment_sha, + mr2_merge_commit_sha ) expect(deploy.merge_requests).to include(mr1, mr2) end + + it 'links picked merge requests' do + environment = create(:environment, project: project) + deploy = + create(:deployment, :success, project: project, environment: environment) + + picked_mr = create( + :merge_request, + :merged, + merge_commit_sha: '123abc', + source_project: project, + target_project: project + ) + + mr1 = create( + :merge_request, + :merged, + merge_commit_sha: mr1_merge_commit_sha, + source_project: project, + target_project: project + ) + + # mr1 includes c1c67abba which is a cherry-pick of the fake picked_mr merge request + create(:track_mr_picking_note, noteable: picked_mr, project: project, commit_id: 'c1c67abbaf91f624347bb3ae96eabe3a1b742478') + + described_class.new(deploy).link_merge_requests_for_range( + first_deployment_sha, + mr1_merge_commit_sha + ) + + expect(deploy.merge_requests).to include(mr1, picked_mr) + end + + context 'when :track_mr_picking feature flag is disabled' do + before do + stub_feature_flags(track_mr_picking: false) + end + + it 'does not link picked merge requests' do + environment = create(:environment, project: project) + deploy = + create(:deployment, :success, project: project, environment: environment) + + picked_mr = create( + :merge_request, + :merged, + merge_commit_sha: '123abc', + source_project: project, + target_project: project + ) + + mr1 = create( + :merge_request, + :merged, + merge_commit_sha: mr1_merge_commit_sha, + source_project: project, + target_project: project + ) + + # mr1 includes c1c67abba which is a cherry-pick of the fake picked_mr merge request + create(:track_mr_picking_note, noteable: picked_mr, project: project, commit_id: 'c1c67abbaf91f624347bb3ae96eabe3a1b742478') + + mr2 = create( + :merge_request, + :merged, + merge_commit_sha: mr2_merge_commit_sha, + source_project: project, + target_project: project + ) + + described_class.new(deploy).link_merge_requests_for_range( + first_deployment_sha, + mr2_merge_commit_sha + ) + + expect(deploy.merge_requests).to include(mr1, mr2) + expect(deploy.merge_requests).not_to include(picked_mr) + end + end end describe '#link_all_merged_merge_requests' do diff --git a/spec/services/system_notes/merge_requests_service_spec.rb b/spec/services/system_notes/merge_requests_service_spec.rb index 6d2473e8c03..2a04e888ae0 100644 --- a/spec/services/system_notes/merge_requests_service_spec.rb +++ b/spec/services/system_notes/merge_requests_service_spec.rb @@ -240,4 +240,25 @@ describe ::SystemNotes::MergeRequestsService do expect(subject.note).to eq("created merge request #{merge_request.to_reference(project)} to address this issue") end end + + describe '.picked_into_branch' do + let(:branch_name) { 'a-branch' } + let(:commit_sha) { project.commit.sha } + let(:merge_request) { noteable } + + subject { service.picked_into_branch(branch_name, commit_sha) } + + it_behaves_like 'a system note' do + let(:action) { 'cherry_pick' } + end + + it "posts the 'picked merge request' system note" do + expect(subject.note).to eq("picked this merge request into branch [`#{branch_name}`](/#{project.full_path}/tree/#{branch_name}) with commit #{commit_sha}") + end + + it 'links the merge request and the cherry-pick commit' do + expect(subject.noteable).to eq(merge_request) + expect(subject.commit_id).to eq(commit_sha) + end + end end diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index 6d9c27d0255..bb8b0dfde21 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -136,6 +136,7 @@ module GraphqlHelpers allow_unlimited_graphql_complexity allow_unlimited_graphql_depth allow_high_graphql_recursion + allow_high_graphql_transaction_threshold type = GitlabSchema.types[class_name.to_s] return "" unless type @@ -295,6 +296,10 @@ module GraphqlHelpers allow_any_instance_of(Gitlab::Graphql::QueryAnalyzers::RecursionAnalyzer).to receive(:recursion_threshold).and_return 1000 end + def allow_high_graphql_transaction_threshold + stub_const("Gitlab::QueryLimiting::Transaction::THRESHOLD", 1000) + end + def node_array(data, extract_attribute = nil) data.map do |item| extract_attribute ? item['node'][extract_attribute] : item['node']