diff --git a/app/models/group.rb b/app/models/group.rb index c38ddbdf6fb..ea9091455ac 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -172,6 +172,10 @@ class Group < Namespace notification_settings(hierarchy_order: hierarchy_order).where(user: user) end + def packages_feature_enabled? + ::Gitlab.config.packages.enabled + end + def notification_email_for(user) # Finds the closest notification_setting with a `notification_email` notification_settings = notification_settings_for(user, hierarchy_order: :asc) diff --git a/app/models/project.rb b/app/models/project.rb index 3aa0db56404..32f9b580b47 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -838,6 +838,10 @@ class Project < ApplicationRecord auto_devops_config[:scope] != :project && !auto_devops_config[:status] end + def has_packages?(package_type) + packages.where(package_type: package_type).exists? + end + def first_auto_devops_config return namespace.first_auto_devops_config if auto_devops&.enabled.nil? diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb index fbbf4ce8baf..885f5c1edd0 100644 --- a/app/services/groups/transfer_service.rb +++ b/app/services/groups/transfer_service.rb @@ -46,6 +46,21 @@ module Groups raise_transfer_error(:namespace_with_same_path) if namespace_with_same_path? raise_transfer_error(:group_contains_images) if group_projects_contain_registry_images? raise_transfer_error(:cannot_transfer_to_subgroup) if transfer_to_subgroup? + raise_transfer_error(:group_contains_npm_packages) if group_with_npm_packages? + end + + def group_with_npm_packages? + return false unless group.packages_feature_enabled? + + npm_packages = ::Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute + + return true if different_root_ancestor? && npm_packages.exists? + + false + end + + def different_root_ancestor? + group.root_ancestor != new_parent_group&.root_ancestor end def group_is_already_root? @@ -133,7 +148,8 @@ module Groups same_parent_as_current: s_('TransferGroup|Group is already associated to the parent group.'), invalid_policies: s_("TransferGroup|You don't have enough permissions."), group_contains_images: s_('TransferGroup|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again.'), - cannot_transfer_to_subgroup: s_('TransferGroup|Cannot transfer group to one of its subgroup.') + cannot_transfer_to_subgroup: s_('TransferGroup|Cannot transfer group to one of its subgroup.'), + group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.') }.freeze end end diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 948540619ae..81393681dc0 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -17,6 +17,8 @@ module Groups return false unless valid_share_with_group_lock_change? + return false unless valid_path_change_with_npm_packages? + before_assignment_hook(group, params) group.assign_attributes(params) @@ -36,6 +38,20 @@ module Groups private + def valid_path_change_with_npm_packages? + return true unless group.packages_feature_enabled? + return true if params[:path].blank? + return true if !group.has_parent? && group.path == params[:path] + + npm_packages = ::Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute + if npm_packages.exists? + group.errors.add(:path, s_('GroupSettings|cannot change when group contains projects with NPM packages')) + return + end + + true + end + def before_assignment_hook(group, params) # overridden in EE end diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index 60e5b7e2639..0fb70feec86 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -55,10 +55,18 @@ module Projects raise TransferError.new(s_('TransferProject|Project cannot be transferred, because tags are present in its container registry')) end + if project.has_packages?(:npm) && !new_namespace_has_same_root?(project) + raise TransferError.new(s_("TransferProject|Root namespace can't be updated if project has NPM packages")) + end + attempt_transfer_transaction end # rubocop: enable CodeReuse/ActiveRecord + def new_namespace_has_same_root?(project) + new_namespace.root_ancestor == project.namespace.root_ancestor + end + def attempt_transfer_transaction Project.transaction do project.expire_caches_before_rename(@old_path) diff --git a/doc/administration/packages/container_registry.md b/doc/administration/packages/container_registry.md index 44f1d075a5e..4933a9dab5d 100644 --- a/doc/administration/packages/container_registry.md +++ b/doc/administration/packages/container_registry.md @@ -693,6 +693,35 @@ notifications: backoff: 1000 ``` +## Run the Cleanup policy now + +To reduce the amount of [Container Registry disk space used by a given project](../troubleshooting/gitlab_rails_cheat_sheet.md#registry-disk-space-usage-by-project), +administrators can clean up image tags +and [run garbage collection](#container-registry-garbage-collection). + +To remove image tags by running the cleanup policy, run the following commands in the +[GitLab Rails console](../troubleshooting/navigating_gitlab_via_rails_console.md): + +```ruby +# Numeric ID of the project whose container registry should be cleaned up +P = + +# Numeric ID of a developer, maintainer or owner in that project +U = + +# Get required details / objects +user = User.find_by_id(U) +project = Project.find_by_id(P) +repo = ContainerRepository.find_by(project_id: P) +policy = ContainerExpirationPolicy.find_by(project_id: P) + +# Start the tag cleanup +Projects::ContainerRepository::CleanupTagsService.new(project, user, policy.attributes.except("created_at", "updated_at")).execute(repo) +``` + +NOTE: **Note:** +You can also [run cleanup on a schedule](../../user/packages/container_registry/index.md#cleanup-policy). + ## Container Registry garbage collection NOTE: **Note:** diff --git a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md index 8e9749fb239..097374e1801 100644 --- a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md +++ b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md @@ -689,10 +689,10 @@ end As a GitLab administrator, you may need to reduce disk space consumption. A common culprit is Docker Registry images that are no longer in use. To find the storage broken down by each project, run the following in the -GitLab Rails console: +[GitLab Rails console](../troubleshooting/navigating_gitlab_via_rails_console.md): ```ruby -projects_and_size = [] +projects_and_size = [["project_id", "creator_id", "registry_size_bytes", "project path"]] # You need to specify the projects that you want to look through. You can get these in any manner. projects = Project.last(100) @@ -707,17 +707,21 @@ projects.each do |p| end if project_total_size > 0 - projects_and_size << [p.full_path,project_total_size] + projects_and_size << [p.project_id, p.creator.id, project_total_size, p.full_path] end end # projects_and_size is filled out now # maybe print it as comma separated output? projects_and_size.each do |ps| - puts "%s,%s" % ps + puts "%s,%s,%s,%s" % ps end ``` +### Run the Cleanup policy now + +Find this content in the [Container Registry troubleshooting docs](../packages/container_registry.md#run-the-cleanup-policy-now). + ## Sidekiq This content has been moved to the [Troubleshooting Sidekiq docs](./sidekiq.md). diff --git a/doc/user/project/merge_requests/img/comment-on-any-diff-line.png b/doc/user/project/merge_requests/img/comment-on-any-diff-line.png index 5b9844bf02f..b5c4b12a6a7 100644 Binary files a/doc/user/project/merge_requests/img/comment-on-any-diff-line.png and b/doc/user/project/merge_requests/img/comment-on-any-diff-line.png differ diff --git a/doc/user/project/merge_requests/img/multiline-comment-highlighted.png b/doc/user/project/merge_requests/img/multiline-comment-highlighted.png new file mode 100644 index 00000000000..c254af06c8f Binary files /dev/null and b/doc/user/project/merge_requests/img/multiline-comment-highlighted.png differ diff --git a/doc/user/project/merge_requests/img/multiline-comment-saved.png b/doc/user/project/merge_requests/img/multiline-comment-saved.png new file mode 100644 index 00000000000..6c44ee9f777 Binary files /dev/null and b/doc/user/project/merge_requests/img/multiline-comment-saved.png differ diff --git a/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md b/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md index 91ca48f85d5..d87e9f3daa8 100644 --- a/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md +++ b/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md @@ -141,10 +141,52 @@ whitespace changes. > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/13950) in GitLab 11.5. GitLab provides a way of leaving comments in any part of the file being changed -in a Merge Request. To do so, click the **...** button in the gutter of the Merge Request diff UI to expand the diff lines and leave a comment, just as you would for a changed line. +in a Merge Request. To do so, click the **{comment}** **comment** icon in the gutter of the Merge Request diff UI to expand the diff lines and leave a comment, just as you would for a changed line. ![Comment on any diff file line](img/comment-on-any-diff-line.png) +### Commenting on multiple lines + +> - [Introduced](https://gitlab.com/gitlab-org/ux-research/-/issues/870) in GitLab 13.2. +> - It's deployed behind a feature flag, enabled by default. +> - It's enabled on GitLab.com. +> - It's able to be enabled or disabled per-project +> - It's recommended for production use. +> - For GitLab self-managed instances, GitLab administrators can opt to [enable it](#enable-or-disable-multiline-comments-core-only). **(CORE ONLY)** + +GitLab provides a way to select which lines of code a comment refers to. After starting a comment +a dropdown selector is shown to select the first line that this comment refers to. +The last line is the line that the comment icon was initially clicked on. + +New comments default to single line comments by having the first and last lines +the same. Selecting a different starting line turns this into a multiline comment. + +![Multiline comment selection highlighted](img/multiline-comment-highlighted.png) + +Once a multiline comment is saved the lines of code pertaining to that comment are listed directly +above it. + +![Multiline comment selection displayed above comment](img/multiline-comment-saved.png) + +### Enable or disable multiline comments **(CORE ONLY)** + +The multiline comments feature is under development but ready for production use. +It is deployed behind a feature flag that is **enabled by default**. +[GitLab administrators with access to the GitLab Rails console](../../../administration/feature_flags.md) +can opt to disable it for your instance. + +To disable it: + +```ruby +Feature.disable(:multiline_comments) +``` + +To enable it: + +```ruby +Feature.enable(:multiline_comments) +``` + ## Pipeline status in merge requests widgets If you've set up [GitLab CI/CD](../../../ci/README.md) in your project, diff --git a/spec/lib/banzai/reference_parser/base_parser_spec.rb b/spec/lib/banzai/reference_parser/base_parser_spec.rb index 0eea51262ba..5ab76b2c68b 100644 --- a/spec/lib/banzai/reference_parser/base_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/base_parser_spec.rb @@ -8,13 +8,14 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do let(:user) { create(:user) } let(:project) { create(:project, :public) } let(:context) { Banzai::RenderContext.new(project, user) } - - subject do - klass = Class.new(described_class) do + let(:parser_class) do + Class.new(described_class) do self.reference_type = :foo end + end - klass.new(context) + subject do + parser_class.new(context) end describe '.reference_type=' do @@ -43,12 +44,20 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do let(:link) { empty_html_link } context 'when the link has a data-project attribute' do - it 'checks if user can read the resource' do + before do link['data-project'] = project.id.to_s + end - expect(subject).to receive(:can_read_reference?).with(user, project, link) + it 'includes the link if can_read_reference? returns true' do + expect(subject).to receive(:can_read_reference?).with(user, project, link).and_return(true) - subject.nodes_visible_to_user(user, [link]) + expect(subject.nodes_visible_to_user(user, [link])).to contain_exactly(link) + end + + it 'excludes the link if can_read_reference? returns false' do + expect(subject).to receive(:can_read_reference?).with(user, project, link).and_return(false) + + expect(subject.nodes_visible_to_user(user, [link])).to be_empty end end @@ -178,58 +187,56 @@ RSpec.describe Banzai::ReferenceParser::BaseParser do it 'gathers the references for every node matching the reference type' do dummy = Class.new(described_class) do self.reference_type = :test + + def gather_references(nodes) + nodes + end end - instance = dummy.new(Banzai::RenderContext.new(project, user)) - document = Nokogiri::HTML.fragment('') + instance = dummy.new(context) + document_a = Nokogiri::HTML.fragment(<<-FRAG) + one + two + three + FRAG + document_b = Nokogiri::HTML.fragment(<<-FRAG) + four + FRAG + document_c = Nokogiri::HTML.fragment('') - expect(instance).to receive(:gather_references) - .with([document.children[1]]) - .and_return([user]) - - expect(instance.process([document])).to eq([user]) + expect(instance.process([document_a, document_b, document_c])) + .to contain_exactly(document_a.css('a')[1], document_b.css('a')[0]) end end describe '#gather_references' do - let(:link) { double(:link) } + let(:nodes) { (1..10).map { |n| double(:link, id: n) } } - it 'does not process links a user can not reference' do - expect(subject).to receive(:nodes_user_can_reference) - .with(user, [link]) - .and_return([]) + let(:parser_class) do + Class.new(described_class) do + def nodes_user_can_reference(_user, nodes) + nodes.select { |n| n.id.even? } + end - expect(subject).to receive(:referenced_by).with([]) + def nodes_visible_to_user(_user, nodes) + nodes.select { |n| n.id > 5 } + end - subject.gather_references([link]) + def referenced_by(nodes) + nodes.map(&:id) + end + end end - it 'does not process links a user can not see' do - expect(subject).to receive(:nodes_user_can_reference) - .with(user, [link]) - .and_return([link]) - - expect(subject).to receive(:nodes_visible_to_user) - .with(user, [link]) - .and_return([]) - - expect(subject).to receive(:referenced_by).with([]) - - subject.gather_references([link]) + it 'returns referenceable and visible objects, alongside nodes that are referenceable but not visible' do + expect(subject.gather_references(nodes)).to match( + visible: contain_exactly(6, 8, 10), + not_visible: match_array(nodes.select { |n| n.id.even? && n.id <= 5 }) + ) end - it 'returns the references if a user can reference and see a link' do - expect(subject).to receive(:nodes_user_can_reference) - .with(user, [link]) - .and_return([link]) - - expect(subject).to receive(:nodes_visible_to_user) - .with(user, [link]) - .and_return([link]) - - expect(subject).to receive(:referenced_by).with([link]) - - subject.gather_references([link]) + it 'is always empty if the input is empty' do + expect(subject.gather_references([])) .to match(visible: be_empty, not_visible: be_empty) end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 8fdda241719..722bf77faed 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -6154,6 +6154,46 @@ RSpec.describe Project do end end + describe '#has_packages?' do + let(:project) { create(:project, :public) } + + subject { project.has_packages?(package_type) } + + shared_examples 'returning true examples' do + let!(:package) { create("#{package_type}_package", project: project) } + + it { is_expected.to be true } + end + + shared_examples 'returning false examples' do + it { is_expected.to be false } + end + + context 'with maven packages' do + it_behaves_like 'returning true examples' do + let(:package_type) { :maven } + end + end + + context 'with npm packages' do + it_behaves_like 'returning true examples' do + let(:package_type) { :npm } + end + end + + context 'with conan packages' do + it_behaves_like 'returning true examples' do + let(:package_type) { :conan } + end + end + + context 'with no package type' do + it_behaves_like 'returning false examples' do + let(:package_type) { nil } + end + end + end + describe '#environments_for_scope' do let_it_be(:project, reload: true) { create(:project) } diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index fa254bba6a9..d21c59e24c8 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -8,6 +8,74 @@ RSpec.describe Groups::TransferService do let!(:group_member) { create(:group_member, :owner, group: group, user: user) } let(:transfer_service) { described_class.new(group, user) } + context 'handling packages' do + let(:group) { create(:group, :public) } + let(:project) { create(:project, :public, namespace: group) } + let(:new_group) { create(:group, :public) } + + before do + group.add_owner(user) + new_group&.add_owner(user) + end + + context 'with an npm package' do + before do + create(:npm_package, project: project) + end + + shared_examples 'transfer not allowed' do + it 'does not allow transfer when there is a root namespace change' do + transfer_service.execute(new_group) + + expect(transfer_service.error).to eq('Transfer failed: Group contains projects with NPM packages.') + end + end + + it_behaves_like 'transfer not allowed' + + context 'with a project within subgroup' do + let(:root_group) { create(:group) } + let(:group) { create(:group, parent: root_group) } + + before do + root_group.add_owner(user) + end + + it_behaves_like 'transfer not allowed' + + context 'without a root namespace change' do + let(:new_group) { create(:group, parent: root_group) } + + it 'allows transfer' do + transfer_service.execute(new_group) + + expect(transfer_service.error).not_to be + expect(group.parent).to eq(new_group) + end + end + + context 'when transferring a group into a root group' do + let(:new_group) { nil } + + it_behaves_like 'transfer not allowed' + end + end + end + + context 'without an npm package' do + context 'when transferring a group into a root group' do + let(:group) { create(:group, parent: create(:group)) } + + it 'allows transfer' do + transfer_service.execute(nil) + + expect(transfer_service.error).not_to be + expect(group.parent).to be_nil + end + end + end + end + shared_examples 'ensuring allowed transfer for a group' do context "when there's an exception on GitLab shell directories" do let(:new_parent_group) { create(:group, :public) } diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 25c79d9e600..1e6a8d53354 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -9,6 +9,50 @@ RSpec.describe Groups::UpdateService do let!(:public_group) { create(:group, :public) } describe "#execute" do + shared_examples 'with packages' do + before do + group.add_owner(user) + end + + context 'with npm packages' do + let!(:package) { create(:npm_package, project: project) } + + it 'does not allow a path update' do + expect(update_group(group, user, path: 'updated')).to be false + expect(group.errors[:path]).to include('cannot change when group contains projects with NPM packages') + end + + it 'allows name update' do + expect(update_group(group, user, name: 'Updated')).to be true + expect(group.errors).to be_empty + expect(group.name).to eq('Updated') + end + end + end + + context 'with project' do + let!(:group) { create(:group, :public) } + let(:project) { create(:project, namespace: group) } + + it_behaves_like 'with packages' + + context 'located in a subgroup' do + let(:subgroup) { create(:group, parent: group) } + let!(:project) { create(:project, namespace: subgroup) } + + before do + subgroup.add_owner(user) + end + + it_behaves_like 'with packages' + + it 'does allow a path update if there is not a root namespace change' do + expect(update_group(subgroup, user, path: 'updated')).to be true + expect(subgroup.errors[:path]).to be_empty + end + end + end + context "project visibility_level validation" do context "public group with public projects" do let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } @@ -238,4 +282,8 @@ RSpec.describe Groups::UpdateService do end end end + + def update_group(group, user, opts) + Groups::UpdateService.new(group, user, opts).execute + end end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 72426a6f6ec..3362b333c6e 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -11,6 +11,39 @@ RSpec.describe Projects::TransferService do subject(:execute_transfer) { described_class.new(project, user).execute(group) } + context 'with npm packages' do + before do + group.add_owner(user) + end + + subject(:transfer_service) { described_class.new(project, user) } + + let!(:package) { create(:npm_package, project: project) } + + context 'with a root namespace change' do + it 'does not allow the transfer' do + expect(transfer_service.execute(group)).to be false + expect(project.errors[:new_namespace]).to include("Root namespace can't be updated if project has NPM packages") + end + end + + context 'without a root namespace change' do + let(:root) { create(:group) } + let(:group) { create(:group, parent: root) } + let(:other_group) { create(:group, parent: root) } + let(:project) { create(:project, :repository, namespace: group) } + + before do + other_group.add_owner(user) + end + + it 'does allow the transfer' do + expect(transfer_service.execute(other_group)).to be true + expect(project.errors[:new_namespace]).to be_empty + end + end + end + context 'namespace -> namespace' do before do allow_next_instance_of(Gitlab::UploadsTransfer) do |service|