From 518897664a2887f189c01f6efacff955d9ed7bef Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Fri, 26 Jul 2024 16:57:58 +0100 Subject: [PATCH] Add MR mergeability check for locked LFS files This change introduces a new check. This check will prevent merging MRs which include locked files. We also have a check which runs when a user pushes code that prevents pushing files that are locked by other users. This change will allow us to enable the fix for https://gitlab.com/gitlab-org/gitlab/-/issues/23625+ Related to https://gitlab.com/gitlab-org/gitlab/-/issues/428247+ Changelog: added --- .../components/checks/constants.js | 1 + .../detailed_merge_status_enum.rb | 3 + app/models/lfs_file_lock.rb | 3 + app/models/merge_request.rb | 7 +- .../check_lfs_file_locks_service.rb | 59 ++++++++ .../locked_lfs_files_mergeability_check.yml | 9 ++ doc/api/graphql/reference/index.md | 2 + doc/api/merge_requests.md | 1 + locale/gitlab.pot | 3 + spec/models/merge_request_spec.rb | 8 +- .../check_lfs_file_locks_service_spec.rb | 127 ++++++++++++++++++ 11 files changed, 217 insertions(+), 6 deletions(-) create mode 100644 app/services/merge_requests/mergeability/check_lfs_file_locks_service.rb create mode 100644 config/feature_flags/beta/locked_lfs_files_mergeability_check.yml create mode 100644 spec/services/merge_requests/mergeability/check_lfs_file_locks_service_spec.rb diff --git a/app/assets/javascripts/vue_merge_request_widget/components/checks/constants.js b/app/assets/javascripts/vue_merge_request_widget/components/checks/constants.js index 91814a93000504..e1cffbcbef285e 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/checks/constants.js +++ b/app/assets/javascripts/vue_merge_request_widget/components/checks/constants.js @@ -24,4 +24,5 @@ export const FAILURE_REASONS = { jira_association_missing: __('Either the title or description must reference a Jira issue.'), requested_changes: __('The change requests must be completed or resolved.'), approvals_syncing: __('The merge request approvals are currently syncing.'), + locked_lfs_files: __('All LFS files must be unlocked.'), }; diff --git a/app/graphql/types/merge_requests/detailed_merge_status_enum.rb b/app/graphql/types/merge_requests/detailed_merge_status_enum.rb index c612b297213fe2..190417bdaeb2da 100644 --- a/app/graphql/types/merge_requests/detailed_merge_status_enum.rb +++ b/app/graphql/types/merge_requests/detailed_merge_status_enum.rb @@ -57,6 +57,9 @@ class DetailedMergeStatusEnum < BaseEnum value 'APPROVALS_SYNCING', value: :approvals_syncing, description: 'Merge request approvals currently syncing.' + value 'LOCKED_LFS_FILES', + value: :locked_lfs_files, + description: 'Merge request includes locked LFS files.' end end end diff --git a/app/models/lfs_file_lock.rb b/app/models/lfs_file_lock.rb index 624b1d02e1a61c..c442760dce1038 100644 --- a/app/models/lfs_file_lock.rb +++ b/app/models/lfs_file_lock.rb @@ -4,6 +4,9 @@ class LfsFileLock < ApplicationRecord belongs_to :project belongs_to :user + scope :for_paths, ->(paths) { where(path: paths) } + scope :not_for_users, ->(user_ids) { where.not(user_id: user_ids) } + validates :project_id, :user_id, :path, presence: true def can_be_unlocked_by?(current_user, forced = false) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ec87b6b36b1bb7..c4959cd6ee1bb7 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1273,7 +1273,8 @@ def skipped_mergeable_checks(options = {}) skip_discussions_check: merge_when_checks_pass_strat, skip_external_status_check: merge_when_checks_pass_strat, skip_requested_changes_check: merge_when_checks_pass_strat, - skip_jira_check: merge_when_checks_pass_strat + skip_jira_check: merge_when_checks_pass_strat, + skip_locked_lfs_files_check: merge_when_checks_pass_strat } end @@ -1286,6 +1287,7 @@ def skipped_mergeable_checks(options = {}) # skip_external_status_check # skip_requested_changes_check # skip_jira_check + # skip_locked_lfs_files_check def mergeable?(check_mergeability_retry_lease: false, skip_rebase_check: false, **mergeable_state_check_params) return false unless mergeable_state?(**mergeable_state_check_params) @@ -1302,7 +1304,8 @@ def self.mergeable_state_checks ::MergeRequests::Mergeability::CheckDraftStatusService, ::MergeRequests::Mergeability::CheckCommitsStatusService, ::MergeRequests::Mergeability::CheckDiscussionsStatusService, - ::MergeRequests::Mergeability::CheckCiStatusService + ::MergeRequests::Mergeability::CheckCiStatusService, + ::MergeRequests::Mergeability::CheckLfsFileLocksService ] end diff --git a/app/services/merge_requests/mergeability/check_lfs_file_locks_service.rb b/app/services/merge_requests/mergeability/check_lfs_file_locks_service.rb new file mode 100644 index 00000000000000..f87f26f41c2521 --- /dev/null +++ b/app/services/merge_requests/mergeability/check_lfs_file_locks_service.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +module MergeRequests + module Mergeability + class CheckLfsFileLocksService < CheckBaseService + include ::Gitlab::Utils::StrongMemoize + + identifier :locked_lfs_files + description <<~DESC.chomp + Checks whether the merge request contains locked LFS files that are locked by users other than the merge request author + DESC + + CACHE_KEY = 'merge_request:%{id}:%{sha}:lfs_file_locks_mergeability:%{epoch}' + + def execute + return inactive if check_inactive? + return failure if contains_locked_lfs_files? + + success + end + + def skip? + params[:skip_locked_lfs_files_check].present? + end + + def cacheable? + true + end + + def cache_key + # If the feature is disabled we will return inactive so we don't need to + # link the cache key to a specific MR. + return 'inactive_lfs_file_locks_mergeability_check' if check_inactive? + + # Cache is linked to a specific MR + id = merge_request.id + # Cache is invalidated when new changes are added + sha = merge_request.diff_head_sha + # Cache is invalidated when lfs_file_locks are added or removed + epoch = project.lfs_file_locks_changed_epoch + + format(CACHE_KEY, id: id, sha: sha, epoch: epoch) + end + + private + + delegate :project, :author_id, :changed_paths, to: :merge_request + + def contains_locked_lfs_files? + project.lfs_file_locks.for_paths(changed_paths.map(&:path)).not_for_users(author_id).exists? + end + + def check_inactive? + Feature.disabled?(:locked_lfs_files_mergeability_check, project) || !project.lfs_enabled? + end + strong_memoize_attr :check_inactive? + end + end +end diff --git a/config/feature_flags/beta/locked_lfs_files_mergeability_check.yml b/config/feature_flags/beta/locked_lfs_files_mergeability_check.yml new file mode 100644 index 00000000000000..f95e678293b92a --- /dev/null +++ b/config/feature_flags/beta/locked_lfs_files_mergeability_check.yml @@ -0,0 +1,9 @@ +--- +name: locked_lfs_files_mergeability_check +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/428247 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/158773 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/474485 +milestone: '17.3' +group: group::source code +type: beta +default_enabled: false diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index ff0dffd0ec6904..1c937aa0b6fbea 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -35144,6 +35144,7 @@ Detailed representation of whether a GitLab merge request can be merged. | `DRAFT_STATUS` | Merge request must not be draft before merging. | | `EXTERNAL_STATUS_CHECKS` | Status checks must pass. | | `JIRA_ASSOCIATION` | Either the title or description must reference a Jira issue. | +| `LOCKED_LFS_FILES` | Merge request includes locked LFS files. | | `MERGEABLE` | Branch can be merged. | | `NEED_REBASE` | Merge request needs to be rebased. | | `NOT_APPROVED` | Merge request must be approved before merging. | @@ -35919,6 +35920,7 @@ Representation of mergeability check identifier. | `DISCUSSIONS_NOT_RESOLVED` | Checks whether the merge request has open discussions. | | `DRAFT_STATUS` | Checks whether the merge request is draft. | | `JIRA_ASSOCIATION_MISSING` | Checks whether the title or description references a Jira issue. | +| `LOCKED_LFS_FILES` | Checks whether the merge request contains locked LFS files that are locked by users other than the merge request author. | | `MERGE_REQUEST_BLOCKED` | Checks whether the merge request is blocked. | | `NEED_REBASE` | Checks whether the merge request needs to be rebased. | | `NOT_APPROVED` | Checks whether the merge request is approved. | diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index f8086c2a410e10..8e406cd4e9288f 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -909,6 +909,7 @@ Use `detailed_merge_status` instead of `merge_status` to account for all potenti - `not_open`: The merge request must be open before merge. - `requested_changes`: The merge request has reviewers who have requested changes. - `unchecked`: Git has not yet tested if a valid merge is possible. + - `locked_lfs_files`: LFS files locked by other users must be unlocked before merge. ### Preparation steps diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 68e7692aca37bf..36fbb507d7c9f2 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5383,6 +5383,9 @@ msgstr "" msgid "All GitLab" msgstr "" +msgid "All LFS files must be unlocked." +msgstr "" + msgid "All Members" msgstr "" diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index e3c86fdd0754f9..f97a2c20b1eb56 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -3947,10 +3947,10 @@ def set_compare(merge_request) let(:options) { { auto_merge_requested: true, auto_merge_strategy: auto_merge_strategy } } where(:auto_merge_strategy, :skip_approved_check, :skip_draft_check, :skip_blocked_check, - :skip_discussions_check, :skip_external_status_check, :skip_requested_changes_check, :skip_jira_check) do - '' | false | false | false | false | false | false | false - AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS | false | false | false | false | false | false | false - AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS | true | true | true | true | true | true | true + :skip_discussions_check, :skip_external_status_check, :skip_requested_changes_check, :skip_jira_check, :skip_locked_lfs_files_check) do + '' | false | false | false | false | false | false | false | false + AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS | false | false | false | false | false | false | false | false + AutoMergeService::STRATEGY_MERGE_WHEN_CHECKS_PASS | true | true | true | true | true | true | true | true end with_them do diff --git a/spec/services/merge_requests/mergeability/check_lfs_file_locks_service_spec.rb b/spec/services/merge_requests/mergeability/check_lfs_file_locks_service_spec.rb new file mode 100644 index 00000000000000..ea25cce4f6c930 --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_lfs_file_locks_service_spec.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckLfsFileLocksService, feature_category: :code_review_workflow do + subject(:check_lfs_file_locks) { described_class.new(merge_request: merge_request, params: params) } + + let_it_be(:project) { create(:project) } + let_it_be(:merge_request) { build(:merge_request, source_project: project) } + let(:params) { { skip_locked_lfs_files_check: skip_check } } + let(:skip_check) { false } + let(:feature_flag_enabled) { true } + let(:lfs_enabled) { true } + + before do + stub_feature_flags(locked_lfs_files_mergeability_check: feature_flag_enabled) + allow(project).to receive(:lfs_enabled?).and_return(lfs_enabled) + end + + it_behaves_like 'mergeability check service', :locked_lfs_files, <<~DESC.chomp + Checks whether the merge request contains locked LFS files that are locked by users other than the merge request author + DESC + + describe '#execute' do + subject(:execute) { check_lfs_file_locks.execute } + + context 'when lfs is enabled' do + let(:only_allow_merge_if_pipeline_succeeds) { true } + let(:changed_path) { instance_double('Gitlab::Git::ChangedPath', path: 'README.md') } + + before do + allow(merge_request).to receive(:changed_paths).and_return([changed_path]) + end + + context 'when there are no lfs files locks for this project' do + it 'returns a check result with status success' do + expect(execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when there are lfs files locked by the merge request author' do + let(:user) { create(:user) } + + before do + allow(merge_request).to receive(:author_id).and_return(user.id) + create(:lfs_file_lock, project: project, path: changed_path.path, user: user) + end + + it 'returns a check result with status success' do + expect(execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when there are lfs files locked by another user' do + before do + allow(merge_request).to receive(:author_id).and_return(0) + create(:lfs_file_lock, project: project, path: changed_path.path) + end + + it 'returns a check result with status failure' do + expect(execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + end + + context 'when lfs is not enabled' do + let(:lfs_enabled) { false } + + it 'returns a check result with inactive status' do + expect(execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::INACTIVE_STATUS + end + end + end + + describe '#skip?' do + subject(:skip) { check_lfs_file_locks.skip? } + + context 'when skip check is true' do + let(:skip_check) { true } + + it { expect(skip).to eq(true) } + end + + context 'when skip check is false' do + let(:skip_check) { false } + + it { expect(skip).to eq(false) } + end + end + + describe '#cacheable?' do + subject(:cacheable) { check_lfs_file_locks.cacheable? } + + it { expect(cacheable).to eq(true) } + end + + describe '#cache_key' do + subject(:cache_key) { check_lfs_file_locks.cache_key } + + context 'when the feature flag is enabled' do + let(:id) { 'id' } + let(:sha) { 'sha' } + let(:epoch) { 'epoch' } + let(:expected_cache_key) { format(described_class::CACHE_KEY, id: id, sha: sha, epoch: epoch) } + + before do + allow(merge_request).to receive(:id).and_return(id) + allow(merge_request).to receive(:diff_head_sha).and_return(sha) + allow(project).to receive(:lfs_file_locks_changed_epoch).and_return(epoch) + end + + it { expect(cache_key).to eq(expected_cache_key) } + end + + context 'when the feature flag is disabled' do + let(:feature_flag_enabled) { false } + + it { expect(cache_key).to eq('inactive_lfs_file_locks_mergeability_check') } + end + + context 'when lfs is disabled' do + let(:lfs_enabled) { false } + + it { expect(cache_key).to eq('inactive_lfs_file_locks_mergeability_check') } + end + end +end -- GitLab