From 6e4e1050d9dba2b7b2523fdd1768823ab85feef4 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Thu, 20 Aug 2020 18:42:06 +0000 Subject: Add latest changes from gitlab-org/gitlab@13-3-stable-ee --- .../projects/alerting/notify_service_spec.rb | 18 +- spec/services/projects/cleanup_service_spec.rb | 2 +- .../delete_tags_service_spec.rb | 293 +++++---------------- .../gitlab/delete_tags_service_spec.rb | 56 ++++ .../third_party/delete_tags_service_spec.rb | 89 +++++++ spec/services/projects/create_service_spec.rb | 6 + spec/services/projects/fork_service_spec.rb | 69 +++-- .../projects/operations/update_service_spec.rb | 1 + .../prometheus/alerts/notify_service_spec.rb | 56 +--- .../projects/propagate_service_template_spec.rb | 4 +- spec/services/projects/transfer_service_spec.rb | 33 +++ .../update_pages_configuration_service_spec.rb | 78 ++++-- .../services/projects/update_pages_service_spec.rb | 22 +- .../projects/update_remote_mirror_service_spec.rb | 62 +---- .../update_repository_storage_service_spec.rb | 27 +- spec/services/projects/update_service_spec.rb | 62 +++-- 16 files changed, 467 insertions(+), 411 deletions(-) create mode 100644 spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb create mode 100644 spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb (limited to 'spec/services/projects') diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 123b0bad2a8..3e74a15c3c0 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -30,21 +30,6 @@ RSpec.describe Projects::Alerting::NotifyService do end end - shared_examples 'sends notification email' do - let(:notification_service) { spy } - - it 'sends a notification for firing alerts only' do - expect(NotificationService) - .to receive(:new) - .and_return(notification_service) - - expect(notification_service) - .to receive_message_chain(:async, :prometheus_alerts_fired) - - expect(subject).to be_success - end - end - shared_examples 'does not process incident issues' do it 'does not process issues' do expect(IncidentManagement::ProcessAlertWorker) @@ -81,6 +66,7 @@ RSpec.describe Projects::Alerting::NotifyService do fingerprint: fingerprint }.with_indifferent_access end + let(:payload) { ActionController::Parameters.new(payload_raw).permit! } subject { service.execute(token) } @@ -234,7 +220,7 @@ RSpec.describe Projects::Alerting::NotifyService do context 'with emails turned on' do let(:email_enabled) { true } - it_behaves_like 'sends notification email' + it_behaves_like 'Alert Notification Service sends notification email' end end diff --git a/spec/services/projects/cleanup_service_spec.rb b/spec/services/projects/cleanup_service_spec.rb index 528f31456a9..7c28b729e84 100644 --- a/spec/services/projects/cleanup_service_spec.rb +++ b/spec/services/projects/cleanup_service_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Projects::CleanupService do it 'runs garbage collection on the repository' do expect_next_instance_of(GitGarbageCollectWorker) do |worker| - expect(worker).to receive(:perform) + expect(worker).to receive(:perform).with(project.id, :gc, "project_cleanup:gc:#{project.id}") end service.execute diff --git a/spec/services/projects/container_repository/delete_tags_service_spec.rb b/spec/services/projects/container_repository/delete_tags_service_spec.rb index 3d065deefdf..3014ccbd7ba 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -3,21 +3,15 @@ require 'spec_helper' RSpec.describe Projects::ContainerRepository::DeleteTagsService do - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :private) } - let_it_be(:repository) { create(:container_repository, :root, project: project) } + include_context 'container repository delete tags service shared context' - let(:params) { { tags: tags } } let(:service) { described_class.new(project, user, params) } - before do - stub_container_registry_config(enabled: true, - api_url: 'http://registry.gitlab', - host_port: 'registry.gitlab') - - stub_container_registry_tags( - repository: repository.path, - tags: %w(latest A Ba Bb C D E)) + let_it_be(:available_service_classes) do + [ + ::Projects::ContainerRepository::Gitlab::DeleteTagsService, + ::Projects::ContainerRepository::ThirdParty::DeleteTagsService + ] end RSpec.shared_examples 'logging a success response' do @@ -45,8 +39,54 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do end end + RSpec.shared_examples 'calling the correct delete tags service' do |expected_service_class| + let(:service_response) { { status: :success, deleted: tags } } + let(:excluded_service_class) { available_service_classes.excluding(expected_service_class).first } + + before do + service_double = double + expect(expected_service_class).to receive(:new).with(repository, tags).and_return(service_double) + expect(excluded_service_class).not_to receive(:new) + expect(service_double).to receive(:execute).and_return(service_response) + end + + it { is_expected.to include(status: :success) } + + it_behaves_like 'logging a success response' + + context 'with an error service response' do + let(:service_response) { { status: :error, message: 'could not delete tags' } } + + it { is_expected.to include(status: :error) } + + it_behaves_like 'logging an error response' + end + end + + RSpec.shared_examples 'handling invalid params' do + context 'with invalid params' do + before do + expect(::Projects::ContainerRepository::Gitlab::DeleteTagsService).not_to receive(:new) + expect(::Projects::ContainerRepository::ThirdParty::DeleteTagsService).not_to receive(:new) + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + end + + context 'when no params are specified' do + let_it_be(:params) { {} } + + it { is_expected.to include(status: :error) } + end + + context 'with empty tags' do + let_it_be(:tags) { [] } + + it { is_expected.to include(status: :error) } + end + end + end + describe '#execute' do - let(:tags) { %w[A] } + let(:tags) { %w[A Ba] } subject { service.execute(repository) } @@ -61,247 +101,58 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do context 'when the registry supports fast delete' do context 'and the feature is enabled' do - let_it_be(:project) { create(:project, :private) } - let_it_be(:repository) { create(:container_repository, :root, project: project) } - before do allow(repository.client).to receive(:supports_tag_delete?).and_return(true) end - context 'with tags to delete' do - let_it_be(:tags) { %w[A Ba] } - - it 'deletes the tags by name' do - stub_delete_reference_request('A') - stub_delete_reference_request('Ba') - - expect_delete_tag_by_name('A') - expect_delete_tag_by_name('Ba') - - is_expected.to include(status: :success) - end - - it 'succeeds when tag delete returns 404' do - stub_delete_reference_request('A') - stub_delete_reference_request('Ba', 404) - - is_expected.to include(status: :success) - end - - it_behaves_like 'logging a success response' do - before do - stub_delete_reference_request('A') - stub_delete_reference_request('Ba') - end - end - - context 'with failures' do - context 'when the delete request fails' do - before do - stub_delete_reference_request('A', 500) - stub_delete_reference_request('Ba', 500) - end - - it { is_expected.to include(status: :error) } - - it_behaves_like 'logging an error response' - end - end - end - - context 'when no params are specified' do - let_it_be(:params) { {} } + it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::Gitlab::DeleteTagsService - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + it_behaves_like 'handling invalid params' - is_expected.to include(status: :error) + context 'with the real service' do + before do + stub_delete_reference_requests(tags) + expect_delete_tag_by_names(tags) end - end - context 'with empty tags' do - let_it_be(:tags) { [] } + it { is_expected.to include(status: :success) } - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) - - is_expected.to include(status: :error) - end + it_behaves_like 'logging a success response' end end context 'and the feature is disabled' do - let_it_be(:tags) { %w[A Ba] } - before do stub_feature_flags(container_registry_fast_tag_delete: false) - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - stub_put_manifest_request('A') - stub_put_manifest_request('Ba') end - it 'fallbacks to slow delete' do - expect(service).not_to receive(:fast_delete) - expect(service).to receive(:slow_delete).with(repository, tags).and_call_original - - expect_delete_tag_by_digest('sha256:dummy') + it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::ThirdParty::DeleteTagsService - subject - end + it_behaves_like 'handling invalid params' - it_behaves_like 'logging a success response' do + context 'with the real service' do before do - allow(service).to receive(:slow_delete).and_call_original + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + tags.each { |tag| stub_put_manifest_request(tag) } expect_delete_tag_by_digest('sha256:dummy') end + + it { is_expected.to include(status: :success) } + + it_behaves_like 'logging a success response' end end end context 'when the registry does not support fast delete' do - let_it_be(:project) { create(:project, :private) } - let_it_be(:repository) { create(:container_repository, :root, project: project) } - before do - stub_tag_digest('latest', 'sha256:configA') - stub_tag_digest('A', 'sha256:configA') - stub_tag_digest('Ba', 'sha256:configB') - allow(repository.client).to receive(:supports_tag_delete?).and_return(false) end - context 'when no params are specified' do - let_it_be(:params) { {} } - - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest) + it_behaves_like 'calling the correct delete tags service', ::Projects::ContainerRepository::ThirdParty::DeleteTagsService - is_expected.to include(status: :error) - end - end - - context 'with empty tags' do - let_it_be(:tags) { [] } - - it 'does not remove anything' do - expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_digest) - - is_expected.to include(status: :error) - end - end - - context 'with tags to delete' do - let_it_be(:tags) { %w[A Ba] } - - it 'deletes the tags using a dummy image' do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - - stub_put_manifest_request('A') - stub_put_manifest_request('Ba') - - expect_delete_tag_by_digest('sha256:dummy') - - is_expected.to include(status: :success) - end - - it 'succeeds when tag delete returns 404' do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - - stub_put_manifest_request('A') - stub_put_manifest_request('Ba') - - stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy") - .to_return(status: 404, body: "", headers: {}) - - is_expected.to include(status: :success) - end - - it_behaves_like 'logging a success response' do - before do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - stub_put_manifest_request('A') - stub_put_manifest_request('Ba') - expect_delete_tag_by_digest('sha256:dummy') - end - end - - context 'with failures' do - context 'when the dummy manifest generation fails' do - before do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3', success: false) - end - - it { is_expected.to include(status: :error) } - - it_behaves_like 'logging an error response', message: 'could not generate manifest' - end - - context 'when updating the tags fails' do - before do - stub_upload("{\n \"config\": {\n }\n}", 'sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') - - stub_put_manifest_request('A', 500) - stub_put_manifest_request('Ba', 500) - - stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3") - .to_return(status: 200, body: "", headers: {}) - end - - it { is_expected.to include(status: :error) } - it_behaves_like 'logging an error response' - end - end - end + it_behaves_like 'handling invalid params' end end end - - private - - def stub_delete_reference_request(tag, status = 200) - stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/tags/reference/#{tag}") - .to_return(status: status, body: '') - end - - def stub_put_manifest_request(tag, status = 200, headers = { 'docker-content-digest' => 'sha256:dummy' }) - stub_request(:put, "http://registry.gitlab/v2/#{repository.path}/manifests/#{tag}") - .to_return(status: status, body: '', headers: headers) - end - - def stub_tag_digest(tag, digest) - stub_request(:head, "http://registry.gitlab/v2/#{repository.path}/manifests/#{tag}") - .to_return(status: 200, body: "", headers: { 'docker-content-digest' => digest }) - end - - def stub_digest_config(digest, created_at) - allow_any_instance_of(ContainerRegistry::Client) - .to receive(:blob) - .with(repository.path, digest, nil) do - { 'created' => created_at.to_datetime.rfc3339 }.to_json if created_at - end - end - - def stub_upload(content, digest, success: true) - expect_any_instance_of(ContainerRegistry::Client) - .to receive(:upload_blob) - .with(repository.path, content, digest) { double(success?: success ) } - end - - def expect_delete_tag_by_digest(digest) - expect_any_instance_of(ContainerRegistry::Client) - .to receive(:delete_repository_tag_by_digest) - .with(repository.path, digest) { true } - - expect_any_instance_of(ContainerRegistry::Client) - .not_to receive(:delete_repository_tag_by_name) - end - - def expect_delete_tag_by_name(name) - expect_any_instance_of(ContainerRegistry::Client) - .to receive(:delete_repository_tag_by_name) - .with(repository.path, name) { true } - - expect_any_instance_of(ContainerRegistry::Client) - .not_to receive(:delete_repository_tag_by_digest) - end end diff --git a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb new file mode 100644 index 00000000000..68c232e5d83 --- /dev/null +++ b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do + include_context 'container repository delete tags service shared context' + + let(:service) { described_class.new(repository, tags) } + + describe '#execute' do + let(:tags) { %w[A Ba] } + + subject { service.execute } + + context 'with tags to delete' do + it 'deletes the tags by name' do + stub_delete_reference_requests(tags) + expect_delete_tag_by_names(tags) + + is_expected.to eq(status: :success, deleted: tags) + end + + it 'succeeds when tag delete returns 404' do + stub_delete_reference_requests('A' => 200, 'Ba' => 404) + + is_expected.to eq(status: :success, deleted: tags) + end + + it 'succeeds when a tag delete returns 500' do + stub_delete_reference_requests('A' => 200, 'Ba' => 500) + + is_expected.to eq(status: :success, deleted: ['A']) + end + + context 'with failures' do + context 'when the delete request fails' do + before do + stub_delete_reference_requests('A' => 500, 'Ba' => 500) + end + + it { is_expected.to eq(status: :error, message: 'could not delete tags') } + end + end + end + + context 'with empty tags' do + let_it_be(:tags) { [] } + + it 'does not remove anything' do + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + + is_expected.to eq(status: :success, deleted: []) + end + end + end +end diff --git a/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb b/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb new file mode 100644 index 00000000000..7fc963949eb --- /dev/null +++ b/spec/services/projects/container_repository/third_party/delete_tags_service_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ContainerRepository::ThirdParty::DeleteTagsService do + include_context 'container repository delete tags service shared context' + + let(:service) { described_class.new(repository, tags) } + + describe '#execute' do + let(:tags) { %w[A Ba] } + + subject { service.execute } + + context 'with tags to delete' do + it 'deletes the tags by name' do + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + tags.each { |tag| stub_put_manifest_request(tag) } + + expect_delete_tag_by_digest('sha256:dummy') + + is_expected.to eq(status: :success, deleted: tags) + end + + it 'succeeds when tag delete returns 404' do + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + stub_put_manifest_request('A') + stub_put_manifest_request('Ba') + + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy") + .to_return(status: 404, body: '', headers: {}) + + is_expected.to eq(status: :success, deleted: tags) + end + + context 'with failures' do + context 'when the dummy manifest generation fails' do + before do + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3', success: false) + end + + it { is_expected.to eq(status: :error, message: 'could not generate manifest') } + end + + context 'when updating tags fails' do + before do + stub_upload('sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3') + + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:4435000728ee66e6a80e55637fc22725c256b61de344a2ecdeaac6bdb36e8bc3") + .to_return(status: 200, body: '', headers: {}) + end + + context 'all tag updates fail' do + before do + stub_put_manifest_request('A', 500, {}) + stub_put_manifest_request('Ba', 500, {}) + end + + it { is_expected.to eq(status: :error, message: 'could not delete tags') } + end + + context 'a single tag update fails' do + before do + stub_put_manifest_request('A') + stub_put_manifest_request('Ba', 500, {}) + + stub_request(:delete, "http://registry.gitlab/v2/#{repository.path}/manifests/sha256:dummy") + .to_return(status: 404, body: '', headers: {}) + end + + it { is_expected.to eq(status: :success, deleted: ['A']) } + end + end + end + end + + context 'with empty tags' do + let_it_be(:tags) { [] } + + it 'does not remove anything' do + expect_any_instance_of(ContainerRegistry::Client).not_to receive(:delete_repository_tag_by_name) + + is_expected.to eq(status: :success, deleted: []) + end + end + end +end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 9eb7cacbbcb..e1df8700795 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -48,6 +48,12 @@ RSpec.describe Projects::CreateService, '#execute' do expect(project.project_setting).to be_new_record end + + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { project: subject.full_path, related_class: described_class.to_s } } + + subject { create_project(user, opts) } + end end context "admin creates project with other user's namespace_id" do diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index c49aa42b147..925c2ff5d88 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -105,6 +105,7 @@ RSpec.describe Projects::ForkService do group.add_owner(@to_user) group end + let(:to_project) { fork_project(from_forked_project, @to_user, namespace: other_namespace) } it 'sets the root of the network to the root project' do @@ -439,37 +440,71 @@ RSpec.describe Projects::ForkService do end describe '#valid_fork_target?' do - subject { described_class.new(project, user, params).valid_fork_target? } - let(:project) { Project.new } let(:params) { {} } - context 'when current user is an admin' do - let(:user) { build(:user, :admin) } + context 'when target is not passed' do + subject { described_class.new(project, user, params).valid_fork_target? } - it { is_expected.to be_truthy } - end + context 'when current user is an admin' do + let(:user) { build(:user, :admin) } - context 'when current_user is not an admin' do - let(:user) { create(:user) } + it { is_expected.to be_truthy } + end - let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [user.namespace]) } - let(:project) { create(:project) } + context 'when current_user is not an admin' do + let(:user) { create(:user) } - before do - allow(ForkTargetsFinder).to receive(:new).with(project, user).and_return(finder_mock) + let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [user.namespace]) } + let(:project) { create(:project) } + + before do + allow(ForkTargetsFinder).to receive(:new).with(project, user).and_return(finder_mock) + end + + context 'when target namespace is in valid fork targets' do + let(:params) { { namespace: user.namespace } } + + it { is_expected.to be_truthy } + end + + context 'when target namespace is not in valid fork targets' do + let(:params) { { namespace: create(:group) } } + + it { is_expected.to be_falsey } + end end + end + + context 'when target is passed' do + let(:target) { create(:group) } - context 'when target namespace is in valid fork targets' do - let(:params) { { namespace: user.namespace } } + subject { described_class.new(project, user, params).valid_fork_target?(target) } + + context 'when current user is an admin' do + let(:user) { build(:user, :admin) } it { is_expected.to be_truthy } end - context 'when target namespace is not in valid fork targets' do - let(:params) { { namespace: create(:group) } } + context 'when current user is not an admin' do + let(:user) { create(:user) } - it { is_expected.to be_falsey } + before do + allow(ForkTargetsFinder).to receive(:new).with(project, user).and_return(finder_mock) + end + + context 'when target namespace is in valid fork targets' do + let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [target]) } + + it { is_expected.to be_truthy } + end + + context 'when target namespace is not in valid fork targets' do + let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [create(:group)]) } + + it { is_expected.to be_falsey } + end end end end diff --git a/spec/services/projects/operations/update_service_spec.rb b/spec/services/projects/operations/update_service_spec.rb index 3cfc9844d65..8a538bc67ed 100644 --- a/spec/services/projects/operations/update_service_spec.rb +++ b/spec/services/projects/operations/update_service_spec.rb @@ -384,6 +384,7 @@ RSpec.describe Projects::Operations::UpdateService do manual_configuration: "0" }) end + let(:params) do { prometheus_integration_attributes: { diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index aae257e3e3a..efe8e8b9243 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -21,38 +21,6 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do project.clear_memoization(:licensed_feature_available) end - shared_examples 'sends notification email' do - let(:notification_service) { spy } - - it 'sends a notification for firing alerts only' do - expect(NotificationService) - .to receive(:new) - .and_return(notification_service) - - expect(notification_service) - .to receive_message_chain(:async, :prometheus_alerts_fired) - - expect(subject).to be_success - end - end - - shared_examples 'notifies alerts' do - it_behaves_like 'sends notification email' - end - - shared_examples 'no notifications' do |http_status:| - let(:notification_service) { spy } - let(:create_events_service) { spy } - - it 'does not notify' do - expect(notification_service).not_to receive(:async) - expect(create_events_service).not_to receive(:execute) - - expect(subject).to be_error - expect(subject.http_status).to eq(http_status) - end - end - context 'with valid payload' do let_it_be(:alert_firing) { create(:prometheus_alert, project: project) } let_it_be(:alert_resolved) { create(:prometheus_alert, project: project) } @@ -89,11 +57,11 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do context 'without token' do let(:token_input) { nil } - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' end context 'with token' do - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized end end @@ -125,9 +93,9 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do case result = params[:result] when :success - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' when :failure - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized else raise "invalid result: #{result.inspect}" end @@ -137,7 +105,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do context 'without project specific cluster' do let!(:cluster) { create(:cluster, enabled: true) } - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized end context 'with manual prometheus installation' do @@ -166,9 +134,9 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do case result = params[:result] when :success - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' when :failure - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized else raise "invalid result: #{result.inspect}" end @@ -199,9 +167,9 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do case result = params[:result] when :success - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' when :failure - it_behaves_like 'no notifications', http_status: :unauthorized + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unauthorized else raise "invalid result: #{result.inspect}" end @@ -226,7 +194,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end context 'when incident_management_setting.send_email is true' do - it_behaves_like 'notifies alerts' + it_behaves_like 'Alert Notification Service sends notification email' end context 'incident_management_setting.send_email is false' do @@ -278,7 +246,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do .and_return(false) end - it_behaves_like 'no notifications', http_status: :unprocessable_entity + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :unprocessable_entity end context 'when the payload is too big' do @@ -289,7 +257,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do allow(Gitlab::Utils::DeepSize).to receive(:new).and_return(deep_size_object) end - it_behaves_like 'no notifications', http_status: :bad_request + it_behaves_like 'Alert Notification Service sends no notifications', http_status: :bad_request it 'does not process Prometheus alerts' do expect(AlertManagement::ProcessPrometheusAlertService) diff --git a/spec/services/projects/propagate_service_template_spec.rb b/spec/services/projects/propagate_service_template_spec.rb index 266bf2cc213..df69e5a29fb 100644 --- a/spec/services/projects/propagate_service_template_spec.rb +++ b/spec/services/projects/propagate_service_template_spec.rb @@ -20,7 +20,7 @@ RSpec.describe Projects::PropagateServiceTemplate do end let!(:project) { create(:project) } - let(:excluded_attributes) { %w[id project_id template created_at updated_at title description] } + let(:excluded_attributes) { %w[id project_id template created_at updated_at default] } it 'creates services for projects' do expect(project.pushover_service).to be_nil @@ -120,7 +120,7 @@ RSpec.describe Projects::PropagateServiceTemplate do describe 'external tracker' do it 'updates the project external tracker' do - service_template.update!(category: 'issue_tracker', default: false) + service_template.update!(category: 'issue_tracker') expect { described_class.propagate(service_template) } .to change { project.reload.has_external_issue_tracker }.to(true) 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| diff --git a/spec/services/projects/update_pages_configuration_service_spec.rb b/spec/services/projects/update_pages_configuration_service_spec.rb index c4c9fc779fa..9f7ebd40df6 100644 --- a/spec/services/projects/update_pages_configuration_service_spec.rb +++ b/spec/services/projects/update_pages_configuration_service_spec.rb @@ -3,41 +3,75 @@ require 'spec_helper' RSpec.describe Projects::UpdatePagesConfigurationService do - let(:project) { create(:project) } let(:service) { described_class.new(project) } - describe "#update" do - let(:file) { Tempfile.new('pages-test') } - + describe "#execute" do subject { service.execute } - after do - file.close - file.unlink - end + context 'when pages are deployed' do + let_it_be(:project) do + create(:project).tap(&:mark_pages_as_deployed) + end - before do - allow(service).to receive(:pages_config_file).and_return(file.path) - end + let(:file) { Tempfile.new('pages-test') } + + before do + allow(service).to receive(:pages_config_file).and_return(file.path) + end + + after do + file.close + file.unlink + end + + context 'when configuration changes' do + it 'updates the config and reloads the daemon' do + allow(service).to receive(:update_file).and_call_original - context 'when configuration changes' do - it 'updates the .update file' do - expect(service).to receive(:reload_daemon).and_call_original + expect(service).to receive(:update_file).with(file.path, an_instance_of(String)) + .and_call_original + expect(service).to receive(:reload_daemon).and_call_original - expect(subject).to include(status: :success, reload: true) + expect(subject).to include(status: :success) + end + end + + context 'when configuration does not change' do + before do + # we set the configuration + service.execute + end + + it 'does not update the .update file' do + expect(service).not_to receive(:reload_daemon) + + expect(subject).to include(status: :success) + end + end + + context 'when an error occurs' do + it 'returns an error object' do + e = StandardError.new("Failure") + allow(service).to receive(:reload_daemon).and_raise(e) + + expect(subject).to eq(status: :error, message: "Failure", exception: e) + end end end - context 'when configuration does not change' do - before do - # we set the configuration - service.execute + context 'when pages are not deployed' do + let_it_be(:project) do + create(:project).tap(&:mark_pages_as_not_deployed) + end + + it 'returns successfully' do + expect(subject).to eq(status: :success) end - it 'does not update the .update file' do - expect(service).not_to receive(:reload_daemon) + it 'does not update the config' do + expect(service).not_to receive(:update_file) - expect(subject).to include(status: :success, reload: false) + subject end end end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 2e02cb56668..374ce4f4ce2 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -53,7 +53,7 @@ RSpec.describe Projects::UpdatePagesService do # Check that all expected files are extracted %w[index.html zero .hidden/file].each do |filename| - expect(File.exist?(File.join(project.public_pages_path, filename))).to be_truthy + expect(File.exist?(File.join(project.pages_path, 'public', filename))).to be_truthy end end @@ -65,15 +65,17 @@ RSpec.describe Projects::UpdatePagesService do it 'removes pages after destroy' do expect(PagesWorker).to receive(:perform_in) expect(project.pages_deployed?).to be_falsey + expect(Dir.exist?(File.join(project.pages_path))).to be_falsey expect(execute).to eq(:success) expect(project.pages_metadatum).to be_deployed expect(project.pages_deployed?).to be_truthy + expect(Dir.exist?(File.join(project.pages_path))).to be_truthy project.destroy - expect(project.pages_deployed?).to be_falsey + expect(Dir.exist?(File.join(project.pages_path))).to be_falsey expect(ProjectPagesMetadatum.find_by_project_id(project)).to be_nil end @@ -160,19 +162,9 @@ RSpec.describe Projects::UpdatePagesService do end context 'with background jobs running', :sidekiq_inline do - where(:ci_atomic_processing) do - [true, false] - end - - with_them do - before do - stub_feature_flags(ci_atomic_processing: ci_atomic_processing) - end - - it 'succeeds' do - expect(project.pages_deployed?).to be_falsey - expect(execute).to eq(:success) - end + it 'succeeds' do + expect(project.pages_deployed?).to be_falsey + expect(execute).to eq(:success) end end end diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb index f0a8074f46c..09244db8010 100644 --- a/spec/services/projects/update_remote_mirror_service_spec.rb +++ b/spec/services/projects/update_remote_mirror_service_spec.rb @@ -10,10 +10,6 @@ RSpec.describe Projects::UpdateRemoteMirrorService do subject(:service) { described_class.new(project, project.creator) } - before do - stub_feature_flags(gitaly_ruby_remote_branches_ls_remote: false) - end - describe '#execute' do subject(:execute!) { service.execute(remote_mirror, 0) } @@ -26,17 +22,14 @@ RSpec.describe Projects::UpdateRemoteMirrorService do end it 'ensures the remote exists' do - stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) - expect(remote_mirror).to receive(:ensure_remote!) execute! end - it 'fetches the remote repository' do - expect(project.repository) - .to receive(:fetch_remote) - .with(remote_mirror.remote_name, no_tags: true, ssh_auth: remote_mirror) + it 'does not fetch the remote repository' do + # See https://gitlab.com/gitlab-org/gitaly/-/issues/2670 + expect(project.repository).not_to receive(:fetch_remote) execute! end @@ -48,8 +41,6 @@ RSpec.describe Projects::UpdateRemoteMirrorService do end it 'marks the mirror as successfully finished' do - stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) - result = execute! expect(result[:status]).to eq(:success) @@ -57,7 +48,7 @@ RSpec.describe Projects::UpdateRemoteMirrorService do end it 'marks the mirror as failed and raises the error when an unexpected error occurs' do - allow(project.repository).to receive(:fetch_remote).and_raise('Badly broken') + allow(remote_mirror).to receive(:update_repository).and_raise('Badly broken') expect { execute! }.to raise_error(/Badly broken/) @@ -67,33 +58,30 @@ RSpec.describe Projects::UpdateRemoteMirrorService do context 'when the update fails because of a `Gitlab::Git::CommandError`' do before do - allow(project.repository).to receive(:fetch_remote).and_raise(Gitlab::Git::CommandError.new('fetch failed')) + allow(remote_mirror).to receive(:update_repository) + .and_raise(Gitlab::Git::CommandError.new('update failed')) end it 'wraps `Gitlab::Git::CommandError`s in a service error' do - expect(execute!).to eq(status: :error, message: 'fetch failed') + expect(execute!).to eq(status: :error, message: 'update failed') end it 'marks the mirror as to be retried' do execute! expect(remote_mirror).to be_to_retry - expect(remote_mirror.last_error).to include('fetch failed') + expect(remote_mirror.last_error).to include('update failed') end it "marks the mirror as failed after #{described_class::MAX_TRIES} tries" do service.execute(remote_mirror, described_class::MAX_TRIES) expect(remote_mirror).to be_failed - expect(remote_mirror.last_error).to include('fetch failed') + expect(remote_mirror.last_error).to include('update failed') end end context 'when there are divergent refs' do - before do - stub_fetch_remote(project, remote_name: remote_name, ssh_auth: remote_mirror) - end - it 'marks the mirror as failed and sets an error message' do response = double(divergent_refs: %w[refs/heads/master refs/heads/develop]) expect(remote_mirror).to receive(:update_repository).and_return(response) @@ -106,37 +94,5 @@ RSpec.describe Projects::UpdateRemoteMirrorService do expect(remote_mirror.last_error).to include("refs/heads/develop") end end - - # https://gitlab.com/gitlab-org/gitaly/-/issues/2670 - context 'when `gitaly_ruby_remote_branches_ls_remote` is enabled' do - before do - stub_feature_flags(gitaly_ruby_remote_branches_ls_remote: true) - end - - it 'does not perform a fetch' do - expect(project.repository).not_to receive(:fetch_remote) - - execute! - end - end - end - - def stub_fetch_remote(project, remote_name:, ssh_auth:) - allow(project.repository) - .to receive(:fetch_remote) - .with(remote_name, no_tags: true, ssh_auth: ssh_auth) { fetch_remote(project.repository, remote_name) } - end - - def fetch_remote(repository, remote_name) - local_branch_names(repository).each do |branch| - commit = repository.commit(branch) - repository.write_ref("refs/remotes/#{remote_name}/#{branch}", commit.id) if commit - end - end - - def local_branch_names(repository) - branch_names = repository.branches.map(&:name) - # we want the protected branch to be pushed first - branch_names.unshift(branch_names.delete('master')) end end diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index 57e02c26b71..0fcd14f3bc9 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -21,6 +21,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } let!(:checksum) { project.repository.checksum } let(:project_repository_double) { double(:repository) } + let(:original_project_repository_double) { double(:repository) } before do allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original @@ -29,6 +30,9 @@ RSpec.describe Projects::UpdateRepositoryStorageService do allow(Gitlab::Git::Repository).to receive(:new) .with('test_second_storage', project.repository.raw.relative_path, project.repository.gl_repository, project.repository.full_path) .and_return(project_repository_double) + allow(Gitlab::Git::Repository).to receive(:new) + .with('default', project.repository.raw.relative_path, nil, nil) + .and_return(original_project_repository_double) end context 'when the move succeeds' do @@ -41,8 +45,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do .with(project.repository.raw) expect(project_repository_double).to receive(:checksum) .and_return(checksum) - expect(GitlabShellWorker).to receive(:perform_async).with(:mv_repository, 'default', anything, anything) - .and_call_original + expect(original_project_repository_double).to receive(:remove) result = subject.execute project.reload @@ -74,13 +77,29 @@ RSpec.describe Projects::UpdateRepositoryStorageService do expect(project_repository_double).to receive(:replicate) .with(project.repository.raw) .and_raise(Gitlab::Git::CommandError) - expect(GitlabShellWorker).not_to receive(:perform_async) result = subject.execute expect(result).to be_error expect(project).not_to be_repository_read_only expect(project.repository_storage).to eq('default') + expect(repository_storage_move).to be_failed + end + end + + context 'when the cleanup fails' do + it 'sets the correct state' do + expect(project_repository_double).to receive(:replicate) + .with(project.repository.raw) + expect(project_repository_double).to receive(:checksum) + .and_return(checksum) + expect(original_project_repository_double).to receive(:remove) + .and_raise(Gitlab::Git::CommandError) + + result = subject.execute + + expect(result).to be_error + expect(repository_storage_move).to be_cleanup_failed end end @@ -93,7 +112,6 @@ RSpec.describe Projects::UpdateRepositoryStorageService do .with(project.repository.raw) expect(project_repository_double).to receive(:checksum) .and_return('not matching checksum') - expect(GitlabShellWorker).not_to receive(:perform_async) result = subject.execute @@ -114,6 +132,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do .with(project.repository.raw) expect(project_repository_double).to receive(:checksum) .and_return(checksum) + expect(original_project_repository_double).to receive(:remove) result = subject.execute project.reload diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 6620ee6e697..4a613f42556 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -396,6 +396,50 @@ RSpec.describe Projects::UpdateService do end end + shared_examples 'updating pages configuration' do + it 'schedules the `PagesUpdateConfigurationWorker` when pages are deployed' do + project.mark_pages_as_deployed + + expect(PagesUpdateConfigurationWorker).to receive(:perform_async).with(project.id) + + subject + end + + it "does not schedule a job when pages aren't deployed" do + project.mark_pages_as_not_deployed + + expect(PagesUpdateConfigurationWorker).not_to receive(:perform_async).with(project.id) + + subject + end + + context 'when `async_update_pages_config` is disabled' do + before do + stub_feature_flags(async_update_pages_config: false) + end + + it 'calls Projects::UpdatePagesConfigurationService when pages are deployed' do + project.mark_pages_as_deployed + + expect(Projects::UpdatePagesConfigurationService) + .to receive(:new) + .with(project) + .and_call_original + + subject + end + + it "does not update pages config when pages aren't deployed" do + project.mark_pages_as_not_deployed + + expect(Projects::UpdatePagesConfigurationService) + .not_to receive(:new) + + subject + end + end + end + context 'when updating #pages_https_only', :https_pages_enabled do subject(:call_service) do update_project(project, admin, pages_https_only: false) @@ -407,14 +451,7 @@ RSpec.describe Projects::UpdateService do .to(false) end - it 'calls Projects::UpdatePagesConfigurationService' do - expect(Projects::UpdatePagesConfigurationService) - .to receive(:new) - .with(project) - .and_call_original - - call_service - end + it_behaves_like 'updating pages configuration' end context 'when updating #pages_access_level' do @@ -428,14 +465,7 @@ RSpec.describe Projects::UpdateService do .to(ProjectFeature::ENABLED) end - it 'calls Projects::UpdatePagesConfigurationService' do - expect(Projects::UpdatePagesConfigurationService) - .to receive(:new) - .with(project) - .and_call_original - - call_service - end + it_behaves_like 'updating pages configuration' end context 'when updating #emails_disabled' do -- cgit v1.2.3