From 0640b3d1d89b7a4eda343eb23b0518a835ac9106 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 31 Jul 2017 19:01:36 -0300 Subject: Store MergeWorker JID on merge request, and clean up stuck merges --- .flayignore | 1 + .../components/states/mr_widget_locked.js | 29 ------------- .../components/states/mr_widget_merging.js | 29 +++++++++++++ .../vue_merge_request_widget/dependencies.js | 2 +- .../vue_merge_request_widget/mr_widget_options.js | 4 +- .../stores/mr_widget_store.js | 9 ++-- .../vue_merge_request_widget/stores/state_maps.js | 4 +- .../projects/merge_requests_controller.rb | 5 --- app/models/merge_request.rb | 22 +++------- app/serializers/merge_request_entity.rb | 2 +- app/workers/merge_worker.rb | 2 + app/workers/stuck_merge_jobs_worker.rb | 34 +++++++++++++++ .../31207-clean-locked-merge-requests.yml | 4 ++ config/initializers/1_settings.rb | 4 ++ ...170731183033_add_merge_jid_to_merge_requests.rb | 7 +++ ..._remove_locked_at_column_from_merge_requests.rb | 11 +++++ db/schema.rb | 2 +- doc/user/project/integrations/webhooks.md | 1 - spec/features/merge_requests/widget_spec.rb | 13 ++++++ .../api/schemas/entities/merge_request.json | 4 +- .../components/states/mr_widget_locked_spec.js | 10 ++--- spec/javascripts/vue_mr_widget/mock_data.js | 1 - .../vue_mr_widget/mr_widget_options_spec.js | 2 +- spec/lib/gitlab/import_export/project.json | 9 ---- .../gitlab/import_export/safe_model_attributes.yml | 1 - spec/models/merge_request_spec.rb | 26 +++++++++++ spec/serializers/merge_request_entity_spec.rb | 2 +- spec/workers/merge_worker_spec.rb | 11 +++++ spec/workers/stuck_merge_jobs_worker_spec.rb | 50 ++++++++++++++++++++++ 29 files changed, 220 insertions(+), 81 deletions(-) delete mode 100644 app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js create mode 100644 app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js create mode 100644 app/workers/stuck_merge_jobs_worker.rb create mode 100644 changelogs/unreleased/31207-clean-locked-merge-requests.yml create mode 100644 db/migrate/20170731183033_add_merge_jid_to_merge_requests.rb create mode 100644 db/migrate/20170801000000_remove_locked_at_column_from_merge_requests.rb create mode 100644 spec/workers/stuck_merge_jobs_worker_spec.rb diff --git a/.flayignore b/.flayignore index e2d0a2e50c5..b63ce4c4df0 100644 --- a/.flayignore +++ b/.flayignore @@ -3,4 +3,5 @@ lib/gitlab/sanitizers/svg/whitelist.rb lib/gitlab/diff/position_tracer.rb app/policies/project_policy.rb app/models/concerns/relative_positioning.rb +app/workers/stuck_merge_jobs_worker.rb lib/gitlab/redis/*.rb diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js deleted file mode 100644 index a12f418e1af..00000000000 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js +++ /dev/null @@ -1,29 +0,0 @@ -import statusIcon from '../mr_widget_status_icon'; - -export default { - name: 'MRWidgetLocked', - props: { - mr: { type: Object, required: true }, - }, - components: { - statusIcon, - }, - template: ` -
- -
-

- This merge request is in the process of being merged, during which time it is locked and cannot be closed -

-
-

- The changes will be merged into - - {{mr.targetBranch}} - -

-
-
-
- `, -}; diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js new file mode 100644 index 00000000000..f6d1a4feeb2 --- /dev/null +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js @@ -0,0 +1,29 @@ +import statusIcon from '../mr_widget_status_icon'; + +export default { + name: 'MRWidgetMerging', + props: { + mr: { type: Object, required: true }, + }, + components: { + statusIcon, + }, + template: ` +
+ +
+

+ This merge request is in the process of being merged +

+
+

+ The changes will be merged into + + {{mr.targetBranch}} + +

+
+
+
+ `, +}; diff --git a/app/assets/javascripts/vue_merge_request_widget/dependencies.js b/app/assets/javascripts/vue_merge_request_widget/dependencies.js index 546a3f625c7..49340c232c8 100644 --- a/app/assets/javascripts/vue_merge_request_widget/dependencies.js +++ b/app/assets/javascripts/vue_merge_request_widget/dependencies.js @@ -19,7 +19,7 @@ export { default as WidgetRelatedLinks } from './components/mr_widget_related_li export { default as MergedState } from './components/states/mr_widget_merged'; export { default as FailedToMerge } from './components/states/mr_widget_failed_to_merge'; export { default as ClosedState } from './components/states/mr_widget_closed'; -export { default as LockedState } from './components/states/mr_widget_locked'; +export { default as MergingState } from './components/states/mr_widget_merging'; export { default as WipState } from './components/states/mr_widget_wip'; export { default as ArchivedState } from './components/states/mr_widget_archived'; export { default as ConflictsState } from './components/states/mr_widget_conflicts'; diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 577d77f09a6..0042c48816f 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -8,7 +8,7 @@ import { WidgetRelatedLinks, MergedState, ClosedState, - LockedState, + MergingState, WipState, ArchivedState, ConflictsState, @@ -212,7 +212,7 @@ export default { 'mr-widget-related-links': WidgetRelatedLinks, 'mr-widget-merged': MergedState, 'mr-widget-closed': ClosedState, - 'mr-widget-locked': LockedState, + 'mr-widget-merging': MergingState, 'mr-widget-failed-to-merge': FailedToMerge, 'mr-widget-wip': WipState, 'mr-widget-archived': ArchivedState, diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index fddafb0ddfa..fbea764b739 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -73,6 +73,7 @@ export default class MergeRequestStore { this.canCancelAutomaticMerge = !!data.cancel_merge_when_pipeline_succeeds_path; this.hasSHAChanged = this.sha !== data.diff_head_sha; this.canBeMerged = data.can_be_merged || false; + this.mergeOngoing = data.merge_ongoing; // Cherry-pick and Revert actions related this.canCherryPickInCurrentMR = currentUser.can_cherry_pick_on_current_merge_request || false; @@ -94,6 +95,11 @@ export default class MergeRequestStore { } setState(data) { + if (this.mergeOngoing) { + this.state = 'merging'; + return; + } + if (this.isOpen) { this.state = getStateKey.call(this, data); } else { @@ -104,9 +110,6 @@ export default class MergeRequestStore { case 'closed': this.state = 'closed'; break; - case 'locked': - this.state = 'locked'; - break; default: this.state = null; } diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js index 605dd3a1ff4..9074a064a6d 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js @@ -1,7 +1,7 @@ const stateToComponentMap = { merged: 'mr-widget-merged', closed: 'mr-widget-closed', - locked: 'mr-widget-locked', + merging: 'mr-widget-merging', conflicts: 'mr-widget-conflicts', missingBranch: 'mr-widget-missing-branch', workInProgress: 'mr-widget-wip', @@ -20,7 +20,7 @@ const stateToComponentMap = { }; const statesToShowHelpWidget = [ - 'locked', + 'merging', 'conflicts', 'workInProgress', 'readyToMerge', diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d361e661d0e..4de814d0ca8 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -67,11 +67,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @noteable = @merge_request @commits_count = @merge_request.commits_count - if @merge_request.locked_long_ago? - @merge_request.unlock_mr - @merge_request.close - end - labels set_pipeline_variables diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 8ca850b6d96..e0c779b4fbc 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -61,16 +61,6 @@ class MergeRequest < ActiveRecord::Base transition locked: :opened end - after_transition any => :locked do |merge_request, transition| - merge_request.locked_at = Time.now - merge_request.save - end - - after_transition locked: (any - :locked) do |merge_request, transition| - merge_request.locked_at = nil - merge_request.save - end - state :opened state :closed state :merged @@ -392,6 +382,12 @@ class MergeRequest < ActiveRecord::Base 'Source project is not a fork of the target project' end + def merge_ongoing? + return false unless merge_jid + + Gitlab::SidekiqStatus.num_running([merge_jid]) > 0 + end + def closed_without_fork? closed? && source_project_missing? end @@ -725,12 +721,6 @@ class MergeRequest < ActiveRecord::Base end end - def locked_long_ago? - return false unless locked? - - locked_at.nil? || locked_at < (Time.now - 1.day) - end - def has_ci? has_ci_integration = source_project.try(:ci_service) uses_gitlab_ci = all_pipelines.any? diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb index 7f17f2bf604..07650ce6f20 100644 --- a/app/serializers/merge_request_entity.rb +++ b/app/serializers/merge_request_entity.rb @@ -2,7 +2,6 @@ class MergeRequestEntity < IssuableEntity include RequestAwareEntity expose :in_progress_merge_commit_sha - expose :locked_at expose :merge_commit_sha expose :merge_error expose :merge_params @@ -32,6 +31,7 @@ class MergeRequestEntity < IssuableEntity expose :head_pipeline, with: PipelineDetailsEntity, as: :pipeline # Booleans + expose :merge_ongoing?, as: :merge_ongoing expose :work_in_progress?, as: :work_in_progress expose :source_branch_exists?, as: :source_branch_exists expose :mergeable_discussions_state?, as: :mergeable_discussions_state diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb index 48e2da338f6..c3b58df92c1 100644 --- a/app/workers/merge_worker.rb +++ b/app/workers/merge_worker.rb @@ -7,6 +7,8 @@ class MergeWorker current_user = User.find(current_user_id) merge_request = MergeRequest.find(merge_request_id) + merge_request.update_column(:merge_jid, jid) + MergeRequests::MergeService.new(merge_request.target_project, current_user, params) .execute(merge_request) end diff --git a/app/workers/stuck_merge_jobs_worker.rb b/app/workers/stuck_merge_jobs_worker.rb new file mode 100644 index 00000000000..7843179d77c --- /dev/null +++ b/app/workers/stuck_merge_jobs_worker.rb @@ -0,0 +1,34 @@ +class StuckMergeJobsWorker + include Sidekiq::Worker + include CronjobQueue + + def perform + stuck_merge_requests.find_in_batches(batch_size: 100) do |group| + jids = group.map(&:merge_jid) + + # Find the jobs that aren't currently running or that exceeded the threshold. + completed_jids = Gitlab::SidekiqStatus.completed_jids(jids) + + if completed_jids.any? + completed_ids = group.select { |merge_request| completed_jids.include?(merge_request.merge_jid) }.map(&:id) + + apply_current_state!(completed_jids, completed_ids) + end + end + end + + private + + def apply_current_state!(completed_jids, completed_ids) + merge_requests = MergeRequest.where(id: completed_ids) + + merge_requests.where.not(merge_commit_sha: nil).update_all(state: :merged) + merge_requests.where(merge_commit_sha: nil).update_all(state: :opened) + + Rails.logger.info("Updated state of locked merge jobs. JIDs: #{completed_jids.join(', ')}") + end + + def stuck_merge_requests + MergeRequest.select('id, merge_jid').with_state(:locked).where.not(merge_jid: nil).reorder(nil) + end +end diff --git a/changelogs/unreleased/31207-clean-locked-merge-requests.yml b/changelogs/unreleased/31207-clean-locked-merge-requests.yml new file mode 100644 index 00000000000..1f52987baef --- /dev/null +++ b/changelogs/unreleased/31207-clean-locked-merge-requests.yml @@ -0,0 +1,4 @@ +--- +title: Unlock stuck merge request and set the proper state +merge_request: 13207 +author: diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 7a43bf939ea..396bc57bd64 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -395,6 +395,10 @@ Settings.cron_jobs['remove_old_web_hook_logs_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['remove_old_web_hook_logs_worker']['cron'] ||= '40 0 * * *' Settings.cron_jobs['remove_old_web_hook_logs_worker']['job_class'] = 'RemoveOldWebHookLogsWorker' +Settings.cron_jobs['stuck_merge_jobs_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['stuck_merge_jobs_worker']['cron'] ||= '0 */2 * * *' +Settings.cron_jobs['stuck_merge_jobs_worker']['job_class'] = 'StuckMergeJobsWorker' + # # GitLab Shell # diff --git a/db/migrate/20170731183033_add_merge_jid_to_merge_requests.rb b/db/migrate/20170731183033_add_merge_jid_to_merge_requests.rb new file mode 100644 index 00000000000..a7d8f2f3604 --- /dev/null +++ b/db/migrate/20170731183033_add_merge_jid_to_merge_requests.rb @@ -0,0 +1,7 @@ +class AddMergeJidToMergeRequests < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column :merge_requests, :merge_jid, :string + end +end diff --git a/db/migrate/20170801000000_remove_locked_at_column_from_merge_requests.rb b/db/migrate/20170801000000_remove_locked_at_column_from_merge_requests.rb new file mode 100644 index 00000000000..ea3d1fb3e02 --- /dev/null +++ b/db/migrate/20170801000000_remove_locked_at_column_from_merge_requests.rb @@ -0,0 +1,11 @@ +class RemoveLockedAtColumnFromMergeRequests < ActiveRecord::Migration + DOWNTIME = false + + def up + remove_column :merge_requests, :locked_at + end + + def down + add_column :merge_requests, :locked_at, :datetime_with_timezone + end +end diff --git a/db/schema.rb b/db/schema.rb index f2f35acef95..4816e1aefab 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -840,7 +840,6 @@ ActiveRecord::Schema.define(version: 20170803130232) do t.integer "target_project_id", null: false t.integer "iid" t.text "description" - t.datetime "locked_at" t.integer "updated_by_id" t.text "merge_error" t.text "merge_params" @@ -858,6 +857,7 @@ ActiveRecord::Schema.define(version: 20170803130232) do t.integer "last_edited_by_id" t.integer "head_pipeline_id" t.boolean "ref_fetched" + t.string "merge_jid" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index c03a2df9a72..47eb0b34f66 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -438,7 +438,6 @@ X-Gitlab-Event: Note Hook "iid": 1, "description": "Et voluptas corrupti assumenda temporibus. Architecto cum animi eveniet amet asperiores. Vitae numquam voluptate est natus sit et ad id.", "position": 0, - "locked_at": null, "source":{ "name":"Gitlab Test", "description":"Aut reprehenderit ut est.", diff --git a/spec/features/merge_requests/widget_spec.rb b/spec/features/merge_requests/widget_spec.rb index 69e31c7481f..fd991293ee9 100644 --- a/spec/features/merge_requests/widget_spec.rb +++ b/spec/features/merge_requests/widget_spec.rb @@ -219,4 +219,17 @@ describe 'Merge request', :js do expect(page).to have_field('remove-source-branch-input', disabled: true) end end + + context 'ongoing merge process' do + it 'shows Merging state' do + allow_any_instance_of(MergeRequest).to receive(:merge_ongoing?).and_return(true) + + visit project_merge_request_path(project, merge_request) + + wait_for_requests + + expect(page).not_to have_button('Merge') + expect(page).to have_content('This merge request is in the process of being merged') + end + end end diff --git a/spec/fixtures/api/schemas/entities/merge_request.json b/spec/fixtures/api/schemas/entities/merge_request.json index 7ffa82fc4bd..2f12b671dec 100644 --- a/spec/fixtures/api/schemas/entities/merge_request.json +++ b/spec/fixtures/api/schemas/entities/merge_request.json @@ -19,7 +19,6 @@ "human_time_estimate": { "type": ["integer", "null"] }, "human_total_time_spent": { "type": ["integer", "null"] }, "in_progress_merge_commit_sha": { "type": ["string", "null"] }, - "locked_at": { "type": ["string", "null"] }, "merge_error": { "type": ["string", "null"] }, "merge_commit_sha": { "type": ["string", "null"] }, "merge_params": { "type": ["object", "null"] }, @@ -94,7 +93,8 @@ "commit_change_content_path": { "type": "string" }, "remove_wip_path": { "type": "string" }, "commits_count": { "type": "integer" }, - "remove_source_branch": { "type": ["boolean", "null"] } + "remove_source_branch": { "type": ["boolean", "null"] }, + "merge_ongoing": { "type": "boolean" } }, "additionalProperties": false } diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_locked_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_locked_spec.js index fb2ef606604..237035648cf 100644 --- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_locked_spec.js +++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_locked_spec.js @@ -1,10 +1,10 @@ import Vue from 'vue'; -import lockedComponent from '~/vue_merge_request_widget/components/states/mr_widget_locked'; +import mergingComponent from '~/vue_merge_request_widget/components/states/mr_widget_merging'; -describe('MRWidgetLocked', () => { +describe('MRWidgetMerging', () => { describe('props', () => { it('should have props', () => { - const { mr } = lockedComponent.props; + const { mr } = mergingComponent.props; expect(mr.type instanceof Object).toBeTruthy(); expect(mr.required).toBeTruthy(); @@ -13,7 +13,7 @@ describe('MRWidgetLocked', () => { describe('template', () => { it('should have correct elements', () => { - const Component = Vue.extend(lockedComponent); + const Component = Vue.extend(mergingComponent); const mr = { targetBranchPath: '/branch-path', targetBranch: 'branch', @@ -24,7 +24,7 @@ describe('MRWidgetLocked', () => { }).$el; expect(el.classList.contains('mr-widget-body')).toBeTruthy(); - expect(el.innerText).toContain('it is locked'); + expect(el.innerText).toContain('This merge request is in the process of being merged'); expect(el.innerText).toContain('changes will be merged into'); expect(el.querySelector('.label-branch a').getAttribute('href')).toEqual(mr.targetBranchPath); expect(el.querySelector('.label-branch a').textContent).toContain(mr.targetBranch); diff --git a/spec/javascripts/vue_mr_widget/mock_data.js b/spec/javascripts/vue_mr_widget/mock_data.js index ad2f28b24f0..0795d0aaa82 100644 --- a/spec/javascripts/vue_mr_widget/mock_data.js +++ b/spec/javascripts/vue_mr_widget/mock_data.js @@ -20,7 +20,6 @@ export default { "human_time_estimate": null, "human_total_time_spent": null, "in_progress_merge_commit_sha": null, - "locked_at": null, "merge_commit_sha": "53027d060246c8f47e4a9310fb332aa52f221775", "merge_error": null, "merge_params": { diff --git a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js index 3a0c50b750f..669ee248bf1 100644 --- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js +++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js @@ -342,7 +342,7 @@ describe('mrWidgetOptions', () => { expect(comps['mr-widget-related-links']).toBeDefined(); expect(comps['mr-widget-merged']).toBeDefined(); expect(comps['mr-widget-closed']).toBeDefined(); - expect(comps['mr-widget-locked']).toBeDefined(); + expect(comps['mr-widget-merging']).toBeDefined(); expect(comps['mr-widget-failed-to-merge']).toBeDefined(); expect(comps['mr-widget-wip']).toBeDefined(); expect(comps['mr-widget-archived']).toBeDefined(); diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json index 469a014e4d2..4e631e13410 100644 --- a/spec/lib/gitlab/import_export/project.json +++ b/spec/lib/gitlab/import_export/project.json @@ -2534,7 +2534,6 @@ "iid": 9, "description": null, "position": 0, - "locked_at": null, "updated_by_id": null, "merge_error": null, "merge_params": { @@ -2983,7 +2982,6 @@ "iid": 8, "description": null, "position": 0, - "locked_at": null, "updated_by_id": null, "merge_error": null, "merge_params": { @@ -3267,7 +3265,6 @@ "iid": 7, "description": "Et commodi deserunt aspernatur vero rerum. Ut non dolorum alias in odit est libero. Voluptatibus eos in et vitae repudiandae facilis ex mollitia.", "position": 0, - "locked_at": null, "updated_by_id": null, "merge_error": null, "merge_params": { @@ -3551,7 +3548,6 @@ "iid": 6, "description": "Dicta magnam non voluptates nam dignissimos nostrum deserunt. Dolorum et suscipit iure quae doloremque. Necessitatibus saepe aut labore sed.", "position": 0, - "locked_at": null, "updated_by_id": null, "merge_error": null, "merge_params": { @@ -4241,7 +4237,6 @@ "iid": 5, "description": "Est eaque quasi qui qui. Similique voluptatem impedit iusto ratione reprehenderit. Itaque est illum ut nulla aut.", "position": 0, - "locked_at": null, "updated_by_id": null, "merge_error": null, "merge_params": { @@ -4789,7 +4784,6 @@ "iid": 4, "description": "Nam magnam odit velit rerum. Sapiente dolore sunt saepe debitis. Culpa maiores ut ad dolores dolorem et.", "position": 0, - "locked_at": null, "updated_by_id": null, "merge_error": null, "merge_params": { @@ -5288,7 +5282,6 @@ "iid": 3, "description": "Libero nesciunt mollitia quis odit eos vero quasi. Iure voluptatem ut sint pariatur voluptates ut aut. Laborum possimus unde illum ipsum eum.", "position": 0, - "locked_at": null, "updated_by_id": null, "merge_error": null, "merge_params": { @@ -5548,7 +5541,6 @@ "iid": 2, "description": "Ut dolor quia aliquid dolore et nisi. Est minus suscipit enim quaerat sapiente consequatur rerum. Eveniet provident consequatur dolor accusantium reiciendis.", "position": 0, - "locked_at": null, "updated_by_id": null, "merge_error": null, "merge_params": { @@ -6238,7 +6230,6 @@ "iid": 1, "description": "Eveniet nihil ratione veniam similique qui aut sapiente tempora. Sed praesentium iusto dignissimos possimus id repudiandae quo nihil. Qui doloremque autem et iure fugit.", "position": 0, - "locked_at": null, "updated_by_id": null, "merge_error": null, "merge_params": { diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 11f4c16ff96..4dce48f8079 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -145,7 +145,6 @@ MergeRequest: - iid - description - position -- locked_at - updated_by_id - merge_error - merge_params diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 3402c260f27..a1a3e70a7d2 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1369,6 +1369,32 @@ describe MergeRequest do end end + describe '#merge_ongoing?' do + it 'returns true when merge process is ongoing for merge_jid' do + merge_request = create(:merge_request, merge_jid: 'foo') + + allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']).and_return(1) + + expect(merge_request.merge_ongoing?).to be(true) + end + + it 'returns false when no merge process running for merge_jid' do + merge_request = build(:merge_request, merge_jid: 'foo') + + allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']).and_return(0) + + expect(merge_request.merge_ongoing?).to be(false) + end + + it 'returns false when merge_jid is nil' do + merge_request = build(:merge_request, merge_jid: nil) + + expect(Gitlab::SidekiqStatus).not_to receive(:num_running) + + expect(merge_request.merge_ongoing?).to be(false) + end + end + describe "#closed_without_fork?" do let(:project) { create(:project) } let(:fork_project) { create(:project, forked_from_project: project) } diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_entity_spec.rb index 18cd9e9c006..a2fd5b7daae 100644 --- a/spec/serializers/merge_request_entity_spec.rb +++ b/spec/serializers/merge_request_entity_spec.rb @@ -47,7 +47,7 @@ describe MergeRequestEntity do :cancel_merge_when_pipeline_succeeds_path, :create_issue_to_resolve_discussions_path, :source_branch_path, :target_branch_commits_path, - :target_branch_tree_path, :commits_count) + :target_branch_tree_path, :commits_count, :merge_ongoing) end it 'has email_patches_path' do diff --git a/spec/workers/merge_worker_spec.rb b/spec/workers/merge_worker_spec.rb index 303193bab9b..ee51000161a 100644 --- a/spec/workers/merge_worker_spec.rb +++ b/spec/workers/merge_worker_spec.rb @@ -27,4 +27,15 @@ describe MergeWorker do expect(source_project.repository.branch_names).not_to include('markdown') end end + + it 'persists merge_jid' do + merge_request = create(:merge_request, merge_jid: nil) + user = create(:user) + worker = described_class.new + + allow(worker).to receive(:jid) { '999' } + + expect { worker.perform(merge_request.id, user.id, {}) } + .to change { merge_request.reload.merge_jid }.from(nil).to('999') + end end diff --git a/spec/workers/stuck_merge_jobs_worker_spec.rb b/spec/workers/stuck_merge_jobs_worker_spec.rb new file mode 100644 index 00000000000..a5ad78393c9 --- /dev/null +++ b/spec/workers/stuck_merge_jobs_worker_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' + +describe StuckMergeJobsWorker do + describe 'perform' do + let(:worker) { described_class.new } + + context 'merge job identified as completed' do + it 'updates merge request to merged when locked but has merge_commit_sha' do + allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return(%w(123 456)) + mr_with_sha = create(:merge_request, :locked, merge_jid: '123', state: :locked, merge_commit_sha: 'foo-bar-baz') + mr_without_sha = create(:merge_request, :locked, merge_jid: '123', state: :locked, merge_commit_sha: nil) + + worker.perform + + expect(mr_with_sha.reload).to be_merged + expect(mr_without_sha.reload).to be_opened + end + + it 'updates merge request to opened when locked but has not been merged' do + allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return(%w(123)) + merge_request = create(:merge_request, :locked, merge_jid: '123', state: :locked) + + worker.perform + + expect(merge_request.reload).to be_opened + end + + it 'logs updated stuck merge job ids' do + allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return(%w(123 456)) + + create(:merge_request, :locked, merge_jid: '123') + create(:merge_request, :locked, merge_jid: '456') + + expect(Rails).to receive_message_chain(:logger, :info).with('Updated state of locked merge jobs. JIDs: 123, 456') + + worker.perform + end + end + + context 'merge job not identified as completed' do + it 'does not change merge request state when job is not completed yet' do + allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return([]) + + merge_request = create(:merge_request, :locked, merge_jid: '123') + + expect { worker.perform }.not_to change { merge_request.reload.state }.from('locked') + end + end + end +end -- cgit v1.2.3 From 16cffa97f64f55a16a50cf861e133021f31c828f Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 7 Aug 2017 16:34:57 -0300 Subject: Move locked_at removal to post-deployment migration --- app/models/merge_request.rb | 1 + ...70801000000_remove_locked_at_column_from_merge_requests.rb | 11 ----------- ...70807160457_remove_locked_at_column_from_merge_requests.rb | 11 +++++++++++ db/schema.rb | 2 +- 4 files changed, 13 insertions(+), 12 deletions(-) delete mode 100644 db/migrate/20170801000000_remove_locked_at_column_from_merge_requests.rb create mode 100644 db/post_migrate/20170807160457_remove_locked_at_column_from_merge_requests.rb diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e0c779b4fbc..e83b11f7668 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -8,6 +8,7 @@ class MergeRequest < ActiveRecord::Base include CreatedAtFilterable ignore_column :position + ignore_column :locked_at belongs_to :target_project, class_name: "Project" belongs_to :source_project, class_name: "Project" diff --git a/db/migrate/20170801000000_remove_locked_at_column_from_merge_requests.rb b/db/migrate/20170801000000_remove_locked_at_column_from_merge_requests.rb deleted file mode 100644 index ea3d1fb3e02..00000000000 --- a/db/migrate/20170801000000_remove_locked_at_column_from_merge_requests.rb +++ /dev/null @@ -1,11 +0,0 @@ -class RemoveLockedAtColumnFromMergeRequests < ActiveRecord::Migration - DOWNTIME = false - - def up - remove_column :merge_requests, :locked_at - end - - def down - add_column :merge_requests, :locked_at, :datetime_with_timezone - end -end diff --git a/db/post_migrate/20170807160457_remove_locked_at_column_from_merge_requests.rb b/db/post_migrate/20170807160457_remove_locked_at_column_from_merge_requests.rb new file mode 100644 index 00000000000..3949cf8261a --- /dev/null +++ b/db/post_migrate/20170807160457_remove_locked_at_column_from_merge_requests.rb @@ -0,0 +1,11 @@ +class RemoveLockedAtColumnFromMergeRequests < ActiveRecord::Migration + DOWNTIME = false + + def up + remove_column :merge_requests, :locked_at + end + + def down + # nothing to do to recover the values + end +end diff --git a/db/schema.rb b/db/schema.rb index 4816e1aefab..9fdf064a225 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170803130232) do +ActiveRecord::Schema.define(version: 20170807160457) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" -- cgit v1.2.3 From 43b03e9c5a6918a6106819d8819ac1088c2e7af9 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 7 Aug 2017 18:09:53 -0300 Subject: Re-add column locked_at on migration rollback --- .../20170807160457_remove_locked_at_column_from_merge_requests.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/post_migrate/20170807160457_remove_locked_at_column_from_merge_requests.rb b/db/post_migrate/20170807160457_remove_locked_at_column_from_merge_requests.rb index 3949cf8261a..ea3d1fb3e02 100644 --- a/db/post_migrate/20170807160457_remove_locked_at_column_from_merge_requests.rb +++ b/db/post_migrate/20170807160457_remove_locked_at_column_from_merge_requests.rb @@ -6,6 +6,6 @@ class RemoveLockedAtColumnFromMergeRequests < ActiveRecord::Migration end def down - # nothing to do to recover the values + add_column :merge_requests, :locked_at, :datetime_with_timezone end end -- cgit v1.2.3 From a0c22d1edafbd87d59dbf01acd610da97ec87912 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 7 Aug 2017 20:04:16 -0300 Subject: Exclude merge_jid on Import/Export attribute configuration --- lib/gitlab/import_export/import_export.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index c8ad3a7a5e0..c5c05bfe2fb 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -101,6 +101,7 @@ excluded_attributes: merge_requests: - :milestone_id - :ref_fetched + - :merge_jid award_emoji: - :awardable_id statuses: -- cgit v1.2.3