From c43ca842f67065bc64dd11c3abb954c1632ffccf Mon Sep 17 00:00:00 2001 From: Chris Rose Date: Sat, 30 Mar 2019 21:20:12 +0000 Subject: Correct spelling mistake in documentation --- doc/university/high-availability/aws/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/university/high-availability/aws/README.md b/doc/university/high-availability/aws/README.md index 1906655dfa5..01d5073ab7e 100644 --- a/doc/university/high-availability/aws/README.md +++ b/doc/university/high-availability/aws/README.md @@ -83,7 +83,7 @@ our newly created VPC. ### Internet Gateway Now still on the same dashboard head over to Internet Gateways and -create a new one. After its created pres on the `Attach to VPC` button and +create a new one. After its created press on the `Attach to VPC` button and select our VPC. ![Internet Gateway](img/ig.png) -- cgit v1.2.1 From ef8caefb7b54b2b820e886b273464dbe213aa53d Mon Sep 17 00:00:00 2001 From: chow89 Date: Tue, 2 Apr 2019 19:27:35 +0000 Subject: Fix typo in docs --- doc/user/project/pages/getting_started_part_three.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/project/pages/getting_started_part_three.md b/doc/user/project/pages/getting_started_part_three.md index 756b8b698c7..fa7ab19ece6 100644 --- a/doc/user/project/pages/getting_started_part_three.md +++ b/doc/user/project/pages/getting_started_part_three.md @@ -149,7 +149,7 @@ verify your domain's ownership with a TXT record: Once you've set the DNS record, you'll need navigate to your project's **Setting > Pages** and click **+ New domain** to add your custom domain to GitLab Pages. You can choose whether to add an [SSL/TLS certificate](#ssltls-certificates) -to make your website accessible under HTTPS or leave it blank. If don't add a certificate, +to make your website accessible under HTTPS or leave it blank. If you don't add a certificate, your site will be accessible only via HTTP: ![Add new domain](img/add_certificate_to_pages.png) -- cgit v1.2.1 From d3de5fcfa8331e13bcf00d41653c2c6ee337abf5 Mon Sep 17 00:00:00 2001 From: Bastian Blank Date: Sat, 30 Mar 2019 00:27:10 +0100 Subject: Always show instance configuration link The link to the useful instance configuration page was hidden behind the commercial content setting. Just display it always. --- app/views/help/index.html.haml | 5 +++-- changelogs/unreleased/always-link-instance-configuration.yml | 5 +++++ 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/always-link-instance-configuration.yml diff --git a/app/views/help/index.html.haml b/app/views/help/index.html.haml index f40fdb0b86b..916f98a62d1 100644 --- a/app/views/help/index.html.haml +++ b/app/views/help/index.html.haml @@ -24,8 +24,9 @@ Used by more than 100,000 organizations, GitLab is the most popular solution to manage git repositories on-premises. %br Read more about GitLab at #{link_to promo_host, promo_url, target: '_blank', rel: 'noopener noreferrer'}. - %p= link_to 'Check the current instance configuration ', help_instance_configuration_url - %hr + +%p= link_to 'Check the current instance configuration ', help_instance_configuration_url +%hr .row.prepend-top-default .col-md-8 diff --git a/changelogs/unreleased/always-link-instance-configuration.yml b/changelogs/unreleased/always-link-instance-configuration.yml new file mode 100644 index 00000000000..3f08747edf7 --- /dev/null +++ b/changelogs/unreleased/always-link-instance-configuration.yml @@ -0,0 +1,5 @@ +--- +title: Always show instance configuration link +merge_request: 26783 +author: Bastian Blank +type: fixed -- cgit v1.2.1 From 8938cf8874075a8049a92d5ee0c62a74fc471439 Mon Sep 17 00:00:00 2001 From: mlncn Date: Wed, 3 Apr 2019 02:30:36 +0000 Subject: Fix grammar and try to make clearer how a fork works --- doc/gitlab-basics/fork-project.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/gitlab-basics/fork-project.md b/doc/gitlab-basics/fork-project.md index 6c232fe6086..a128a7c7dd3 100644 --- a/doc/gitlab-basics/fork-project.md +++ b/doc/gitlab-basics/fork-project.md @@ -1,8 +1,8 @@ # How to fork a project -A fork is a copy of an original repository that you can put in another namespace -where you can experiment and apply changes that you can later decide if -publishing or not, without affecting your original project. +A fork is a copy of an original repository that you put in another namespace +where you can experiment and apply changes that you can later decide whether or +not to share, without affecting the original project. It takes just a few steps to fork a project in GitLab. -- cgit v1.2.1 From f8fdb59059b8c1037fd804636e4247642a08ebb9 Mon Sep 17 00:00:00 2001 From: Joan Queralt Date: Wed, 3 Apr 2019 07:59:46 +0000 Subject: public key -> private key --- doc/user/project/pages/lets_encrypt_for_gitlab_pages.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/project/pages/lets_encrypt_for_gitlab_pages.md b/doc/user/project/pages/lets_encrypt_for_gitlab_pages.md index f639188684b..5ad500c4d20 100644 --- a/doc/user/project/pages/lets_encrypt_for_gitlab_pages.md +++ b/doc/user/project/pages/lets_encrypt_for_gitlab_pages.md @@ -134,7 +134,7 @@ Now that your certificate has been issued, let's add it to your Pages site: sudo cat /etc/letsencrypt/live/example.com/fullchain.pem | pbcopy ``` -1. Copy and paste the public key into the second field **Key (PEM)**: +1. Copy and paste the private key into the second field **Key (PEM)**: ```bash sudo cat /etc/letsencrypt/live/example.com/privkey.pem | pbcopy -- cgit v1.2.1 From 5725be9508df7922c77cfab4a6c5501548e5e3ca Mon Sep 17 00:00:00 2001 From: Bastian Blank Date: Sat, 30 Mar 2019 00:50:02 +0100 Subject: Display maximum artifact size from runtime config The maximum artifact size was moved into runtime config some time ago. Update the instance configuration code to read this value. --- app/models/instance_configuration.rb | 2 +- changelogs/unreleased/instance-configuration-artifact-size.yml | 5 +++++ spec/models/instance_configuration_spec.rb | 7 +++++++ 3 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/instance-configuration-artifact-size.yml diff --git a/app/models/instance_configuration.rb b/app/models/instance_configuration.rb index 11289887e00..a9b1962f24c 100644 --- a/app/models/instance_configuration.rb +++ b/app/models/instance_configuration.rb @@ -39,7 +39,7 @@ class InstanceConfiguration def gitlab_ci Settings.gitlab_ci .to_h - .merge(artifacts_max_size: { value: Settings.artifacts.max_size&.megabytes, + .merge(artifacts_max_size: { value: Gitlab::CurrentSettings.max_artifacts_size.megabytes, default: 100.megabytes }) end diff --git a/changelogs/unreleased/instance-configuration-artifact-size.yml b/changelogs/unreleased/instance-configuration-artifact-size.yml new file mode 100644 index 00000000000..077f8631af5 --- /dev/null +++ b/changelogs/unreleased/instance-configuration-artifact-size.yml @@ -0,0 +1,5 @@ +--- +title: Display maximum artifact size from runtime config +merge_request: 26784 +author: Bastian Blank +type: fixed diff --git a/spec/models/instance_configuration_spec.rb b/spec/models/instance_configuration_spec.rb index e65f97df3c3..43954511858 100644 --- a/spec/models/instance_configuration_spec.rb +++ b/spec/models/instance_configuration_spec.rb @@ -82,6 +82,13 @@ describe InstanceConfiguration do it 'returns the key artifacts_max_size' do expect(gitlab_ci.keys).to include(:artifacts_max_size) end + + it 'returns the key artifacts_max_size with values' do + stub_application_setting(max_artifacts_size: 200) + + expect(gitlab_ci[:artifacts_max_size][:default]).to eq(100.megabytes) + expect(gitlab_ci[:artifacts_max_size][:value]).to eq(200.megabytes) + end end end end -- cgit v1.2.1 From 703d62a549ea18b678edf05d9bbb3bf09d3d7356 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Wed, 3 Apr 2019 16:10:21 -0300 Subject: Extract EE specific files/lines for Discussion spec/services This is part of moving GitLab to single codebase --- spec/services/system_note_service_spec.rb | 49 ++++++++++++++++--------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index b917de14b2e..ff994f3b922 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -1,6 +1,9 @@ +# frozen_string_literal: true + require 'spec_helper' describe SystemNoteService do + include ProjectForksHelper include Gitlab::Routing include RepoHelpers include AssetsHelpers @@ -619,7 +622,7 @@ describe SystemNoteService do context 'commit with cross-reference from fork' do let(:author2) { create(:project_member, :reporter, user: create(:user), project: project).user } - let(:forked_project) { Projects::ForkService.new(project, author2).execute } + let(:forked_project) { fork_project(project, author2, repository: true) } let(:commit2) { forked_project.commit } before do @@ -896,6 +899,28 @@ describe SystemNoteService do end end + describe '.change_time_estimate' do + subject { described_class.change_time_estimate(noteable, project, author) } + + it_behaves_like 'a system note' do + let(:action) { 'time_tracking' } + end + + context 'with a time estimate' do + it 'sets the note text' do + noteable.update_attribute(:time_estimate, 277200) + + expect(subject.note).to eq "changed time estimate to 1w 4d 5h" + end + end + + context 'without a time estimate' do + it 'sets the note text' do + expect(subject.note).to eq "removed time estimate" + end + end + end + describe '.discussion_continued_in_issue' do let(:discussion) { create(:diff_note_on_merge_request, project: project).to_discussion } let(:merge_request) { discussion.noteable } @@ -922,28 +947,6 @@ describe SystemNoteService do end end - describe '.change_time_estimate' do - subject { described_class.change_time_estimate(noteable, project, author) } - - it_behaves_like 'a system note' do - let(:action) { 'time_tracking' } - end - - context 'with a time estimate' do - it 'sets the note text' do - noteable.update_attribute(:time_estimate, 277200) - - expect(subject.note).to eq "changed time estimate to 1w 4d 5h" - end - end - - context 'without a time estimate' do - it 'sets the note text' do - expect(subject.note).to eq "removed time estimate" - end - end - end - describe '.change_time_spent' do # We need a custom noteable in order to the shared examples to be green. let(:noteable) do -- cgit v1.2.1 From 631c3d64bf7b493b0139a9e10f4491289cf6bb3e Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Thu, 4 Apr 2019 00:26:03 +0900 Subject: Remove a "reopen merge request button" on a "merged" merge request --- app/assets/javascripts/notes/components/comment_form.vue | 9 ++++++--- app/assets/javascripts/notes/constants.js | 1 + changelogs/unreleased/do-not-reopen-merged-mr.yml | 5 +++++ 3 files changed, 12 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/do-not-reopen-merged-mr.yml diff --git a/app/assets/javascripts/notes/components/comment_form.vue b/app/assets/javascripts/notes/components/comment_form.vue index 1d6cb9485f7..b30d7fa9b73 100644 --- a/app/assets/javascripts/notes/components/comment_form.vue +++ b/app/assets/javascripts/notes/components/comment_form.vue @@ -115,8 +115,11 @@ export default { author() { return this.getUserData; }, - canUpdateIssue() { - return this.getNoteableData.current_user.can_update; + canToggleIssueState() { + return ( + this.getNoteableData.current_user.can_update && + this.getNoteableData.state !== constants.MERGED + ); }, endpoint() { return this.getNoteableData.create_note_path; @@ -415,7 +418,7 @@ append-right-10 comment-type-dropdown js-comment-type-dropdown droplab-dropdown" Date: Wed, 3 Apr 2019 22:00:29 -0700 Subject: Fix stage index migration failing in PostgreSQL 10 As discussed in https://www.postgresql.org/message-id/9922.1353433645%40sss.pgh.pa.us, the PostgreSQL window function last_value may not consider the right rows: Note that first_value, last_value, and nth_value consider only the rows within the "window frame", which by default contains the rows from the start of the partition through the last peer of the current row. This is likely to give unhelpful results for last_value and sometimes also nth_value. You can redefine the frame by adding a suitable frame specification (RANGE or ROWS) to the OVER clause. See Section 4.2.8 for more information about frame specifications. This query could be fixed by adding `RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING`, but that's quite verbose. It's simpler just to use the first_value function. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/59985 --- lib/gitlab/background_migration/migrate_stage_index.rb | 4 ++-- spec/lib/gitlab/background_migration/migrate_stage_index_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/background_migration/migrate_stage_index.rb b/lib/gitlab/background_migration/migrate_stage_index.rb index f90f35a913d..f921233460d 100644 --- a/lib/gitlab/background_migration/migrate_stage_index.rb +++ b/lib/gitlab/background_migration/migrate_stage_index.rb @@ -21,8 +21,8 @@ module Gitlab AND stage_idx IS NOT NULL GROUP BY stage_id, stage_idx ), indexes AS ( - SELECT DISTINCT stage_id, last_value(stage_idx) - OVER (PARTITION BY stage_id ORDER BY freq ASC) AS index + SELECT DISTINCT stage_id, first_value(stage_idx) + OVER (PARTITION BY stage_id ORDER BY freq DESC) AS index FROM freqs ) diff --git a/spec/lib/gitlab/background_migration/migrate_stage_index_spec.rb b/spec/lib/gitlab/background_migration/migrate_stage_index_spec.rb index f8107dd40b9..4db829b1e7b 100644 --- a/spec/lib/gitlab/background_migration/migrate_stage_index_spec.rb +++ b/spec/lib/gitlab/background_migration/migrate_stage_index_spec.rb @@ -30,6 +30,6 @@ describe Gitlab::BackgroundMigration::MigrateStageIndex, :migration, schema: 201 described_class.new.perform(100, 101) - expect(stages.all.pluck(:position)).to eq [2, 3] + expect(stages.all.order(:id).pluck(:position)).to eq [2, 3] end end -- cgit v1.2.1 From 29a68cc5bc0583b61457dedc78529f97491558f0 Mon Sep 17 00:00:00 2001 From: Vasiliy Ermolovich Date: Wed, 3 Apr 2019 23:59:14 +0300 Subject: Use Rack::Utils.clean_path_info instead of copy-pasted version. --- app/controllers/help_controller.rb | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/app/controllers/help_controller.rb b/app/controllers/help_controller.rb index a9d6addd4a4..10cdce98437 100644 --- a/app/controllers/help_controller.rb +++ b/app/controllers/help_controller.rb @@ -22,7 +22,7 @@ class HelpController < ApplicationController end def show - @path = clean_path_info(path_params[:path]) + @path = Rack::Utils.clean_path_info(path_params[:path]) respond_to do |format| format.any(:markdown, :md, :html) do @@ -75,35 +75,4 @@ class HelpController < ApplicationController params end - - PATH_SEPS = Regexp.union(*[::File::SEPARATOR, ::File::ALT_SEPARATOR].compact) - - # Taken from ActionDispatch::FileHandler - # Cleans up the path, to prevent directory traversal outside the doc folder. - def clean_path_info(path_info) - parts = path_info.split(PATH_SEPS) - - clean = [] - - # Walk over each part of the path - parts.each do |part| - # Turn `one//two` or `one/./two` into `one/two`. - next if part.empty? || part == '.' - - if part == '..' - # Turn `one/two/../` into `one` - clean.pop - else - # Add simple folder names to the clean path. - clean << part - end - end - - # If the path was an absolute path (i.e. `/` or `/one/two`), - # add `/` to the front of the clean path. - clean.unshift '/' if parts.empty? || parts.first.empty? - - # Join all the clean path parts by the path separator. - ::File.join(*clean) - end end -- cgit v1.2.1 From 0adedbb4822a8daaa215b33c88ace136c31d042d Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Mon, 1 Apr 2019 15:11:08 +0900 Subject: Fix the bug that the project statistics is not updated --- app/workers/all_queues.yml | 1 + app/workers/project_cache_worker.rb | 14 ++++--- app/workers/update_project_statistics_worker.rb | 22 ++++++++++ changelogs/unreleased/delay-update-statictics.yml | 5 +++ config/sidekiq_queues.yml | 3 +- spec/workers/project_cache_worker_spec.rb | 36 +++++++++------- .../update_project_statistics_worker_spec.rb | 48 ++++++++++++++++++++++ 7 files changed, 107 insertions(+), 22 deletions(-) create mode 100644 app/workers/update_project_statistics_worker.rb create mode 100644 changelogs/unreleased/delay-update-statictics.yml create mode 100644 spec/workers/update_project_statistics_worker_spec.rb diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 3e8c2a1209a..f9b2e698fc9 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -143,6 +143,7 @@ - repository_remove_remote - system_hook_push - update_merge_requests +- update_project_statistics - upload_checksum - web_hook - repository_update_remote_mirror diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index b31099bc670..73eb461768c 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -28,18 +28,20 @@ class ProjectCacheWorker def update_statistics(project, statistics = []) return if Gitlab::Database.read_only? - return unless try_obtain_lease_for(project.id, :update_statistics) + return unless try_obtain_lease_for(project.id, statistics) - Rails.logger.info("Updating statistics for project #{project.id}") - - project.statistics.refresh!(only: statistics) + UpdateProjectStatisticsWorker.perform_in(LEASE_TIMEOUT, project.id, statistics) end private - def try_obtain_lease_for(project_id, section) + def try_obtain_lease_for(project_id, statistics) Gitlab::ExclusiveLease - .new("project_cache_worker:#{project_id}:#{section}", timeout: LEASE_TIMEOUT) + .new(project_cache_worker_key(project_id, statistics), timeout: LEASE_TIMEOUT) .try_obtain end + + def project_cache_worker_key(project_id, statistics) + (["project_cache_worker", project_id] + statistics.sort).join(":") + end end diff --git a/app/workers/update_project_statistics_worker.rb b/app/workers/update_project_statistics_worker.rb new file mode 100644 index 00000000000..46673c4b07e --- /dev/null +++ b/app/workers/update_project_statistics_worker.rb @@ -0,0 +1,22 @@ + +# frozen_string_literal: true + +# Worker for updating project statistics. +class UpdateProjectStatisticsWorker + include ApplicationWorker + + # project_id - The ID of the project for which to flush the cache. + # statistics - An Array containing columns from ProjectStatistics to + # refresh, if empty all columns will be refreshed + # rubocop: disable CodeReuse/ActiveRecord + def perform(project_id, statistics = []) + project = Project.find_by(id: project_id) + + return unless project && project.repository.exists? + + Rails.logger.info("Updating statistics for project #{project.id}") + + project.statistics.refresh!(only: statistics.map(&:to_sym)) + end + # rubocop: enable CodeReuse/ActiveRecord +end diff --git a/changelogs/unreleased/delay-update-statictics.yml b/changelogs/unreleased/delay-update-statictics.yml new file mode 100644 index 00000000000..d0201fb6db8 --- /dev/null +++ b/changelogs/unreleased/delay-update-statictics.yml @@ -0,0 +1,5 @@ +--- +title: Fix the bug that the project statistics is not updated +merge_request: 26854 +author: Hiroyuki Sato +type: fixed diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 2dc0da00919..8bc2426ec4c 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -89,4 +89,5 @@ - [project_daily_statistics, 1] - [import_issues_csv, 2] - [chat_notification, 2] - - [migrate_external_diffs, 1] + - [migrate_external_diffs, 1] + - [update_project_statistics, 1] diff --git a/spec/workers/project_cache_worker_spec.rb b/spec/workers/project_cache_worker_spec.rb index a7353227043..724947228b7 100644 --- a/spec/workers/project_cache_worker_spec.rb +++ b/spec/workers/project_cache_worker_spec.rb @@ -7,9 +7,9 @@ describe ProjectCacheWorker do let(:worker) { described_class.new } let(:project) { create(:project, :repository) } - let(:statistics) { project.statistics } - let(:lease_key) { "project_cache_worker:#{project.id}:update_statistics" } + let(:lease_key) { (["project_cache_worker", project.id] + statistics.sort).join(":") } let(:lease_timeout) { ProjectCacheWorker::LEASE_TIMEOUT } + let(:statistics) { [] } describe '#perform' do before do @@ -35,14 +35,6 @@ describe ProjectCacheWorker do end context 'with an existing project' do - it 'updates the project statistics' do - expect(worker).to receive(:update_statistics) - .with(kind_of(Project), %i(repository_size)) - .and_call_original - - worker.perform(project.id, [], %w(repository_size)) - end - it 'refreshes the method caches' do expect_any_instance_of(Repository).to receive(:refresh_method_caches) .with(%i(readme)) @@ -51,6 +43,18 @@ describe ProjectCacheWorker do worker.perform(project.id, %w(readme)) end + context 'with statistics' do + let(:statistics) { %w(repository_size) } + + it 'updates the project statistics' do + expect(worker).to receive(:update_statistics) + .with(kind_of(Project), %i(repository_size)) + .and_call_original + + worker.perform(project.id, [], statistics) + end + end + context 'with plain readme' do it 'refreshes the method caches' do allow(MarkupHelper).to receive(:gitlab_markdown?).and_return(false) @@ -66,13 +70,15 @@ describe ProjectCacheWorker do end describe '#update_statistics' do + let(:statistics) { %w(repository_size) } + context 'when a lease could not be obtained' do it 'does not update the repository size' do stub_exclusive_lease_taken(lease_key, timeout: lease_timeout) - expect(statistics).not_to receive(:refresh!) + expect(UpdateProjectStatisticsWorker).not_to receive(:perform_in) - worker.update_statistics(project) + worker.update_statistics(project, statistics.map(&:to_sym)) end end @@ -80,11 +86,11 @@ describe ProjectCacheWorker do it 'updates the project statistics' do stub_exclusive_lease(lease_key, timeout: lease_timeout) - expect(statistics).to receive(:refresh!) - .with(only: %i(repository_size)) + expect(UpdateProjectStatisticsWorker).to receive(:perform_in) + .with(lease_timeout, project.id, statistics.map(&:to_sym)) .and_call_original - worker.update_statistics(project, %i(repository_size)) + worker.update_statistics(project, statistics.map(&:to_sym)) end end end diff --git a/spec/workers/update_project_statistics_worker_spec.rb b/spec/workers/update_project_statistics_worker_spec.rb new file mode 100644 index 00000000000..3411e10da7e --- /dev/null +++ b/spec/workers/update_project_statistics_worker_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper' + +describe UpdateProjectStatisticsWorker do + let(:worker) { described_class.new } + let(:project) { create(:project, :repository) } + + describe '#perform' do + context 'with a non-existing project' do + it 'does nothing' do + expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + + worker.perform(-1) + end + end + + context 'with an existing project without a repository' do + it 'does nothing' do + allow_any_instance_of(Repository).to receive(:exists?).and_return(false) + + expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + + worker.perform(project.id) + end + end + + context 'with an existing project' do + it 'refreshes the project statistics' do + expect_any_instance_of(ProjectStatistics).to receive(:refresh!) + .with(only: []) + .and_call_original + + worker.perform(project.id) + end + + context 'with a specific statistics target' do + it 'refreshes the project repository size' do + statistics_target = %w(repository_size) + + expect_any_instance_of(ProjectStatistics).to receive(:refresh!) + .with(only: statistics_target.map(&:to_sym)) + .and_call_original + + worker.perform(project.id, statistics_target) + end + end + end + end +end -- cgit v1.2.1 From e6780501cbfd0cae31fadd15b2964204171091cc Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Fri, 5 Apr 2019 00:17:52 +0900 Subject: Refactor project_cache_worker_key --- app/workers/project_cache_worker.rb | 2 +- spec/workers/project_cache_worker_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index 73eb461768c..2b25b96a6f8 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -42,6 +42,6 @@ class ProjectCacheWorker end def project_cache_worker_key(project_id, statistics) - (["project_cache_worker", project_id] + statistics.sort).join(":") + ["project_cache_worker", project_id, *statistics.sort].join(":") end end diff --git a/spec/workers/project_cache_worker_spec.rb b/spec/workers/project_cache_worker_spec.rb index 724947228b7..7053bfc2bfd 100644 --- a/spec/workers/project_cache_worker_spec.rb +++ b/spec/workers/project_cache_worker_spec.rb @@ -7,7 +7,7 @@ describe ProjectCacheWorker do let(:worker) { described_class.new } let(:project) { create(:project, :repository) } - let(:lease_key) { (["project_cache_worker", project.id] + statistics.sort).join(":") } + let(:lease_key) { ["project_cache_worker", project.id, *statistics.sort].join(":") } let(:lease_timeout) { ProjectCacheWorker::LEASE_TIMEOUT } let(:statistics) { [] } -- cgit v1.2.1 From 074a1797fe581c8eb5cc65bd56af584d5c500688 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Fri, 5 Apr 2019 00:18:17 +0900 Subject: Update the project statistics immediatelly --- app/workers/project_cache_worker.rb | 7 +++++++ spec/workers/project_cache_worker_spec.rb | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index 2b25b96a6f8..5f4972d8f93 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -26,10 +26,17 @@ class ProjectCacheWorker end # rubocop: enable CodeReuse/ActiveRecord + # NOTE: triggering both an immediate update and one in 15 minutes if we + # successfully obtain the lease. That way, we only need to wait for the + # statistics to become accurate if they were already updated once in the + # last 15 minutes. def update_statistics(project, statistics = []) return if Gitlab::Database.read_only? return unless try_obtain_lease_for(project.id, statistics) + Rails.logger.info("Updating statistics for project #{project.id}") + + project.statistics.refresh!(only: statistics) UpdateProjectStatisticsWorker.perform_in(LEASE_TIMEOUT, project.id, statistics) end diff --git a/spec/workers/project_cache_worker_spec.rb b/spec/workers/project_cache_worker_spec.rb index 7053bfc2bfd..d2445f420f8 100644 --- a/spec/workers/project_cache_worker_spec.rb +++ b/spec/workers/project_cache_worker_spec.rb @@ -86,6 +86,10 @@ describe ProjectCacheWorker do it 'updates the project statistics' do stub_exclusive_lease(lease_key, timeout: lease_timeout) + expect(project.statistics).to receive(:refresh!) + .with(only: statistics.map(&:to_sym)) + .and_call_original + expect(UpdateProjectStatisticsWorker).to receive(:perform_in) .with(lease_timeout, project.id, statistics.map(&:to_sym)) .and_call_original -- cgit v1.2.1 From 4406acd5058ac58b61a203ee0da7e9712c70a3bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rory=20O=E2=80=99Kane?= Date: Thu, 4 Apr 2019 20:58:28 +0000 Subject: =?UTF-8?q?Fix=20typo=20=E2=80=9Csettings=E2=80=9D=20in=20Merge=20?= =?UTF-8?q?requests=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- doc/user/project/merge_requests/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md index 01a3a5bbbe1..7c0380152de 100644 --- a/doc/user/project/merge_requests/index.md +++ b/doc/user/project/merge_requests/index.md @@ -254,7 +254,7 @@ To prevent merge requests from accidentally being accepted before they're completely ready, GitLab blocks the "Accept" button for merge requests that have been marked as a **Work In Progress**. -[Learn more about settings a merge request as "Work In Progress".](work_in_progress_merge_requests.md) +[Learn more about setting a merge request as "Work In Progress".](work_in_progress_merge_requests.md) ## Merge request diff file navigation -- cgit v1.2.1 From 84ec11131dee8a0137bc8d8b6ab9f6a1e452ed11 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Fri, 5 Apr 2019 20:41:58 +0800 Subject: Add doc when rspec-set won't work --- doc/development/testing_guide/best_practices.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md index 7262c04d746..e41148360f2 100644 --- a/doc/development/testing_guide/best_practices.md +++ b/doc/development/testing_guide/best_practices.md @@ -216,8 +216,10 @@ project, one project will do for the entire file. This can be achieved by using reloads or recreates the model, _only_ if needed. That is, when you changed properties or destroyed the object. -There is one gotcha; you can't reference a model defined in a `let` block in a -`set` block. +Note that you can't reference a model defined in a `let` block in a `set` block. + +Also, `set` is not supported in `:js` specs since those don't use transactions +to clean up database state after each example. ### Time-sensitive tests -- cgit v1.2.1 From 770f721962cd30e930ab7b6e06e9386da6325c3c Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Fri, 5 Apr 2019 22:07:09 +0900 Subject: Refactor: extract duplicate steps to a service class --- app/services/projects/update_statistics_service.rb | 19 ++++++++++ app/workers/project_cache_worker.rb | 5 +-- app/workers/update_project_statistics_worker.rb | 6 +-- .../projects/update_statistics_service_spec.rb | 40 ++++++++++++++++++++ spec/workers/project_cache_worker_spec.rb | 19 ++++++---- .../update_project_statistics_worker_spec.rb | 43 +++------------------- 6 files changed, 79 insertions(+), 53 deletions(-) create mode 100644 app/services/projects/update_statistics_service.rb create mode 100644 spec/services/projects/update_statistics_service_spec.rb diff --git a/app/services/projects/update_statistics_service.rb b/app/services/projects/update_statistics_service.rb new file mode 100644 index 00000000000..f32a779fab0 --- /dev/null +++ b/app/services/projects/update_statistics_service.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Projects + class UpdateStatisticsService < BaseService + def execute + return unless project && project.repository.exists? + + Rails.logger.info("Updating statistics for project #{project.id}") + + project.statistics.refresh!(only: statistics.map(&:to_sym)) + end + + private + + def statistics + params[:statistics] + end + end +end diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index 5f4972d8f93..b2e0701008a 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -18,7 +18,7 @@ class ProjectCacheWorker return unless project && project.repository.exists? - update_statistics(project, statistics.map(&:to_sym)) + update_statistics(project, statistics) project.repository.refresh_method_caches(files.map(&:to_sym)) @@ -34,9 +34,8 @@ class ProjectCacheWorker return if Gitlab::Database.read_only? return unless try_obtain_lease_for(project.id, statistics) - Rails.logger.info("Updating statistics for project #{project.id}") + Projects::UpdateStatisticsService.new(project, nil, statistics: statistics).execute - project.statistics.refresh!(only: statistics) UpdateProjectStatisticsWorker.perform_in(LEASE_TIMEOUT, project.id, statistics) end diff --git a/app/workers/update_project_statistics_worker.rb b/app/workers/update_project_statistics_worker.rb index 46673c4b07e..9a29cc12707 100644 --- a/app/workers/update_project_statistics_worker.rb +++ b/app/workers/update_project_statistics_worker.rb @@ -12,11 +12,7 @@ class UpdateProjectStatisticsWorker def perform(project_id, statistics = []) project = Project.find_by(id: project_id) - return unless project && project.repository.exists? - - Rails.logger.info("Updating statistics for project #{project.id}") - - project.statistics.refresh!(only: statistics.map(&:to_sym)) + Projects::UpdateStatisticsService.new(project, nil, statistics: statistics).execute end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/spec/services/projects/update_statistics_service_spec.rb b/spec/services/projects/update_statistics_service_spec.rb new file mode 100644 index 00000000000..7e351c9ce54 --- /dev/null +++ b/spec/services/projects/update_statistics_service_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe Projects::UpdateStatisticsService do + let(:service) { described_class.new(project, nil, statistics: statistics)} + let(:statistics) { %w(repository_size) } + + describe '#execute' do + context 'with a non-existing project' do + let(:project) { nil } + + it 'does nothing' do + expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + + service.execute + end + end + + context 'with an existing project without a repository' do + let(:project) { create(:project) } + + it 'does nothing' do + expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + + service.execute + end + end + + context 'with an existing project with a repository' do + let(:project) { create(:project, :repository) } + + it 'refreshes the project statistics' do + expect_any_instance_of(ProjectStatistics).to receive(:refresh!) + .with(only: statistics.map(&:to_sym)) + .and_call_original + + service.execute + end + end + end +end diff --git a/spec/workers/project_cache_worker_spec.rb b/spec/workers/project_cache_worker_spec.rb index d2445f420f8..3c40269adc7 100644 --- a/spec/workers/project_cache_worker_spec.rb +++ b/spec/workers/project_cache_worker_spec.rb @@ -48,7 +48,7 @@ describe ProjectCacheWorker do it 'updates the project statistics' do expect(worker).to receive(:update_statistics) - .with(kind_of(Project), %i(repository_size)) + .with(kind_of(Project), statistics) .and_call_original worker.perform(project.id, [], statistics) @@ -73,28 +73,31 @@ describe ProjectCacheWorker do let(:statistics) { %w(repository_size) } context 'when a lease could not be obtained' do - it 'does not update the repository size' do + it 'does not update the project statistics' do stub_exclusive_lease_taken(lease_key, timeout: lease_timeout) + expect(Projects::UpdateStatisticsService).not_to receive(:new) + expect(UpdateProjectStatisticsWorker).not_to receive(:perform_in) - worker.update_statistics(project, statistics.map(&:to_sym)) + worker.update_statistics(project, statistics) end end context 'when a lease could be obtained' do - it 'updates the project statistics' do + it 'updates the project statistics twice' do stub_exclusive_lease(lease_key, timeout: lease_timeout) - expect(project.statistics).to receive(:refresh!) - .with(only: statistics.map(&:to_sym)) + expect(Projects::UpdateStatisticsService).to receive(:new) + .with(project, nil, statistics: statistics) .and_call_original + .twice expect(UpdateProjectStatisticsWorker).to receive(:perform_in) - .with(lease_timeout, project.id, statistics.map(&:to_sym)) + .with(lease_timeout, project.id, statistics) .and_call_original - worker.update_statistics(project, statistics.map(&:to_sym)) + worker.update_statistics(project, statistics) end end end diff --git a/spec/workers/update_project_statistics_worker_spec.rb b/spec/workers/update_project_statistics_worker_spec.rb index 3411e10da7e..a268fd2e4ba 100644 --- a/spec/workers/update_project_statistics_worker_spec.rb +++ b/spec/workers/update_project_statistics_worker_spec.rb @@ -3,46 +3,15 @@ require 'spec_helper' describe UpdateProjectStatisticsWorker do let(:worker) { described_class.new } let(:project) { create(:project, :repository) } + let(:statistics) { %w(repository_size) } describe '#perform' do - context 'with a non-existing project' do - it 'does nothing' do - expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + it 'updates the project statistics' do + expect(Projects::UpdateStatisticsService).to receive(:new) + .with(project, nil, statistics: statistics) + .and_call_original - worker.perform(-1) - end - end - - context 'with an existing project without a repository' do - it 'does nothing' do - allow_any_instance_of(Repository).to receive(:exists?).and_return(false) - - expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) - - worker.perform(project.id) - end - end - - context 'with an existing project' do - it 'refreshes the project statistics' do - expect_any_instance_of(ProjectStatistics).to receive(:refresh!) - .with(only: []) - .and_call_original - - worker.perform(project.id) - end - - context 'with a specific statistics target' do - it 'refreshes the project repository size' do - statistics_target = %w(repository_size) - - expect_any_instance_of(ProjectStatistics).to receive(:refresh!) - .with(only: statistics_target.map(&:to_sym)) - .and_call_original - - worker.perform(project.id, statistics_target) - end - end + worker.perform(project.id, statistics) end end end -- cgit v1.2.1 From d8806eef767644318de320b3f312e8a9a3de1b4f Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 5 Apr 2019 15:42:17 +0100 Subject: Adds EE folder in the stylelint command --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index a2d96cc551a..5a940ded7ba 100644 --- a/package.json +++ b/package.json @@ -16,7 +16,7 @@ "prettier-staged-save": "node ./scripts/frontend/prettier.js save", "prettier-all": "node ./scripts/frontend/prettier.js check-all", "prettier-all-save": "node ./scripts/frontend/prettier.js save-all", - "stylelint": "node node_modules/stylelint/bin/stylelint.js app/assets/stylesheets/**/*.* --custom-formatter node_modules/stylelint-error-string-formatter", + "stylelint": "node node_modules/stylelint/bin/stylelint.js app/assets/stylesheets/**/*.* ee/app/assets/stylesheets/**/*.* --custom-formatter node_modules/stylelint-error-string-formatter", "stylelint-file": "node node_modules/stylelint/bin/stylelint.js", "stylelint-create-utility-map": "node scripts/frontend/stylelint/stylelint-utility-map.js", "test": "yarn jest && yarn karma", -- cgit v1.2.1 From e46d4bf4da3ee207043c85524df238475e47d650 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Thu, 28 Mar 2019 14:59:24 +0000 Subject: Extract a Git::{Base,Tag,Branch}HooksService --- app/models/gpg_signature.rb | 9 + app/services/after_branch_delete_service.rb | 20 -- app/services/delete_branch_service.rb | 10 - app/services/git/base_hooks_service.rb | 94 ++++++ app/services/git/branch_hooks_service.rb | 144 +++++++++ app/services/git/branch_push_service.rb | 215 ++------------ app/services/git/tag_hooks_service.rb | 36 +++ app/services/git/tag_push_service.rb | 50 +--- spec/services/after_branch_delete_service_spec.rb | 15 - spec/services/git/branch_hooks_service_spec.rb | 339 ++++++++++++++++++++++ spec/services/git/branch_push_service_spec.rb | 258 +++------------- spec/services/git/tag_hooks_service_spec.rb | 144 +++++++++ spec/services/git/tag_push_service_spec.rb | 179 +----------- spec/workers/post_receive_spec.rb | 25 +- 14 files changed, 879 insertions(+), 659 deletions(-) delete mode 100644 app/services/after_branch_delete_service.rb create mode 100644 app/services/git/base_hooks_service.rb create mode 100644 app/services/git/branch_hooks_service.rb create mode 100644 app/services/git/tag_hooks_service.rb delete mode 100644 spec/services/after_branch_delete_service_spec.rb create mode 100644 spec/services/git/branch_hooks_service_spec.rb create mode 100644 spec/services/git/tag_hooks_service_spec.rb diff --git a/app/models/gpg_signature.rb b/app/models/gpg_signature.rb index 7f9ff7bbda6..46cac1d41bb 100644 --- a/app/models/gpg_signature.rb +++ b/app/models/gpg_signature.rb @@ -38,6 +38,15 @@ class GpgSignature < ApplicationRecord .safe_find_or_create_by!(commit_sha: attributes[:commit_sha]) end + # Find commits that are lacking a signature in the database at present + def self.unsigned_commit_shas(commit_shas) + return [] if commit_shas.empty? + + signed = GpgSignature.where(commit_sha: commit_shas).pluck(:commit_sha) + + commit_shas - signed + end + def gpg_key=(model) case model when GpgKey diff --git a/app/services/after_branch_delete_service.rb b/app/services/after_branch_delete_service.rb deleted file mode 100644 index ece9fbbef43..00000000000 --- a/app/services/after_branch_delete_service.rb +++ /dev/null @@ -1,20 +0,0 @@ -# frozen_string_literal: true - -# Branch can be deleted either by DeleteBranchService or by Git::BranchPushService. -class AfterBranchDeleteService < BaseService - attr_reader :branch_name - - def execute(branch_name) - @branch_name = branch_name - - stop_environments - end - - private - - def stop_environments - Ci::StopEnvironmentsService - .new(project, current_user) - .execute(branch_name) - end -end diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb index 8322a3d74f4..4c3ac19f754 100644 --- a/app/services/delete_branch_service.rb +++ b/app/services/delete_branch_service.rb @@ -29,14 +29,4 @@ class DeleteBranchService < BaseService def success(message) super().merge(message: message) end - - def build_push_data(branch) - Gitlab::DataBuilder::Push.build( - project, - current_user, - branch.dereferenced_target.sha, - Gitlab::Git::BLANK_SHA, - "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch.name}", - []) - end end diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb new file mode 100644 index 00000000000..e0dccb8716a --- /dev/null +++ b/app/services/git/base_hooks_service.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +module Git + class BaseHooksService < ::BaseService + include Gitlab::Utils::StrongMemoize + + # The N most recent commits to process in a single push payload. + PROCESS_COMMIT_LIMIT = 100 + + def execute + project.repository.after_create if project.empty_repo? + + create_events + create_pipelines + execute_project_hooks + + # Not a hook, but it needs access to the list of changed commits + enqueue_invalidate_cache + + push_data + end + + private + + def hook_name + raise NotImplementedError, "Please implement #{self.class}##{__method__}" + end + + def commits + raise NotImplementedError, "Please implement #{self.class}##{__method__}" + end + + def limited_commits + commits.last(PROCESS_COMMIT_LIMIT) + end + + def commits_count + commits.count + end + + def event_message + nil + end + + def invalidated_file_types + [] + end + + def create_events + EventCreateService.new.push(project, current_user, push_data) + end + + def create_pipelines + Ci::CreatePipelineService + .new(project, current_user, push_data) + .execute(:push, pipeline_options) + end + + def execute_project_hooks + project.execute_hooks(push_data, hook_name) + project.execute_services(push_data, hook_name) + end + + def enqueue_invalidate_cache + ProjectCacheWorker.perform_async( + project.id, + invalidated_file_types, + [:commit_count, :repository_size] + ) + end + + def push_data + @push_data ||= Gitlab::DataBuilder::Push.build( + project, + current_user, + params[:oldrev], + params[:newrev], + params[:ref], + limited_commits, + event_message, + commits_count: commits_count, + push_options: params[:push_options] || [] + ) + + # Dependent code may modify the push data, so return a duplicate each time + @push_data.dup + end + + # to be overridden in EE + def pipeline_options + {} + end + end +end diff --git a/app/services/git/branch_hooks_service.rb b/app/services/git/branch_hooks_service.rb new file mode 100644 index 00000000000..d21a6bb1b9a --- /dev/null +++ b/app/services/git/branch_hooks_service.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +module Git + class BranchHooksService < ::Git::BaseHooksService + def execute + execute_branch_hooks + + super.tap do + enqueue_update_gpg_signatures + end + end + + private + + def hook_name + :push_hooks + end + + def commits + strong_memoize(:commits) do + if creating_default_branch? + # The most recent PROCESS_COMMIT_LIMIT commits in the default branch + offset = [count_commits_in_branch - PROCESS_COMMIT_LIMIT, 0].max + project.repository.commits(params[:newrev], offset: offset, limit: PROCESS_COMMIT_LIMIT) + elsif creating_branch? + # Use the pushed commits that aren't reachable by the default branch + # as a heuristic. This may include more commits than are actually + # pushed, but that shouldn't matter because we check for existing + # cross-references later. + project.repository.commits_between(project.default_branch, params[:newrev]) + elsif updating_branch? + project.repository.commits_between(params[:oldrev], params[:newrev]) + else # removing branch + [] + end + end + end + + def commits_count + return count_commits_in_branch if creating_default_branch? + + super + end + + def invalidated_file_types + return super unless default_branch? && !creating_branch? + + paths = limited_commits.each_with_object(Set.new) do |commit, set| + commit.raw_deltas.each do |diff| + set << diff.new_path + end + end + + Gitlab::FileDetector.types_in_paths(paths) + end + + def execute_branch_hooks + project.repository.after_push_commit(branch_name) + + branch_create_hooks if creating_branch? + branch_update_hooks if updating_branch? + branch_change_hooks if creating_branch? || updating_branch? + branch_remove_hooks if removing_branch? + end + + def branch_create_hooks + project.repository.after_create_branch + project.after_create_default_branch if default_branch? + end + + def branch_update_hooks + # Update the bare repositories info/attributes file using the contents of + # the default branch's .gitattributes file + project.repository.copy_gitattributes(params[:ref]) if default_branch? + end + + def branch_change_hooks + enqueue_process_commit_messages + end + + def branch_remove_hooks + project.repository.after_remove_branch + end + + # Schedules processing of commit messages + def enqueue_process_commit_messages + # don't process commits for the initial push to the default branch + return if creating_default_branch? + + limited_commits.each do |commit| + next unless commit.matches_cross_reference_regex? + + ProcessCommitWorker.perform_async( + project.id, + current_user.id, + commit.to_hash, + default_branch? + ) + end + end + + def enqueue_update_gpg_signatures + unsigned = GpgSignature.unsigned_commit_shas(limited_commits.map(&:sha)) + return if unsigned.empty? + + signable = Gitlab::Git::Commit.shas_with_signatures(project.repository, unsigned) + return if signable.empty? + + CreateGpgSignatureWorker.perform_async(signable, project.id) + end + + def creating_branch? + Gitlab::Git.blank_ref?(params[:oldrev]) + end + + def updating_branch? + !creating_branch? && !removing_branch? + end + + def removing_branch? + Gitlab::Git.blank_ref?(params[:newrev]) + end + + def creating_default_branch? + creating_branch? && default_branch? + end + + def count_commits_in_branch + strong_memoize(:count_commits_in_branch) do + project.repository.commit_count_for_ref(params[:ref]) + end + end + + def default_branch? + strong_memoize(:default_branch) do + [nil, branch_name].include?(project.default_branch) + end + end + + def branch_name + strong_memoize(:branch_name) { Gitlab::Git.ref_name(params[:ref]) } + end + end +end diff --git a/app/services/git/branch_push_service.rb b/app/services/git/branch_push_service.rb index b55aeb5f2b9..da053ce80c7 100644 --- a/app/services/git/branch_push_service.rb +++ b/app/services/git/branch_push_service.rb @@ -1,14 +1,10 @@ # frozen_string_literal: true module Git - class BranchPushService < BaseService - attr_accessor :push_data, :push_commits + class BranchPushService < ::BaseService include Gitlab::Access include Gitlab::Utils::StrongMemoize - # The N most recent commits to process in a single push payload. - PROCESS_COMMIT_LIMIT = 100 - # This method will be called after each git update # and only if the provided user and project are present in GitLab. # @@ -23,108 +19,43 @@ module Git # 6. Checks if the project's main language has changed # def execute - update_commits + return unless Gitlab::Git.branch_ref?(params[:ref]) + + enqueue_update_mrs + enqueue_detect_repository_languages + execute_related_hooks perform_housekeeping update_remote_mirrors - update_caches + stop_environments - update_signatures + true end - def update_commits - project.repository.after_create if project.empty_repo? - project.repository.after_push_commit(branch_name) - - if push_remove_branch? - project.repository.after_remove_branch - @push_commits = [] - elsif push_to_new_branch? - project.repository.after_create_branch - - # Re-find the pushed commits. - if default_branch? - # Initial push to the default branch. Take the full history of that branch as "newly pushed". - process_default_branch - else - # Use the pushed commits that aren't reachable by the default branch - # as a heuristic. This may include more commits than are actually pushed, but - # that shouldn't matter because we check for existing cross-references later. - @push_commits = project.repository.commits_between(project.default_branch, params[:newrev]) - - # don't process commits for the initial push to the default branch - process_commit_messages - end - elsif push_to_existing_branch? - # Collect data for this git push - @push_commits = project.repository.commits_between(params[:oldrev], params[:newrev]) - - process_commit_messages - - # Update the bare repositories info/attributes file using the contents of the default branches - # .gitattributes file - update_gitattributes if default_branch? - end - end - - def update_gitattributes - project.repository.copy_gitattributes(params[:ref]) - end - - def update_caches - if default_branch? - if push_to_new_branch? - # If this is the initial push into the default branch, the file type caches - # will already be reset as a result of `Project#change_head`. - types = [] - else - paths = Set.new - - last_pushed_commits.each do |commit| - commit.raw_deltas.each do |diff| - paths << diff.new_path - end - end - - types = Gitlab::FileDetector.types_in_paths(paths.to_a) - end - - DetectRepositoryLanguagesWorker.perform_async(@project.id, current_user.id) - else - types = [] - end - - ProjectCacheWorker.perform_async(project.id, types, [:commit_count, :repository_size]) + # Update merge requests that may be affected by this push. A new branch + # could cause the last commit of a merge request to change. + def enqueue_update_mrs + UpdateMergeRequestsWorker.perform_async( + project.id, + current_user.id, + params[:oldrev], + params[:newrev], + params[:ref] + ) end - # rubocop: disable CodeReuse/ActiveRecord - def update_signatures - commit_shas = last_pushed_commits.map(&:sha) + def enqueue_detect_repository_languages + return unless default_branch? - return if commit_shas.empty? - - shas_with_cached_signatures = GpgSignature.where(commit_sha: commit_shas).pluck(:commit_sha) - commit_shas -= shas_with_cached_signatures - - return if commit_shas.empty? - - commit_shas = Gitlab::Git::Commit.shas_with_signatures(project.repository, commit_shas) - - CreateGpgSignatureWorker.perform_async(commit_shas, project.id) + DetectRepositoryLanguagesWorker.perform_async(project.id, current_user.id) end - # rubocop: enable CodeReuse/ActiveRecord - # Schedules processing of commit messages. - def process_commit_messages - default = default_branch? + # Only stop environments if the ref is a branch that is being deleted + def stop_environments + return unless removing_branch? - last_pushed_commits.each do |commit| - if commit.matches_cross_reference_regex? - ProcessCommitWorker - .perform_async(project.id, current_user.id, commit.to_hash, default) - end - end + Ci::StopEnvironmentsService.new(project, current_user).execute(branch_name) end def update_remote_mirrors @@ -135,23 +66,7 @@ module Git end def execute_related_hooks - # Update merge requests that may be affected by this push. A new branch - # could cause the last commit of a merge request to change. - # - UpdateMergeRequestsWorker - .perform_async(project.id, current_user.id, params[:oldrev], params[:newrev], params[:ref]) - - EventCreateService.new.push(project, current_user, build_push_data) - Ci::CreatePipelineService.new(project, current_user, build_push_data).execute(:push, pipeline_options) - - project.execute_hooks(build_push_data.dup, :push_hooks) - project.execute_services(build_push_data.dup, :push_hooks) - - if push_remove_branch? - AfterBranchDeleteService - .new(project, current_user) - .execute(branch_name) - end + BranchHooksService.new(project, current_user, params).execute end def perform_housekeeping @@ -161,84 +76,18 @@ module Git rescue Projects::HousekeepingService::LeaseTaken end - def process_default_branch - offset = [push_commits_count_for_ref - PROCESS_COMMIT_LIMIT, 0].max - @push_commits = project.repository.commits(params[:newrev], offset: offset, limit: PROCESS_COMMIT_LIMIT) - - project.after_create_default_branch - end - - def build_push_data - @push_data ||= Gitlab::DataBuilder::Push.build( - project, - current_user, - params[:oldrev], - params[:newrev], - params[:ref], - @push_commits, - commits_count: commits_count, - push_options: params[:push_options] || [] - ) - end - - def push_to_existing_branch? - # Return if this is not a push to a branch (e.g. new commits) - branch_ref? && !Gitlab::Git.blank_ref?(params[:oldrev]) - end - - def push_to_new_branch? - strong_memoize(:push_to_new_branch) do - branch_ref? && Gitlab::Git.blank_ref?(params[:oldrev]) - end - end - - def push_remove_branch? - strong_memoize(:push_remove_branch) do - branch_ref? && Gitlab::Git.blank_ref?(params[:newrev]) - end - end - - def default_branch? - branch_ref? && - (branch_name == project.default_branch || project.default_branch.nil?) - end - - def commit_user(commit) - commit.author || current_user + def removing_branch? + Gitlab::Git.blank_ref?(params[:newrev]) end def branch_name - strong_memoize(:branch_name) do - Gitlab::Git.ref_name(params[:ref]) - end + strong_memoize(:branch_name) { Gitlab::Git.ref_name(params[:ref]) } end - def branch_ref? - strong_memoize(:branch_ref) do - Gitlab::Git.branch_ref?(params[:ref]) - end - end - - def commits_count - return push_commits_count_for_ref if default_branch? && push_to_new_branch? - - Array(@push_commits).size - end - - def push_commits_count_for_ref - strong_memoize(:push_commits_count_for_ref) do - project.repository.commit_count_for_ref(params[:ref]) + def default_branch? + strong_memoize(:default_branch) do + [nil, branch_name].include?(project.default_branch) end end - - def last_pushed_commits - @last_pushed_commits ||= @push_commits.last(PROCESS_COMMIT_LIMIT) - end - - private - - def pipeline_options - {} # to be overridden in EE - end end end diff --git a/app/services/git/tag_hooks_service.rb b/app/services/git/tag_hooks_service.rb new file mode 100644 index 00000000000..18eb780579f --- /dev/null +++ b/app/services/git/tag_hooks_service.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module Git + class TagHooksService < ::Git::BaseHooksService + private + + def hook_name + :tag_push_hooks + end + + def commits + [tag_commit].compact + end + + def event_message + tag&.message + end + + def tag + strong_memoize(:tag) do + next if Gitlab::Git.blank_ref?(params[:newrev]) + + tag_name = Gitlab::Git.ref_name(params[:ref]) + tag = project.repository.find_tag(tag_name) + + tag if tag && tag.target == params[:newrev] + end + end + + def tag_commit + strong_memoize(:tag_commit) do + project.commit(tag.dereferenced_target) if tag + end + end + end +end diff --git a/app/services/git/tag_push_service.rb b/app/services/git/tag_push_service.rb index 9ce0fbdb206..ee4166dccd0 100644 --- a/app/services/git/tag_push_service.rb +++ b/app/services/git/tag_push_service.rb @@ -1,56 +1,14 @@ # frozen_string_literal: true module Git - class TagPushService < BaseService - attr_accessor :push_data - + class TagPushService < ::BaseService def execute - project.repository.after_create if project.empty_repo? - project.repository.before_push_tag - - @push_data = build_push_data - - EventCreateService.new.push(project, current_user, push_data) - Ci::CreatePipelineService.new(project, current_user, push_data).execute(:push, pipeline_options) + return unless Gitlab::Git.tag_ref?(params[:ref]) - project.execute_hooks(push_data.dup, :tag_push_hooks) - project.execute_services(push_data.dup, :tag_push_hooks) - - ProjectCacheWorker.perform_async(project.id, [], [:commit_count, :repository_size]) + project.repository.before_push_tag + TagHooksService.new(project, current_user, params).execute true end - - private - - def build_push_data - commits = [] - message = nil - - unless Gitlab::Git.blank_ref?(params[:newrev]) - tag_name = Gitlab::Git.ref_name(params[:ref]) - tag = project.repository.find_tag(tag_name) - - if tag && tag.target == params[:newrev] - commit = project.commit(tag.dereferenced_target) - commits = [commit].compact - message = tag.message - end - end - - Gitlab::DataBuilder::Push.build( - project, - current_user, - params[:oldrev], - params[:newrev], - params[:ref], - commits, - message, - push_options: params[:push_options] || []) - end - - def pipeline_options - {} # to be overridden in EE - end end end diff --git a/spec/services/after_branch_delete_service_spec.rb b/spec/services/after_branch_delete_service_spec.rb deleted file mode 100644 index bc9747d1413..00000000000 --- a/spec/services/after_branch_delete_service_spec.rb +++ /dev/null @@ -1,15 +0,0 @@ -require 'spec_helper' - -describe AfterBranchDeleteService do - let(:project) { create(:project, :repository) } - let(:user) { create(:user) } - let(:service) { described_class.new(project, user) } - - describe '#execute' do - it 'stops environments attached to branch' do - expect(service).to receive(:stop_environments) - - service.execute('feature') - end - end -end diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb new file mode 100644 index 00000000000..bb87267db7d --- /dev/null +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -0,0 +1,339 @@ +require 'spec_helper' + +describe Git::BranchHooksService do + include RepoHelpers + + let(:project) { create(:project, :repository) } + let(:user) { project.creator } + + let(:branch) { project.default_branch } + let(:ref) { "refs/heads/#{branch}" } + let(:commit) { project.commit(sample_commit.id) } + let(:oldrev) { commit.parent_id } + let(:newrev) { commit.id } + + let(:service) do + described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + end + + describe "Git Push Data" do + subject(:push_data) { service.execute } + + it 'has expected push data attributes' do + is_expected.to match a_hash_including( + object_kind: 'push', + before: oldrev, + after: newrev, + ref: ref, + user_id: user.id, + user_name: user.name, + project_id: project.id + ) + end + + context "with repository data" do + subject { push_data[:repository] } + + it 'has expected attributes' do + is_expected.to match a_hash_including( + name: project.name, + url: project.url_to_repo, + description: project.description, + homepage: project.web_url + ) + end + end + + context "with commits" do + subject { push_data[:commits] } + + it { is_expected.to be_an(Array) } + + it 'has 1 element' do + expect(subject.size).to eq(1) + end + + context "the commit" do + subject { push_data[:commits].first } + + it { expect(subject[:timestamp].in_time_zone).to eq(commit.date.in_time_zone) } + + it 'includes expected commit data' do + is_expected.to match a_hash_including( + id: commit.id, + message: commit.safe_message, + url: [ + Gitlab.config.gitlab.url, + project.namespace.to_param, + project.to_param, + 'commit', + commit.id + ].join('/') + ) + end + + context "with a author" do + subject { push_data[:commits].first[:author] } + + it 'includes expected author data' do + is_expected.to match a_hash_including( + name: commit.author_name, + email: commit.author_email + ) + end + end + end + end + end + + describe 'Push Event' do + let(:event) { Event.find_by_action(Event::PUSHED) } + + before do + service.execute + end + + context "with an existing branch" do + it 'generates a push event with one commit' do + expect(event).to be_an_instance_of(PushEvent) + expect(event.project).to eq(project) + expect(event.action).to eq(Event::PUSHED) + expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) + expect(event.push_event_payload.commit_from).to eq(oldrev) + expect(event.push_event_payload.commit_to).to eq(newrev) + expect(event.push_event_payload.ref).to eq('master') + expect(event.push_event_payload.commit_count).to eq(1) + end + end + + context "with a new branch" do + let(:oldrev) { Gitlab::Git::BLANK_SHA } + + it 'generates a push event with more than one commit' do + expect(event).to be_an_instance_of(PushEvent) + expect(event.project).to eq(project) + expect(event.action).to eq(Event::PUSHED) + expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) + expect(event.push_event_payload.commit_from).to be_nil + expect(event.push_event_payload.commit_to).to eq(newrev) + expect(event.push_event_payload.ref).to eq('master') + expect(event.push_event_payload.commit_count).to be > 1 + end + end + + context 'removing a branch' do + let(:newrev) { Gitlab::Git::BLANK_SHA } + + it 'generates a push event with no commits' do + expect(event).to be_an_instance_of(PushEvent) + expect(event.project).to eq(project) + expect(event.action).to eq(Event::PUSHED) + expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) + expect(event.push_event_payload.commit_from).to eq(oldrev) + expect(event.push_event_payload.commit_to).to be_nil + expect(event.push_event_payload.ref).to eq('master') + expect(event.push_event_payload.commit_count).to eq(0) + end + end + end + + describe 'Invalidating project cache' do + let(:commit_id) do + project.repository.update_file( + user, 'README.md', '', message: 'Update', branch_name: branch + ) + end + + let(:commit) { project.repository.commit(commit_id) } + let(:blank_sha) { Gitlab::Git::BLANK_SHA } + + def clears_cache(extended: []) + expect(ProjectCacheWorker) + .to receive(:perform_async) + .with(project.id, extended, %i[commit_count repository_size]) + + service.execute + end + + def clears_extended_cache + clears_cache(extended: %i[readme]) + end + + context 'on default branch' do + context 'create' do + # FIXME: When creating the default branch,the cache worker runs twice + before do + allow(ProjectCacheWorker).to receive(:perform_async) + end + + let(:oldrev) { blank_sha } + + it { clears_cache } + end + + context 'update' do + it { clears_extended_cache } + end + + context 'remove' do + let(:newrev) { blank_sha } + + # TODO: this case should pass, but we only take account of added files + it { clears_cache } + end + end + + context 'on ordinary branch' do + let(:branch) { 'fix' } + + context 'create' do + let(:oldrev) { blank_sha } + + it { clears_cache } + end + + context 'update' do + it { clears_cache } + end + + context 'remove' do + let(:newrev) { blank_sha } + + it { clears_cache } + end + end + end + + describe 'GPG signatures' do + context 'when the commit has a signature' do + context 'when the signature is already cached' do + before do + create(:gpg_signature, commit_sha: commit.id) + end + + it 'does not queue a CreateGpgSignatureWorker' do + expect(CreateGpgSignatureWorker).not_to receive(:perform_async) + + service.execute + end + end + + context 'when the signature is not yet cached' do + it 'queues a CreateGpgSignatureWorker' do + expect(CreateGpgSignatureWorker).to receive(:perform_async).with([commit.id], project.id) + + service.execute + end + + it 'can queue several commits to create the gpg signature' do + allow(Gitlab::Git::Commit) + .to receive(:shas_with_signatures) + .and_return([sample_commit.id, another_sample_commit.id]) + + expect(CreateGpgSignatureWorker) + .to receive(:perform_async) + .with([sample_commit.id, another_sample_commit.id], project.id) + + service.execute + end + end + end + + context 'when the commit does not have a signature' do + before do + allow(Gitlab::Git::Commit) + .to receive(:shas_with_signatures) + .with(project.repository, [sample_commit.id]) + .and_return([]) + end + + it 'does not queue a CreateGpgSignatureWorker' do + expect(CreateGpgSignatureWorker) + .not_to receive(:perform_async) + .with(sample_commit.id, project.id) + + service.execute + end + end + end + + describe 'Processing commit messages' do + # Create 4 commits, 2 of which have references. Limiting to 2 commits, we + # expect to see one commit message processor enqueued. + let(:commit_ids) do + Array.new(4) do |i| + message = "Issue #{'#' if i.even?}#{i}" + project.repository.update_file( + user, 'README.md', '', message: message, branch_name: branch + ) + end + end + + let(:oldrev) { commit_ids.first } + let(:newrev) { commit_ids.last } + + before do + stub_const("::Git::BaseHooksService::PROCESS_COMMIT_LIMIT", 2) + end + + context 'creating the default branch' do + let(:oldrev) { Gitlab::Git::BLANK_SHA } + + it 'does not process commit messages' do + expect(ProcessCommitWorker).not_to receive(:perform_async) + + service.execute + end + end + + context 'updating the default branch' do + it 'processes a limited number of commit messages' do + expect(ProcessCommitWorker).to receive(:perform_async).once + + service.execute + end + end + + context 'removing the default branch' do + let(:newrev) { Gitlab::Git::BLANK_SHA } + + it 'does not process commit messages' do + expect(ProcessCommitWorker).not_to receive(:perform_async) + + service.execute + end + end + + context 'creating a normal branch' do + let(:branch) { 'fix' } + let(:oldrev) { Gitlab::Git::BLANK_SHA } + + it 'processes a limited number of commit messages' do + expect(ProcessCommitWorker).to receive(:perform_async).once + + service.execute + end + end + + context 'updating a normal branch' do + let(:branch) { 'fix' } + + it 'processes a limited number of commit messages' do + expect(ProcessCommitWorker).to receive(:perform_async).once + + service.execute + end + end + + context 'removing a normal branch' do + let(:branch) { 'fix' } + let(:newrev) { Gitlab::Git::BLANK_SHA } + + it 'does not process commit messages' do + expect(ProcessCommitWorker).not_to receive(:perform_async) + + service.execute + end + end + end +end diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index d0e2169b4a6..322e40a8112 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -8,7 +8,8 @@ describe Git::BranchPushService, services: true do let(:blankrev) { Gitlab::Git::BLANK_SHA } let(:oldrev) { sample_commit.parent_id } let(:newrev) { sample_commit.id } - let(:ref) { 'refs/heads/master' } + let(:branch) { 'master' } + let(:ref) { "refs/heads/#{branch}" } before do project.add_maintainer(user) @@ -132,64 +133,6 @@ describe Git::BranchPushService, services: true do end end - describe "Git Push Data" do - let(:commit) { project.commit(newrev) } - - subject { push_data_from_service(project, user, oldrev, newrev, ref) } - - it { is_expected.to include(object_kind: 'push') } - it { is_expected.to include(before: oldrev) } - it { is_expected.to include(after: newrev) } - it { is_expected.to include(ref: ref) } - it { is_expected.to include(user_id: user.id) } - it { is_expected.to include(user_name: user.name) } - it { is_expected.to include(project_id: project.id) } - - context "with repository data" do - subject { push_data_from_service(project, user, oldrev, newrev, ref)[:repository] } - - it { is_expected.to include(name: project.name) } - it { is_expected.to include(url: project.url_to_repo) } - it { is_expected.to include(description: project.description) } - it { is_expected.to include(homepage: project.web_url) } - end - - context "with commits" do - subject { push_data_from_service(project, user, oldrev, newrev, ref)[:commits] } - - it { is_expected.to be_an(Array) } - it 'has 1 element' do - expect(subject.size).to eq(1) - end - - context "the commit" do - subject { push_data_from_service(project, user, oldrev, newrev, ref)[:commits].first } - - it { is_expected.to include(id: commit.id) } - it { is_expected.to include(message: commit.safe_message) } - it { expect(subject[:timestamp].in_time_zone).to eq(commit.date.in_time_zone) } - it do - is_expected.to include( - url: [ - Gitlab.config.gitlab.url, - project.namespace.to_param, - project.to_param, - 'commit', - commit.id - ].join('/') - ) - end - - context "with a author" do - subject { push_data_from_service(project, user, oldrev, newrev, ref)[:commits].first[:author] } - - it { is_expected.to include(name: commit.author_name) } - it { is_expected.to include(email: commit.author_email) } - end - end - end - end - describe "Pipelines" do subject { execute_service(project, user, oldrev, newrev, ref) } @@ -203,59 +146,13 @@ describe Git::BranchPushService, services: true do end end - describe "Push Event" do - context "with an existing branch" do - let!(:push_data) { push_data_from_service(project, user, oldrev, newrev, ref) } - let(:event) { Event.find_by_action(Event::PUSHED) } - - it 'generates a push event with one commit' do - expect(event).to be_an_instance_of(PushEvent) - expect(event.project).to eq(project) - expect(event.action).to eq(Event::PUSHED) - expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) - expect(event.push_event_payload.commit_from).to eq(oldrev) - expect(event.push_event_payload.commit_to).to eq(newrev) - expect(event.push_event_payload.ref).to eq('master') - expect(event.push_event_payload.commit_count).to eq(1) - end - end - - context "with a new branch" do - let!(:new_branch_data) { push_data_from_service(project, user, Gitlab::Git::BLANK_SHA, newrev, ref) } - let(:event) { Event.find_by_action(Event::PUSHED) } - - it 'generates a push event with more than one commit' do - expect(event).to be_an_instance_of(PushEvent) - expect(event.project).to eq(project) - expect(event.action).to eq(Event::PUSHED) - expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) - expect(event.push_event_payload.commit_from).to be_nil - expect(event.push_event_payload.commit_to).to eq(newrev) - expect(event.push_event_payload.ref).to eq('master') - expect(event.push_event_payload.commit_count).to be > 1 - end - end - - context "Updates merge requests" do - it "when pushing a new branch for the first time" do - expect(UpdateMergeRequestsWorker).to receive(:perform_async) - .with(project.id, user.id, blankrev, 'newrev', ref) - execute_service(project, user, blankrev, 'newrev', ref ) - end - end - - describe 'system hooks' do - let!(:push_data) { push_data_from_service(project, user, oldrev, newrev, ref) } - let!(:system_hooks_service) { SystemHooksService.new } + describe "Updates merge requests" do + it "when pushing a new branch for the first time" do + expect(UpdateMergeRequestsWorker) + .to receive(:perform_async) + .with(project.id, user.id, blankrev, 'newrev', ref) - it "sends a system hook after pushing a branch" do - allow(SystemHooksService).to receive(:new).and_return(system_hooks_service) - allow(system_hooks_service).to receive(:execute_hooks) - - execute_service(project, user, oldrev, newrev, ref) - - expect(system_hooks_service).to have_received(:execute_hooks).with(push_data, :push_hooks) - end + execute_service(project, user, blankrev, 'newrev', ref ) end end @@ -700,125 +597,64 @@ describe Git::BranchPushService, services: true do end end - describe '#update_caches' do - let(:service) do - described_class.new(project, - user, - oldrev: oldrev, - newrev: newrev, - ref: ref) - end - - context 'on the default branch' do - before do - allow(service).to receive(:default_branch?).and_return(true) - end - - it 'flushes the caches of any special files that have been changed' do - commit = double(:commit) - diff = double(:diff, new_path: 'README.md') - - expect(commit).to receive(:raw_deltas) - .and_return([diff]) - - service.push_commits = [commit] + describe "CI environments" do + context 'create branch' do + let(:oldrev) { blankrev } - expect(ProjectCacheWorker).to receive(:perform_async) - .with(project.id, %i(readme), %i(commit_count repository_size)) + it 'does nothing' do + expect(::Ci::StopEnvironmentsService).not_to receive(:new) - service.update_caches + execute_service(project, user, oldrev, newrev, ref) end end - context 'on a non-default branch' do - before do - allow(service).to receive(:default_branch?).and_return(false) - end - - it 'does not flush any conditional caches' do - expect(ProjectCacheWorker).to receive(:perform_async) - .with(project.id, [], %i(commit_count repository_size)) - .and_call_original + context 'update branch' do + it 'does nothing' do + expect(::Ci::StopEnvironmentsService).not_to receive(:new) - service.update_caches + execute_service(project, user, oldrev, newrev, ref) end end - end - - describe '#process_commit_messages' do - let(:service) do - described_class.new(project, - user, - oldrev: oldrev, - newrev: newrev, - ref: ref) - end - it 'only schedules a limited number of commits' do - service.push_commits = Array.new(1000, double(:commit, to_hash: {}, matches_cross_reference_regex?: true)) - - expect(ProcessCommitWorker).to receive(:perform_async).exactly(100).times - - service.process_commit_messages - end - - it "skips commits which don't include cross-references" do - service.push_commits = [double(:commit, to_hash: {}, matches_cross_reference_regex?: false)] - - expect(ProcessCommitWorker).not_to receive(:perform_async) - - service.process_commit_messages - end - end - - describe '#update_signatures' do - let(:service) do - described_class.new( - project, - user, - oldrev: oldrev, - newrev: newrev, - ref: 'refs/heads/master' - ) - end + context 'delete branch' do + let(:newrev) { blankrev } - context 'when the commit has a signature' do - context 'when the signature is already cached' do - before do - create(:gpg_signature, commit_sha: sample_commit.id) + it 'stops environments' do + expect_next_instance_of(::Ci::StopEnvironmentsService) do |stop_service| + expect(stop_service.project).to eq(project) + expect(stop_service.current_user).to eq(user) + expect(stop_service).to receive(:execute).with(branch) end - it 'does not queue a CreateGpgSignatureWorker' do - expect(CreateGpgSignatureWorker).not_to receive(:perform_async) - - execute_service(project, user, oldrev, newrev, ref) - end + execute_service(project, user, oldrev, newrev, ref) end + end + end - context 'when the signature is not yet cached' do - it 'queues a CreateGpgSignatureWorker' do - expect(CreateGpgSignatureWorker).to receive(:perform_async).with([sample_commit.id], project.id) + describe 'Hooks' do + context 'run on a branch' do + it 'delegates to Git::BranchHooksService' do + expect_next_instance_of(::Git::BranchHooksService) do |hooks_service| + expect(hooks_service.project).to eq(project) + expect(hooks_service.current_user).to eq(user) + expect(hooks_service.params).to include( + oldrev: oldrev, + newrev: newrev, + ref: ref + ) - execute_service(project, user, oldrev, newrev, ref) + expect(hooks_service).to receive(:execute) end - it 'can queue several commits to create the gpg signature' do - allow(Gitlab::Git::Commit).to receive(:shas_with_signatures).and_return([sample_commit.id, another_sample_commit.id]) - - expect(CreateGpgSignatureWorker).to receive(:perform_async).with([sample_commit.id, another_sample_commit.id], project.id) - - execute_service(project, user, oldrev, newrev, ref) - end + execute_service(project, user, oldrev, newrev, ref) end end - context 'when the commit does not have a signature' do - before do - allow(Gitlab::Git::Commit).to receive(:shas_with_signatures).with(project.repository, [sample_commit.id]).and_return([]) - end + context 'run on a tag' do + let(:ref) { 'refs/tags/v1.1.0' } - it 'does not queue a CreateGpgSignatureWorker' do - expect(CreateGpgSignatureWorker).not_to receive(:perform_async).with(sample_commit.id, project.id) + it 'does nothing' do + expect(::Git::BranchHooksService).not_to receive(:new) execute_service(project, user, oldrev, newrev, ref) end @@ -830,8 +666,4 @@ describe Git::BranchPushService, services: true do service.execute service end - - def push_data_from_service(project, user, oldrev, newrev, ref) - execute_service(project, user, oldrev, newrev, ref).push_data - end end diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb new file mode 100644 index 00000000000..8f91ce3b4c5 --- /dev/null +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -0,0 +1,144 @@ +require 'spec_helper' + +describe Git::TagHooksService, :service do + let(:user) { create(:user) } + let(:project) { create(:project, :repository) } + + let(:oldrev) { Gitlab::Git::BLANK_SHA } + let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0 + let(:ref) { "refs/tags/#{tag_name}" } + let(:tag_name) { 'v1.1.0' } + + let(:tag) { project.repository.find_tag(tag_name) } + let(:commit) { tag.dereferenced_target } + + let(:service) do + described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) + end + + describe 'System hooks' do + it 'Executes system hooks' do + push_data = service.execute + + expect_next_instance_of(SystemHooksService) do |system_hooks_service| + expect(system_hooks_service) + .to receive(:execute_hooks) + .with(push_data, :tag_push_hooks) + end + + service.execute + end + end + + describe "Webhooks" do + it "executes hooks on the project" do + expect(project).to receive(:execute_hooks) + + service.execute + end + end + + describe "Pipelines" do + before do + stub_ci_pipeline_to_return_yaml_file + project.add_developer(user) + end + + it "creates a new pipeline" do + expect { service.execute }.to change { Ci::Pipeline.count } + + expect(Ci::Pipeline.last).to be_push + end + end + + describe 'Push data' do + shared_examples_for 'tag push data expectations' do + subject(:push_data) { service.execute } + it 'has expected push data attributes' do + is_expected.to match a_hash_including( + object_kind: 'tag_push', + ref: ref, + before: oldrev, + after: newrev, + message: tag.message, + user_id: user.id, + user_name: user.name, + project_id: project.id + ) + end + + context "with repository data" do + subject { push_data[:repository] } + + it 'has expected repository attributes' do + is_expected.to match a_hash_including( + name: project.name, + url: project.url_to_repo, + description: project.description, + homepage: project.web_url + ) + end + end + + context "with commits" do + subject { push_data[:commits] } + + it { is_expected.to be_an(Array) } + + it 'has 1 element' do + expect(subject.size).to eq(1) + end + + context "the commit" do + subject { push_data[:commits].first } + + it { is_expected.to include(timestamp: commit.date.xmlschema) } + + it 'has expected commit attributes' do + is_expected.to match a_hash_including( + id: commit.id, + message: commit.safe_message, + url: [ + Gitlab.config.gitlab.url, + project.namespace.to_param, + project.to_param, + 'commit', + commit.id + ].join('/') + ) + end + + context "with an author" do + subject { push_data[:commits].first[:author] } + + it 'has expected author attributes' do + is_expected.to match a_hash_including( + name: commit.author_name, + email: commit.author_email + ) + end + end + end + end + end + + context 'annotated tag' do + include_examples 'tag push data expectations' + end + + context 'lightweight tag' do + let(:tag_name) { 'light-tag' } + let(:newrev) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' } + + before do + # Create the lightweight tag + rugged_repo(project.repository).tags.create(tag_name, newrev) + + # Clear tag list cache + project.repository.expire_tags_cache + end + + include_examples 'tag push data expectations' + end + end +end diff --git a/spec/services/git/tag_push_service_spec.rb b/spec/services/git/tag_push_service_spec.rb index 2d960fc9f08..5e89a912060 100644 --- a/spec/services/git/tag_push_service_spec.rb +++ b/spec/services/git/tag_push_service_spec.rb @@ -31,178 +31,27 @@ describe Git::TagPushService do end end - describe 'System Hooks' do - let!(:push_data) { service.tap(&:execute).push_data } - - it "executes system hooks after pushing a tag" do - expect_next_instance_of(SystemHooksService) do |system_hooks_service| - expect(system_hooks_service) - .to receive(:execute_hooks) - .with(push_data, :tag_push_hooks) - end - - service.execute - end - end - - describe "Pipelines" do - subject { service.execute } - - before do - stub_ci_pipeline_to_return_yaml_file - project.add_developer(user) - end - - it "creates a new pipeline" do - expect { subject }.to change { Ci::Pipeline.count } - expect(Ci::Pipeline.last).to be_push - end - end - - describe "Git Tag Push Data" do - subject { @push_data } - let(:tag) { project.repository.find_tag(tag_name) } - let(:commit) { tag.dereferenced_target } - - context 'annotated tag' do - let(:tag_name) { Gitlab::Git.ref_name(ref) } - - before do - service.execute - @push_data = service.push_data - end - - it { is_expected.to include(object_kind: 'tag_push') } - it { is_expected.to include(ref: ref) } - it { is_expected.to include(before: oldrev) } - it { is_expected.to include(after: newrev) } - it { is_expected.to include(message: tag.message) } - it { is_expected.to include(user_id: user.id) } - it { is_expected.to include(user_name: user.name) } - it { is_expected.to include(project_id: project.id) } - - context "with repository data" do - subject { @push_data[:repository] } - - it { is_expected.to include(name: project.name) } - it { is_expected.to include(url: project.url_to_repo) } - it { is_expected.to include(description: project.description) } - it { is_expected.to include(homepage: project.web_url) } - end - - context "with commits" do - subject { @push_data[:commits] } - - it { is_expected.to be_an(Array) } - it 'has 1 element' do - expect(subject.size).to eq(1) - end - - context "the commit" do - subject { @push_data[:commits].first } - - it { is_expected.to include(id: commit.id) } - it { is_expected.to include(message: commit.safe_message) } - it { is_expected.to include(timestamp: commit.date.xmlschema) } - it do - is_expected.to include( - url: [ - Gitlab.config.gitlab.url, - project.namespace.to_param, - project.to_param, - 'commit', - commit.id - ].join('/') - ) - end - - context "with a author" do - subject { @push_data[:commits].first[:author] } - - it { is_expected.to include(name: commit.author_name) } - it { is_expected.to include(email: commit.author_email) } - end + describe 'Hooks' do + context 'run on a tag' do + it 'delegates to Git::TagHooksService' do + expect_next_instance_of(::Git::TagHooksService) do |hooks_service| + expect(hooks_service.project).to eq(service.project) + expect(hooks_service.current_user).to eq(service.current_user) + expect(hooks_service.params).to eq(service.params) + + expect(hooks_service).to receive(:execute) end - end - end - - context 'lightweight tag' do - let(:tag_name) { 'light-tag' } - let(:newrev) { '5937ac0a7beb003549fc5fd26fc247adbce4a52e' } - let(:ref) { "refs/tags/light-tag" } - - before do - # Create the lightweight tag - rugged_repo(project.repository).tags.create(tag_name, newrev) - - # Clear tag list cache - project.repository.expire_tags_cache service.execute - @push_data = service.push_data - end - - it { is_expected.to include(object_kind: 'tag_push') } - it { is_expected.to include(ref: ref) } - it { is_expected.to include(before: oldrev) } - it { is_expected.to include(after: newrev) } - it { is_expected.to include(message: tag.message) } - it { is_expected.to include(user_id: user.id) } - it { is_expected.to include(user_name: user.name) } - it { is_expected.to include(project_id: project.id) } - - context "with repository data" do - subject { @push_data[:repository] } - - it { is_expected.to include(name: project.name) } - it { is_expected.to include(url: project.url_to_repo) } - it { is_expected.to include(description: project.description) } - it { is_expected.to include(homepage: project.web_url) } - end - - context "with commits" do - subject { @push_data[:commits] } - - it { is_expected.to be_an(Array) } - it 'has 1 element' do - expect(subject.size).to eq(1) - end - - context "the commit" do - subject { @push_data[:commits].first } - - it { is_expected.to include(id: commit.id) } - it { is_expected.to include(message: commit.safe_message) } - it { is_expected.to include(timestamp: commit.date.xmlschema) } - it do - is_expected.to include( - url: [ - Gitlab.config.gitlab.url, - project.namespace.to_param, - project.to_param, - 'commit', - commit.id - ].join('/') - ) - end - - context "with a author" do - subject { @push_data[:commits].first[:author] } - - it { is_expected.to include(name: commit.author_name) } - it { is_expected.to include(email: commit.author_email) } - end - end end end - end - describe "Webhooks" do - context "execute webhooks" do - let(:service) { described_class.new(project, user, oldrev: 'oldrev', newrev: 'newrev', ref: 'refs/tags/v1.0.0') } + context 'run on a branch' do + let(:ref) { 'refs/heads/master' } + + it 'does nothing' do + expect(::Git::BranchHooksService).not_to receive(:new) - it "when pushing tags" do - expect(project).to receive(:execute_hooks) service.execute end end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 66958a4c116..a3fe8fa4501 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -63,8 +63,12 @@ describe PostReceive do let(:changes) { "123456 789012 refs/heads/tést" } it "calls Git::BranchPushService" do - expect_any_instance_of(Git::BranchPushService).to receive(:execute).and_return(true) - expect_any_instance_of(Git::TagPushService).not_to receive(:execute) + expect_next_instance_of(Git::BranchPushService) do |service| + expect(service).to receive(:execute).and_return(true) + end + + expect(Git::TagPushService).not_to receive(:new) + described_class.new.perform(gl_repository, key_id, base64_changes) end end @@ -73,8 +77,12 @@ describe PostReceive do let(:changes) { "123456 789012 refs/tags/tag" } it "calls Git::TagPushService" do - expect_any_instance_of(Git::BranchPushService).not_to receive(:execute) - expect_any_instance_of(Git::TagPushService).to receive(:execute).and_return(true) + expect(Git::BranchPushService).not_to receive(:execute) + + expect_next_instance_of(Git::TagPushService) do |service| + expect(service).to receive(:execute).and_return(true) + end + described_class.new.perform(gl_repository, key_id, base64_changes) end end @@ -83,8 +91,9 @@ describe PostReceive do let(:changes) { "123456 789012 refs/merge-requests/123" } it "does not call any of the services" do - expect_any_instance_of(Git::BranchPushService).not_to receive(:execute) - expect_any_instance_of(Git::TagPushService).not_to receive(:execute) + expect(Git::BranchPushService).not_to receive(:new) + expect(Git::TagPushService).not_to receive(:new) + described_class.new.perform(gl_repository, key_id, base64_changes) end end @@ -127,7 +136,9 @@ describe PostReceive do allow_any_instance_of(Gitlab::DataBuilder::Repository).to receive(:update).and_return(fake_hook_data) # silence hooks so we can isolate allow_any_instance_of(Key).to receive(:post_create_hook).and_return(true) - allow_any_instance_of(Git::BranchPushService).to receive(:execute).and_return(true) + expect_next_instance_of(Git::BranchPushService) do |service| + expect(service).to receive(:execute).and_return(true) + end end it 'calls SystemHooksService' do -- cgit v1.2.1 From e82ed3f46bcc5442668e64b7080f7bb4425956c0 Mon Sep 17 00:00:00 2001 From: charlieablett Date: Mon, 8 Apr 2019 16:52:40 +1200 Subject: Abstract out method from spec to support --- spec/features/boards/sidebar_spec.rb | 1 + spec/support/helpers/filtered_search_helpers.rb | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/spec/features/boards/sidebar_spec.rb b/spec/features/boards/sidebar_spec.rb index dfdb8d589eb..d036699f226 100644 --- a/spec/features/boards/sidebar_spec.rb +++ b/spec/features/boards/sidebar_spec.rb @@ -129,6 +129,7 @@ describe 'Issue Boards', :js do click_link 'Unassigned' end + close_dropdown_menu_if_visible wait_for_requests expect(page).to have_content('No assignee') diff --git a/spec/support/helpers/filtered_search_helpers.rb b/spec/support/helpers/filtered_search_helpers.rb index 6569feec39b..03057a102c5 100644 --- a/spec/support/helpers/filtered_search_helpers.rb +++ b/spec/support/helpers/filtered_search_helpers.rb @@ -149,4 +149,10 @@ module FilteredSearchHelpers loop until find('.filtered-search').value.strip == text end end + + def close_dropdown_menu_if_visible + find('.dropdown-menu-toggle', visible: :all).tap do |toggle| + toggle.click if toggle.visible? + end + end end -- cgit v1.2.1 From e0efa97c7f24bc5816ecdcb6a633150e7a6a43f6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 3 Apr 2019 18:33:52 +0700 Subject: Prevent triggering pipelines when target branch is updated Currently, pipelines for merge requests are triggered when source or target branch is updated. However, we should create only when source branch is updated, because it runs unexpected pipelines. --- app/services/merge_requests/refresh_service.rb | 10 +++++-- ...nt-running-mr-pipelines-when-target-updated.yml | 5 ++++ .../merge_requests/refresh_service_spec.rb | 34 +++++++++++++++++----- 3 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/prevent-running-mr-pipelines-when-target-updated.yml diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 51d27673787..e0460f7081c 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -21,6 +21,7 @@ module MergeRequests post_merge_manually_merged reload_merge_requests outdate_suggestions + refresh_pipelines_on_merge_requests reset_merge_when_pipeline_succeeds mark_pending_todos_done cache_merge_requests_closing_issues @@ -107,8 +108,6 @@ module MergeRequests end merge_request.mark_as_unchecked - create_pipeline_for(merge_request, current_user) - UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id) end # Upcoming method calls need the refreshed version of @@ -134,6 +133,13 @@ module MergeRequests end end + def refresh_pipelines_on_merge_requests + merge_requests_for_source_branch.each do |merge_request| + create_pipeline_for(merge_request, current_user) + UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id) + end + end + def reset_merge_when_pipeline_succeeds merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds) end diff --git a/changelogs/unreleased/prevent-running-mr-pipelines-when-target-updated.yml b/changelogs/unreleased/prevent-running-mr-pipelines-when-target-updated.yml new file mode 100644 index 00000000000..d003ca55feb --- /dev/null +++ b/changelogs/unreleased/prevent-running-mr-pipelines-when-target-updated.yml @@ -0,0 +1,5 @@ +--- +title: Create pipelines for merge requests only when source branch is updated +merge_request: 26921 +author: +type: fixed diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index bd10523bc94..5ed06df7072 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -146,7 +146,10 @@ describe MergeRequests::RefreshService do stub_ci_pipeline_yaml_file(YAML.dump(config)) end - subject { service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/master') } + subject { service.new(project, @user).execute(@oldrev, @newrev, ref) } + + let(:ref) { 'refs/heads/master' } + let(:project) { @project } context "when .gitlab-ci.yml has merge_requests keywords" do let(:config) do @@ -162,14 +165,17 @@ describe MergeRequests::RefreshService do it 'create detached merge request pipeline with commits' do expect { subject } .to change { @merge_request.merge_request_pipelines.count }.by(1) - .and change { @fork_merge_request.merge_request_pipelines.count }.by(1) .and change { @another_merge_request.merge_request_pipelines.count }.by(0) expect(@merge_request.has_commits?).to be_truthy - expect(@fork_merge_request.has_commits?).to be_truthy expect(@another_merge_request.has_commits?).to be_falsy end + it 'does not create detached merge request pipeline for forked project' do + expect { subject } + .not_to change { @fork_merge_request.merge_request_pipelines.count } + end + it 'create detached merge request pipeline for non-fork merge request' do subject @@ -177,11 +183,25 @@ describe MergeRequests::RefreshService do .to be_detached_merge_request_pipeline end - it 'create legacy detached merge request pipeline for fork merge request' do - subject + context 'when service is hooked by target branch' do + let(:ref) { 'refs/heads/feature' } - expect(@fork_merge_request.merge_request_pipelines.first) - .to be_legacy_detached_merge_request_pipeline + it 'does not create detached merge request pipeline' do + expect { subject } + .not_to change { @merge_request.merge_request_pipelines.count } + end + end + + context 'when service runs on forked project' do + let(:project) { @fork_project } + + it 'creates legacy detached merge request pipeline for fork merge request' do + expect { subject } + .to change { @fork_merge_request.merge_request_pipelines.count }.by(1) + + expect(@fork_merge_request.merge_request_pipelines.first) + .to be_legacy_detached_merge_request_pipeline + end end context 'when ci_use_merge_request_ref feature flag is false' do -- cgit v1.2.1 From e31da57ea2dbe346d8e4cfb53d15a115efa89b4b Mon Sep 17 00:00:00 2001 From: Lukas 'Eipi' Eipert Date: Mon, 8 Apr 2019 06:24:28 +0000 Subject: Update dependency @gitlab/svgs to ^1.59.0 --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 6d3e6764a46..c88e4ed6a1f 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "@babel/plugin-syntax-import-meta": "^7.2.0", "@babel/preset-env": "^7.3.1", "@gitlab/csslab": "^1.9.0", - "@gitlab/svgs": "^1.58.0", + "@gitlab/svgs": "^1.59.0", "@gitlab/ui": "^3.2.0", "apollo-cache-inmemory": "^1.5.1", "apollo-client": "^2.5.1", diff --git a/yarn.lock b/yarn.lock index 571ac952e90..41758181610 100644 --- a/yarn.lock +++ b/yarn.lock @@ -658,10 +658,10 @@ eslint-plugin-promise "^4.1.1" eslint-plugin-vue "^5.0.0" -"@gitlab/svgs@^1.58.0": - version "1.58.0" - resolved "https://registry.yarnpkg.com/@gitlab/svgs/-/svgs-1.58.0.tgz#bb05263ff2eb7ca09a25cd14d0b1a932d2ea9c2f" - integrity sha512-RlWSjjBT4lMIFuNC1ziCO1nws9zqZtxCjhrqK2DxDDTgp2W0At9M/BFkHp8RHyMCrO3g1fHTrLPUgzr5oR3Epg== +"@gitlab/svgs@^1.59.0": + version "1.59.0" + resolved "https://registry.yarnpkg.com/@gitlab/svgs/-/svgs-1.59.0.tgz#affcf9596d736836d37469bb4aea2226ac03e087" + integrity sha512-dokGyyLRRsoBKO70KP1g+ZsDGyTK/RIHWDmvWI6Bx5AxQ3UqAzVXn2OIb3owjJAexyRG1uBmJrriiVVyHznQ4g== "@gitlab/ui@^3.2.0": version "3.2.0" -- cgit v1.2.1 From 4e95aa3de59220d32211f608f66400a1f067d77a Mon Sep 17 00:00:00 2001 From: charlieablett Date: Mon, 8 Apr 2019 19:38:34 +1200 Subject: Add helper reference --- spec/features/boards/sidebar_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/features/boards/sidebar_spec.rb b/spec/features/boards/sidebar_spec.rb index d036699f226..b358c6b9c34 100644 --- a/spec/features/boards/sidebar_spec.rb +++ b/spec/features/boards/sidebar_spec.rb @@ -2,6 +2,7 @@ require 'rails_helper' describe 'Issue Boards', :js do include BoardHelpers + include FilteredSearchHelpers let(:user) { create(:user) } let(:user2) { create(:user) } -- cgit v1.2.1 From c93779accb4a302896485039805a3510de244374 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Thu, 4 Apr 2019 17:31:56 +0200 Subject: Add usefull tips about big repositories --- doc/ci/README.md | 1 + doc/ci/large_repositories/index.md | 235 +++++++++++++++++++++++++++++++++++++ 2 files changed, 236 insertions(+) create mode 100644 doc/ci/large_repositories/index.md diff --git a/doc/ci/README.md b/doc/ci/README.md index 913e9d4d789..06c6b883909 100644 --- a/doc/ci/README.md +++ b/doc/ci/README.md @@ -62,6 +62,7 @@ into more features: | [ChatOps](chatops/README.md) | Trigger CI jobs from chat, with results sent back to the channel. | | [Interactive web terminals](interactive_web_terminal/index.md) | Open an interactive web terminal to debug the running jobs. | | [Review Apps](review_apps/index.md) | Configure GitLab CI/CD to preview code changes in a per-branch basis. | +| [Optimising GitLab for large repositories](large_repositories/index.md) | Useful tips on how to optimise GitLab and GitLab Runner for big repositories. | | [Deploy Boards](https://docs.gitlab.com/ee/user/project/deploy_boards.html) **[PREMIUM]** | Check the current health and status of each CI/CD environment running on Kubernetes. | | [GitLab CI/CD for external repositories](https://docs.gitlab.com/ee/ci/ci_cd_for_external_repos/index.html) **[PREMIUM]** | Get the benefits of GitLab CI/CD combined with repositories in GitHub and BitBucket Cloud. | diff --git a/doc/ci/large_repositories/index.md b/doc/ci/large_repositories/index.md new file mode 100644 index 00000000000..576914e9dc3 --- /dev/null +++ b/doc/ci/large_repositories/index.md @@ -0,0 +1,235 @@ +# Optimising GitLab for large repositories + +Large repositories consisting of more than 50k files in a worktree +often require special consideration because of +the time required to clone and check out. + +GitLab and GitLab Runner handle this scenario well +but require optimised configuration to efficiently perform its +set of operations. + +The general guidelines for handling big repositories are simple. +Each guideline is described in more detail in the sections below: + +- Always fetch incrementally. Do not clone in a way that results in recreating all of the worktree. +- Always use shallow clone to reduce data transfer. Be aware that this puts more burden + on GitLab instance due to higher CPU impact. +- Control the clone directory if you heavily use a fork-based workflow. +- Optimise `git clean` flags to ensure that you remove or keep data that might affect or speed-up your build. + +## Shallow cloning + +> Introduced in GitLab Runner 8.9. + +GitLab and GitLab Runner always perform a full clone by default. +While it means that all changes from GitLab are received, +it often results in receiving extra commit logs. + +Ideally, you should always use `GIT_DEPTH` with a small number +like 10. This will instruct GitLab Runner to perform shallow clones. +Shallow clones makes Git request only the latest set of changes for a given branch, +up to desired number of commits as defined by the `GIT_DEPTH` variable. + +This significantly speeds up fetching of changes from Git repositories, +especially if the repository has a very long backlog consisting of number +of big files as we effectively reduce amount of data transfer. + +The following example makes GitLab Runner shallow clone to fetch only a given branch, +it does not fetch any other branches nor tags. + +```yaml +variables: + GIT_DEPTH: 10 + +test: + script: + - ls -al +``` + +## Git strategy + +> Introduced in GitLab Runner 8.9. + +By default, GitLab is configured to always prefer the `GIT_STRATEGY: fetch` strategy. +The `GIT_STRATEGY: fetch` strategy will re-use existing worktrees if found +on disk. This is different to the `GIT_STRATEGY: clone` strategy +as in case of clones, if a worktree is found, it is removed before clone. + +Usage of `fetch` is preferred because it reduces the amount of data to transfer and +does not really impact the operations that you might do on a repository from CI. + +However, `fetch` does require access to the previous worktree. This works +well when using the `shell` or `docker` executor because these +try to preserve worktrees and try to re-use them by default. + +This does not work today for `kubernetes` executor and has limitations when using +`docker+machine`. `kubernetes` executor today always clones into ephemeral directory. + +GitLab also offers the `GIT_STRATEGY: none` strategy. This disables any `fetch` and `checkout` commands +done by GitLab, requiring you to do them. + +## Git clone path + +> Introduced in GitLab Runner 11.10. + +`GIT_CLONE_PATH` allows you to control where you clone your sources. +This can have implications if you heavily use big repositories with fork workflow. + +Fork workflow from GitLab Runner's perspective is stored as a separate repository +with separate worktree. That means that GitLab Runner cannot optimise the usage +of worktrees and you might have to instruct GitLab Runner to use that. + +In such cases, ideally you want to make the GitLab Runner executor be used only used only +for the given project and not shared across different projects to make this +process more efficient. + +The `GIT_CLONE_PATH` has to be within the `$CI_BUILDS_DIR`. Currently, +it is impossible to pick any path from disk. + +## Git clean flags + +> Introduced in GitLab Runner 11.10. + +`GIT_CLEAN_FLAGS` allows you to control whether or not you require +the `git clean` command to be executed for each CI job. +By default, GitLab ensures that you have your worktree on the given SHA, +and that your repository is clean. + +`GIT_CLEAN_FLAGS` is disabled when set to `none`. On very big repositories, this +might be desired because `git clean` is disk I/O intensive. Controlling that +with `GIT_CLEAN_FLAGS: -ffdx -e .build/`, for example, allows you to control and +disable removal of some directories within the worktree between subsequent runs, +which can speed-up the incremental builds. This has the biggest effect +if you re-use existing machines, and have an existing worktree that you can re-use +for builds. + +For exact parameters accepted by `GIT_CLEAN_FLAGS`, see the documentation +for [git clean](https://git-scm.com/docs/git-clean). The +available parameters are dependent on Git version. + +## Fork-based workflow + +> Introduced in GitLab Runner 11.10. + +Following the guidelines above, lets imagine that we want to: + +- Optimise for a big project (more than 50k files in directory). +- Use forks-based workflow for contributing. +- Reuse existing worktrees. Have preconfigured runners that are pre-cloned with repositories. +- Runner assigned only to project and all forks. + +Lets consider the following two examples, one using `shell` executor and +other using `docker` executor. + +### `shell` executor example + +Lets assume that you have the following [config.toml](https://docs.gitlab.com/runner/configuration/advanced-configuration.html). + +```toml +concurrent = 4 + +[[runners]] + url = "GITLAB_URL" + token = "TOKEN" + executor = "shell" + builds_dir = "/builds" + cache_dir = "/cache" + + [runners.custom_build_dir] + enabled = true +``` + +This `config.toml`: + +- Uses the `shell` executor, +- Specifies a custom `/builds` directory where all clones will be stored. +- Enables the ability to specify `GIT_CLONE_PATH`, +- Runs at most 4 jobs at once. + +### `docker` executor example + +Lets assume that you have the following [config.toml](https://docs.gitlab.com/runner/configuration/advanced-configuration.html). + +```toml +concurrent = 4 + +[[runners]] + url = "GITLAB_URL" + token = "TOKEN" + executor = "docker" + builds_dir = "/builds" + cache_dir = "/cache" + + [runners.docker] + volumes = ["/builds:/builds", "/cache:/cache"] +``` + +This `config.toml`: + +- Uses the `docker` executor, +- Specifies a custom `/builds` directory on disk where all clones will be stored. + We host mount the `/builds` directory to make it reusable between subsequent runs + and be allowed to override the cloning strategy. +- Doesn't enable the ability to specify `GIT_CLONE_PATH` as it is enabled by default. +- Runs at most 4 jobs at once. + +### Our `.gitlab-ci.yml` + +Once we have the executor configured, we need to fine tune our `.gitlab-ci.yml`. + +Our pipeline will be most performant if we use the following `.gitlab-ci.yml`: + +```yaml +variables: + GIT_DEPTH: 10 + GIT_CLONE_PATH: $CI_BUILDS_DIR/$CI_CONCURRENT_ID/$CI_PROJECT_NAME + +build: + script: ls -al +``` + +The above configures a: + +- Shallow clone of 10, to speed up subsequent `git fetch` commands. +- Custom clone path to make it possible to re-use worktrees between parent project and all forks + because we use the same clone path for all forks. + +Why use `$CI_CONCURRENT_ID`? The main reason is to ensure that worktrees used are not conflicting +between projects. The `$CI_CONCURRENT_ID` represents a unique identifier within the given executor, +so as long as we use it to construct the path, it is guaranteed that this directory will not conflict +with other concurrent jobs running. + +### Store custom clone options in `config.toml` + +Ideally, all job-related configuration should be stored in `.gitlab-ci.yml`. +However, sometimes it is desirable to make these schemes part of Runner configuration. + +In the above example of Forks, making this configuration discoverable for users may be preferred, +but this brings administrative overhead as the `.gitlab-ci.yml` needs to be updated for each branch. +In such cases, it might be desirable to keep the `.gitlab-ci.yml` clone path agnostic, but make it +a configuration of Runner. + +We can extend our [config.toml](https://docs.gitlab.com/runner/configuration/advanced-configuration.html) +with the following specification that will be used by Runner if `.gitlab-ci.yml` will not override it: + +```toml +concurrent = 4 + +[[runners]] + url = "GITLAB_URL" + token = "TOKEN" + executor = "docker" + builds_dir = "/builds" + cache_dir = "/cache" + + environment = [ + "GIT_DEPTH=10", + "GIT_CLONE_PATH=$CI_BUILDS_DIR/$CI_CONCURRENT_ID/$CI_PROJECT_NAME" + ] + + [runners.docker] + volumes = ["/builds:/builds", "/cache:/cache"] +``` + +This makes the cloning configuration to be part of given Runner, +and does not require us to update each `.gitlab-ci.yml`. -- cgit v1.2.1 From 8fdfd521b27e6332ee8bd55d5beecea2cbae9ce0 Mon Sep 17 00:00:00 2001 From: Alexandru Croitor Date: Wed, 3 Apr 2019 14:23:58 +0300 Subject: Extract due quick action to shared example --- .../issues/user_uses_quick_actions_spec.rb | 29 +--------------- .../issue/due_quick_action_shared_examples.rb | 40 ++++++++++++++-------- 2 files changed, 26 insertions(+), 43 deletions(-) diff --git a/spec/features/issues/user_uses_quick_actions_spec.rb b/spec/features/issues/user_uses_quick_actions_spec.rb index ea474759547..68af8303c2f 100644 --- a/spec/features/issues/user_uses_quick_actions_spec.rb +++ b/spec/features/issues/user_uses_quick_actions_spec.rb @@ -60,34 +60,7 @@ describe 'Issues > User uses quick actions', :js do it_behaves_like 'remove_due_date quick action' it_behaves_like 'duplicate quick action' it_behaves_like 'create_merge_request quick action' - - describe 'adding a due date from note' do - let(:issue) { create(:issue, project: project) } - - it_behaves_like 'due quick action available and date can be added' - - context 'when the current user cannot update the due date' do - let(:guest) { create(:user) } - before do - project.add_guest(guest) - gitlab_sign_out - sign_in(guest) - visit project_issue_path(project, issue) - end - - it_behaves_like 'due quick action not available' - end - end - - describe 'toggling the WIP prefix from the title from note' do - let(:issue) { create(:issue, project: project) } - - it 'does not recognize the command nor create a note' do - add_note("/wip") - - expect(page).not_to have_content '/wip' - end - end + it_behaves_like 'due quick action' describe 'move the issue to another project' do let(:issue) { create(:issue, project: project) } diff --git a/spec/support/shared_examples/quick_actions/issue/due_quick_action_shared_examples.rb b/spec/support/shared_examples/quick_actions/issue/due_quick_action_shared_examples.rb index db3ecccc339..ae78cd86cd5 100644 --- a/spec/support/shared_examples/quick_actions/issue/due_quick_action_shared_examples.rb +++ b/spec/support/shared_examples/quick_actions/issue/due_quick_action_shared_examples.rb @@ -1,25 +1,35 @@ # frozen_string_literal: true -shared_examples 'due quick action not available' do - it 'does not set the due date' do - add_note('/due 2016-08-28') +shared_examples 'due quick action' do + context 'due quick action available and date can be added' do + it 'sets the due date accordingly' do + add_note('/due 2016-08-28') - expect(page).not_to have_content 'Commands applied' - expect(page).not_to have_content '/due 2016-08-28' - end -end + expect(page).not_to have_content '/due 2016-08-28' + expect(page).to have_content 'Commands applied' + + visit project_issue_path(project, issue) -shared_examples 'due quick action available and date can be added' do - it 'sets the due date accordingly' do - add_note('/due 2016-08-28') + page.within '.due_date' do + expect(page).to have_content 'Aug 28, 2016' + end + end + end - expect(page).not_to have_content '/due 2016-08-28' - expect(page).to have_content 'Commands applied' + context 'due quick action not available' do + let(:guest) { create(:user) } + before do + project.add_guest(guest) + gitlab_sign_out + sign_in(guest) + visit project_issue_path(project, issue) + end - visit project_issue_path(project, issue) + it 'does not set the due date' do + add_note('/due 2016-08-28') - page.within '.due_date' do - expect(page).to have_content 'Aug 28, 2016' + expect(page).not_to have_content 'Commands applied' + expect(page).not_to have_content '/due 2016-08-28' end end end -- cgit v1.2.1 From 575ec3af131e9c101edfc50ee563381b37c8076e Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 8 Apr 2019 10:50:39 +0100 Subject: Add issue links to Danger roulette comments --- danger/roulette/Dangerfile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile index 81ee0397bdc..406dc2c9724 100644 --- a/danger/roulette/Dangerfile +++ b/danger/roulette/Dangerfile @@ -35,7 +35,10 @@ def spin(team, project, category) maintainers = team.select { |member| member.maintainer?(project, category) } # TODO: filter out people who are currently not in the office + # https://gitlab.com/gitlab-org/gitlab-ce/issues/57652 + # # TODO: take CODEOWNERS into account? + # https://gitlab.com/gitlab-org/gitlab-ce/issues/57653 # Make traintainers have triple the chance to be picked as a reviewer reviewer = (reviewers + traintainers + traintainers).sample -- cgit v1.2.1 From 28531ab43666b5fdf37e0a70db3bcbf7d3f92183 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 8 Apr 2019 11:44:57 +0100 Subject: Pick reviewers based on branch name Change reviewer roulette to always pick the same reviewers for the same branch name. We do this by: 1. Making the branch name 'canonical' across CE and EE by stripping a leading 'ce-' or 'ee-' and a trailing '-ce' or '-ee'. If people are following our branch naming guidelines, this should give the same branch name in both repos. 2. Converting the branch name to a stable integer by taking the integer form of its MD5. 3. Passing that integer as a seed to Ruby's `Random` class, which 'may be used to ensure repeatable sequences of pseudo-random numbers between different runs of the program' (from the Ruby documentation). The upshot is that the same branch name (in CE and EE) should always pick the same reviewers, and those should be evenly distributed across the set of possible reviewers due to the use of MD5. --- danger/roulette/Dangerfile | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile index 406dc2c9724..e6820f49ee2 100644 --- a/danger/roulette/Dangerfile +++ b/danger/roulette/Dangerfile @@ -1,5 +1,7 @@ # frozen_string_literal: true +require 'digest/md5' + MESSAGE = < Date: Mon, 8 Apr 2019 12:06:08 +0000 Subject: Document IS_GITLAB_EE environment variable --- doc/development/ee_features.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md index 9452593c510..c5a1d915be6 100644 --- a/doc/development/ee_features.md +++ b/doc/development/ee_features.md @@ -19,6 +19,11 @@ CE specs should remain untouched as much as possible and extra specs should be added for EE. Licensed features can be stubbed using the spec helper `stub_licensed_features` in `EE::LicenseHelpers`. +You can force Webpack to act as CE by either deleting the `ee/` directory or by +setting the [`IS_GITLAB_EE` environment variable](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/config/helpers/is_ee_env.js) +to something that evaluates as `false`. The same works for running tests +(for example `IS_GITLAB_EE=0 yarn jest`). + [ee-as-ce]: https://gitlab.com/gitlab-org/gitlab-ee/issues/2500 ## Separation of EE code -- cgit v1.2.1 From 2040649bbaf5bf5ca01c1d9fc25189f206a0bb50 Mon Sep 17 00:00:00 2001 From: Marcel Amirault Date: Mon, 8 Apr 2019 12:32:38 +0000 Subject: Docs: Fix anchors related to variables doc --- doc/README.md | 2 +- doc/ci/chatops/README.md | 2 +- doc/ci/docker/using_docker_images.md | 3 +-- doc/ci/environments.md | 2 +- .../examples/laravel_with_gitlab_and_envoy/index.md | 2 +- doc/ci/examples/test-scala-application.md | 2 +- doc/ci/ssh_keys/README.md | 4 ++-- doc/ci/triggers/README.md | 2 +- doc/ci/variables/README.md | 19 ++++++------------- doc/ci/variables/predefined_variables.md | 2 +- doc/ci/variables/where_variables_can_be_used.md | 2 +- doc/ci/yaml/README.md | 4 ++-- doc/development/gitaly.md | 2 +- doc/raketasks/backup_restore.md | 2 +- doc/topics/autodevops/index.md | 6 +++--- doc/user/project/clusters/index.md | 2 +- doc/user/project/pipelines/settings.md | 2 +- 17 files changed, 26 insertions(+), 34 deletions(-) diff --git a/doc/README.md b/doc/README.md index aead50ea97e..14a1eeffda0 100644 --- a/doc/README.md +++ b/doc/README.md @@ -288,7 +288,7 @@ The following documentation relates to the DevOps **Configure** stage: | [GitLab ChatOps](ci/chatops/README.md) | Interact with CI/CD jobs through chat services. | | [Installing Applications](user/project/clusters/index.md#installing-applications) | Deploy Helm, Ingress, and Prometheus on Kubernetes. | | [Mattermost slash commands](user/project/integrations/mattermost_slash_commands.md) | Enable and use slash commands from within Mattermost. | -| [Protected variables](ci/variables/README.md#protected-variables) | Restrict variables to protected branches and tags. | +| [Protected variables](ci/variables/README.md#protected-environment-variables) | Restrict variables to protected branches and tags. | | [Serverless](user/project/clusters/serverless/index.md) | Run serverless workloads on Kubernetes. | | [Slack slash commands](user/project/integrations/slack_slash_commands.md) | Enable and use slash commands from within Slack. | diff --git a/doc/ci/chatops/README.md b/doc/ci/chatops/README.md index a06fe6961a7..5ecdf0f8c54 100644 --- a/doc/ci/chatops/README.md +++ b/doc/ci/chatops/README.md @@ -24,7 +24,7 @@ Since ChatOps is built upon GitLab CI/CD, the job has all the same features and * It is strongly recommended to set `only: [chat]` so the job does not run as part of the standard CI pipeline. * If the job is set to `when: manual`, the pipeline will be created however the job will wait to be started. -* It is important to keep in mind that there is very limited support for access control. If the user who triggered the slash command is a developer in the project, the job will run. The job itself can utilize existing [CI/CD variables](../variables/README.html#predefined-environment-variables) like `GITLAB_USER_ID` to perform additional rights validation, however these variables can be [overridden](https://docs.gitlab.com/ce/ci/variables/README.html#priority-of-variables). +* It is important to keep in mind that there is very limited support for access control. If the user who triggered the slash command is a developer in the project, the job will run. The job itself can utilize existing [CI/CD variables](../variables/README.html#predefined-environment-variables) like `GITLAB_USER_ID` to perform additional rights validation, however these variables can be [overridden](../variables/README.html#priority-of-environment-variables). ### Controlling the ChatOps reply diff --git a/doc/ci/docker/using_docker_images.md b/doc/ci/docker/using_docker_images.md index 3314c76d234..13c26bc5f47 100644 --- a/doc/ci/docker/using_docker_images.md +++ b/doc/ci/docker/using_docker_images.md @@ -500,7 +500,7 @@ To configure access for `registry.example.com:5000`, follow these steps: bXlfdXNlcm5hbWU6bXlfcGFzc3dvcmQ= ``` -1. Create a [variable] `DOCKER_AUTH_CONFIG` with the content of the +1. Create a [variable](../variables/README.md#gitlab-cicd-environment-variables) `DOCKER_AUTH_CONFIG` with the content of the Docker configuration file as the value: ```json @@ -642,7 +642,6 @@ creation. [postgres-hub]: https://hub.docker.com/r/_/postgres/ [mysql-hub]: https://hub.docker.com/r/_/mysql/ [runner-priv-reg]: http://docs.gitlab.com/runner/configuration/advanced-configuration.html#using-a-private-container-registry -[variable]: ../variables/README.md#variables [entrypoint]: https://docs.docker.com/engine/reference/builder/#entrypoint [cmd]: https://docs.docker.com/engine/reference/builder/#cmd [register]: https://docs.gitlab.com/runner/register/ diff --git a/doc/ci/environments.md b/doc/ci/environments.md index eaafc7bc1c0..3e52cc786dd 100644 --- a/doc/ci/environments.md +++ b/doc/ci/environments.md @@ -224,7 +224,7 @@ The `name` and `url` parameters for dynamic environments can use most available including: - [Predefined environment variables](variables/README.md#predefined-environment-variables) -- [Project and group variables](variables/README.md#variables) +- [Project and group variables](variables/README.md#gitlab-cicd-environment-variables) - [`.gitlab-ci.yml` variables](yaml/README.md#variables) However, you cannot use variables defined: diff --git a/doc/ci/examples/laravel_with_gitlab_and_envoy/index.md b/doc/ci/examples/laravel_with_gitlab_and_envoy/index.md index 0f809ed05ca..f56d5429fb7 100644 --- a/doc/ci/examples/laravel_with_gitlab_and_envoy/index.md +++ b/doc/ci/examples/laravel_with_gitlab_and_envoy/index.md @@ -116,7 +116,7 @@ cat ~/.ssh/id_rsa.pub >> ~/.ssh/authorized_keys cat ~/.ssh/id_rsa ``` -Now, let's add it to your GitLab project as a [variable](../../variables/README.md#variables). +Now, let's add it to your GitLab project as a [variable](../../variables/README.md#gitlab-cicd-environment-variables). Variables are user-defined variables and are stored out of `.gitlab-ci.yml`, for security purposes. They can be added per project by navigating to the project's **Settings** > **CI/CD**. diff --git a/doc/ci/examples/test-scala-application.md b/doc/ci/examples/test-scala-application.md index fa18cb22aed..e1164b8d03a 100644 --- a/doc/ci/examples/test-scala-application.md +++ b/doc/ci/examples/test-scala-application.md @@ -69,5 +69,5 @@ in the `.gitlab-ci.yml` file with your application's name. ## Heroku API key You can look up your Heroku API key in your -[account](https://dashboard.heroku.com/account). Add a [protected variable](../variables/README.md#protected-variables) with +[account](https://dashboard.heroku.com/account). Add a [protected variable](../variables/README.md#protected-environment-variables) with this value in **Project ➔ Variables** with key `HEROKU_API_KEY`. diff --git a/doc/ci/ssh_keys/README.md b/doc/ci/ssh_keys/README.md index ff63d0bd69f..9ed1ec5aa5c 100644 --- a/doc/ci/ssh_keys/README.md +++ b/doc/ci/ssh_keys/README.md @@ -49,7 +49,7 @@ to access it. This is where an SSH key pair comes in handy. **Do not** add a passphrase to the SSH key, or the `before_script` will prompt for it. -1. Create a new [variable](../variables/README.md#variables). +1. Create a new [variable](../variables/README.md#gitlab-cicd-environment-variables). As **Key** enter the name `SSH_PRIVATE_KEY` and in the **Value** field paste the content of your _private_ key that you created earlier. @@ -157,7 +157,7 @@ ssh-keyscan example.com ssh-keyscan 1.2.3.4 ``` -Create a new [variable](../variables/README.md#variables) with +Create a new [variable](../variables/README.md#gitlab-cicd-environment-variables) with `SSH_KNOWN_HOSTS` as "Key", and as a "Value" add the output of `ssh-keyscan`. NOTE: **Note:** diff --git a/doc/ci/triggers/README.md b/doc/ci/triggers/README.md index 398b017277f..0e72bdddee7 100644 --- a/doc/ci/triggers/README.md +++ b/doc/ci/triggers/README.md @@ -20,7 +20,7 @@ A unique trigger token can be obtained when [adding a new trigger](#adding-a-new DANGER: **Danger:** Passing plain text tokens in public projects is a security issue. Potential attackers can impersonate the user that exposed their trigger token publicly in -their `.gitlab-ci.yml` file. Use [variables](../variables/README.md#variables) +their `.gitlab-ci.yml` file. Use [variables](../variables/README.md#gitlab-cicd-environment-variables) to protect trigger tokens. ## Adding a new trigger diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index 592fdfd2873..61d1a904f76 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -3,7 +3,6 @@ table_display_block: true --- # GitLab CI/CD environment variables -{: #variables} After a brief overview over the use of environment variables, this document teaches you how to use GitLab CI/CD's @@ -65,7 +64,7 @@ To get started, choose one of the existing to be output by the Runner. For example, let's say that you want a given job you're running through your script to output the stage that job is running for. In your `.gitlab-ci.yml` file, -call the variable from your script according to the [syntaxes](#syntax-of-variables-in-job-scripts) available. To +call the variable from your script according to the [syntaxes](#syntax-of-environment-variables-in-job-scripts) available. To output the job stage, use the predefined variable `CI_JOB_STAGE`: ```yaml @@ -145,7 +144,6 @@ settings](../../user/project/pipelines/settings.md#visibility-of-pipelines). Follow the discussion in issue [#13784][ce-13784] for masking the variables. ### Syntax of environment variables in job scripts -{: #syntax-of-variables-in-job-scripts} All variables are set as environment variables in the build environment, and they are accessible with normal methods that are used to access such variables. @@ -278,7 +276,6 @@ script: ``` ### Group-level environment variables -{: #group-level-variables} > Introduced in GitLab 9.4. @@ -297,19 +294,18 @@ Any variables of [subgroups](../../user/group/subgroups/index.md) will be inheri Once you set them, they will be available for all subsequent pipelines. ## Priority of environment variables -{: #priority-of-variables} Variables of different types can take precedence over other variables, depending on where they are defined. The order of precedence for variables is (from highest to lowest): -1. [Trigger variables](../triggers/README.md#making-use-of-trigger-variables) or [scheduled pipeline variables](../../user/project/pipelines/schedules.md#making-use-of-scheduled-pipeline-variables). -1. Project-level [variables](#creating-a-custom-environment-variable) or [protected variables](#protected-variables). -1. Group-level [variables](#group-level-variables) or [protected variables](#protected-variables). +1. [Trigger variables](../triggers/README.md#making-use-of-trigger-variables) or [scheduled pipeline variables](../../user/project/pipelines/schedules.md#using-variables). +1. Project-level [variables](#creating-a-custom-environment-variable) or [protected variables](#protected-environment-variables). +1. Group-level [variables](#group-level-environment-variables) or [protected variables](#protected-environment-variables). 1. YAML-defined [job-level variables](../yaml/README.md#variables). 1. YAML-defined [global variables](../yaml/README.md#variables). -1. [Deployment variables](#deployment-variables). +1. [Deployment variables](#deployment-environment-variables). 1. [Predefined environment variables](predefined_variables.md). For example, if you define: @@ -329,7 +325,6 @@ about which variables are [not supported](where_variables_can_be_used.md). ## Advanced use ### Protected environment variables -{: #protected-variables} > Introduced in GitLab 9.3. @@ -345,7 +340,6 @@ Protected variables can be added by going to your project's Once you set them, they will be available for all subsequent pipelines. ### Deployment environment variables -{: #deployment-variables} > Introduced in GitLab 8.15. @@ -382,7 +376,7 @@ limitations with the current Auto DevOps scripting environment. [Manually triggered pipelines](../pipelines.md#manually-executing-pipelines) allow you to override the value of a current variable. For instance, suppose you added a -[custom variable `$TEST`](#creating-a-custom-variable) +[custom variable `$TEST`](#creating-a-custom-environment-variable) as exemplified above and you want to override it in a manual pipeline. Navigate to your project's **CI/CD > Pipelines** and click **Run pipeline**. Choose the branch you want to run the pipeline for, then add a new variable @@ -396,7 +390,6 @@ value you set for this specific pipeline: ![Manually overridden variable output](img/override_value_via_manual_pipeline_output.png) ## Environment variables expressions -{: #variables-expressions} > Introduced in GitLab 10.7. diff --git a/doc/ci/variables/predefined_variables.md b/doc/ci/variables/predefined_variables.md index b429dc8c8be..00884610e07 100644 --- a/doc/ci/variables/predefined_variables.md +++ b/doc/ci/variables/predefined_variables.md @@ -36,7 +36,7 @@ future GitLab releases.** | `CI_COMMIT_TAG` | 9.0 | 0.5 | The commit tag name. Present only when building tags. | | `CI_COMMIT_TITLE` | 10.8 | all | The title of the commit - the full first line of the message | | `CI_CONFIG_PATH` | 9.4 | 0.5 | The path to CI config file. Defaults to `.gitlab-ci.yml` | -| `CI_DEBUG_TRACE` | all | 1.7 | Whether [debug tracing](#debug-tracing) is enabled | +| `CI_DEBUG_TRACE` | all | 1.7 | Whether [debug tracing](../README.md#debug-tracing) is enabled | | `CI_DEPLOY_PASSWORD` | 10.8 | all | Authentication password of the [GitLab Deploy Token][gitlab-deploy-token], only present if the Project has one related.| | `CI_DEPLOY_USER` | 10.8 | all | Authentication username of the [GitLab Deploy Token][gitlab-deploy-token], only present if the Project has one related.| | `CI_DISPOSABLE_ENVIRONMENT` | all | 10.1 | Marks that the job is executed in a disposable environment (something that is created only for this job and disposed of/destroyed after the execution - all executors except `shell` and `ssh`). If the environment is disposable, it is set to true, otherwise it is not defined at all. | diff --git a/doc/ci/variables/where_variables_can_be_used.md b/doc/ci/variables/where_variables_can_be_used.md index 1218ac0b071..091ddcb0bae 100644 --- a/doc/ci/variables/where_variables_can_be_used.md +++ b/doc/ci/variables/where_variables_can_be_used.md @@ -111,4 +111,4 @@ They are: - Script execution shell. - Not supported: - For definitions where the ["Expansion place"](#gitlab-ciyml-file) is GitLab. - - In the `only` and `except` [variables expressions](README.md#variables-expressions). + - In the `only` and `except` [variables expressions](README.md#environment-variables-expressions). diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 1d76f911943..5e44de13b51 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -518,7 +518,7 @@ end-to-end: - $CI_COMMIT_MESSAGE =~ /skip-end-to-end-tests/ ``` -Learn more about [variables expressions](../variables/README.md#variables-expressions). +Learn more about [variables expressions](../variables/README.md#environment-variables-expressions). #### `only:changes`/`except:changes` @@ -2249,7 +2249,7 @@ Runner itself](../variables/README.md#predefined-environment-variables). One example would be `CI_COMMIT_REF_NAME` which has the value of the branch or tag name for which project is built. Apart from the variables you can set in `.gitlab-ci.yml`, there are also the so called -[Variables](../variables/README.md#variables) +[Variables](../variables/README.md#gitlab-cicd-environment-variables) which can be set in GitLab's UI. Learn more about [variables and their priority][variables]. diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md index 27b69ba8278..f8cf2a7fdc6 100644 --- a/doc/development/gitaly.md +++ b/doc/development/gitaly.md @@ -192,7 +192,7 @@ GITALY_REPO_URL=https://gitlab+deploy-token-1000:token-here@gitlab.com/nick.thom To use a custom Gitaly repository in CI, for instance if you want your GitLab fork to always use your own Gitaly fork, set `GITALY_REPO_URL` -as a [CI environment variable](../ci/variables/README.md#variables). +as a [CI environment variable](../ci/variables/README.md#gitlab-cicd-environment-variables). --- diff --git a/doc/raketasks/backup_restore.md b/doc/raketasks/backup_restore.md index 574ba961cb0..32c2e4a5c99 100644 --- a/doc/raketasks/backup_restore.md +++ b/doc/raketasks/backup_restore.md @@ -632,7 +632,7 @@ directory (repositories, uploads). To restore a backup, you will also need to restore `/etc/gitlab/gitlab-secrets.json` (for Omnibus packages) or `/home/git/gitlab/.secret` (for installations from source). This file contains the database encryption key, -[CI/CD variables](../ci/variables/README.md#variables), and +[CI/CD variables](../ci/variables/README.md#gitlab-cicd-environment-variables), and variables used for [two-factor authentication](../user/profile/account/two_factor_authentication.md). If you fail to restore this encryption key file along with the application data backup, users with two-factor authentication enabled and GitLab Runners will diff --git a/doc/topics/autodevops/index.md b/doc/topics/autodevops/index.md index 9060360e6a2..1df492ba82c 100644 --- a/doc/topics/autodevops/index.md +++ b/doc/topics/autodevops/index.md @@ -141,7 +141,7 @@ in any of the following places: NOTE: **Note** The Auto DevOps base domain variable (`KUBE_INGRESS_BASE_DOMAIN`) follows the same order of precedence -as other environment [variables](../../ci/variables/README.md#priority-of-variables). +as other environment [variables](../../ci/variables/README.md#priority-of-environment-variables). A wildcard DNS A record matching the base domain(s) is required, for example, given a base domain of `example.com`, you'd need a DNS entry like: @@ -669,7 +669,7 @@ repo or by specifying a project variable: file in it, Auto DevOps will detect the chart and use it instead of the [default one](https://gitlab.com/charts/auto-deploy-app). This can be a great way to control exactly how your application is deployed. -- **Project variable** - Create a [project variable](../../ci/variables/README.md#variables) +- **Project variable** - Create a [project variable](../../ci/variables/README.md#gitlab-cicd-environment-variables) `AUTO_DEVOPS_CHART` with the URL of a custom chart to use or create two project variables `AUTO_DEVOPS_CHART_REPOSITORY` with the URL of a custom chart repository and `AUTO_DEVOPS_CHART` with the path to the chart. ### Custom Helm chart per environment **[PREMIUM]** @@ -770,7 +770,7 @@ also be customized, and you can easily use a [custom buildpack](#custom-buildpac TIP: **Tip:** Set up the replica variables using a -[project variable](../../ci/variables/README.md#variables) +[project variable](../../ci/variables/README.md#gitlab-cicd-environment-variables) and scale your application by just redeploying it! CAUTION: **Caution:** diff --git a/doc/user/project/clusters/index.md b/doc/user/project/clusters/index.md index 878d30dddaa..44af9251107 100644 --- a/doc/user/project/clusters/index.md +++ b/doc/user/project/clusters/index.md @@ -545,7 +545,7 @@ The result will then be: ## Deployment variables The Kubernetes cluster integration exposes the following -[deployment variables](../../../ci/variables/README.md#deployment-variables) in the +[deployment variables](../../../ci/variables/README.md#deployment-environment-variables) in the GitLab CI/CD build environment. | Variable | Description | diff --git a/doc/user/project/pipelines/settings.md b/doc/user/project/pipelines/settings.md index 4a989afad4d..8b762307ac4 100644 --- a/doc/user/project/pipelines/settings.md +++ b/doc/user/project/pipelines/settings.md @@ -182,4 +182,4 @@ https://example.gitlab.com///badges//coverage.svg?st ## Environment Variables -[Environment variables](../../../ci/variables/README.html#variables) can be set in an environment to be available to a runner. +[Environment variables](../../../ci/variables/README.html#gitlab-cicd-environment-variables) can be set in an environment to be available to a runner. -- cgit v1.2.1 From e62f9d9b2e362ef7f021f17473ac3e0dc584e221 Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Mon, 8 Apr 2019 14:53:02 +0200 Subject: Remove EE-specific changes from FilteredSearchBoards (cherry picked from commit 250c9b9ecfc35b6b828f6e1a471a803f74daf7f2) --- app/assets/javascripts/boards/filtered_search_boards.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/javascripts/boards/filtered_search_boards.js b/app/assets/javascripts/boards/filtered_search_boards.js index c14d69c5d18..6b54e8baefb 100644 --- a/app/assets/javascripts/boards/filtered_search_boards.js +++ b/app/assets/javascripts/boards/filtered_search_boards.js @@ -1,6 +1,8 @@ +import IssuableFilteredSearchTokenKeys from 'ee_else_ce/filtered_search/issuable_filtered_search_token_keys'; import FilteredSearchContainer from '../filtered_search/container'; import FilteredSearchManager from '../filtered_search/filtered_search_manager'; import boardsStore from './stores/boards_store'; +import { isEE } from '~/lib/utils/common_utils'; export default class FilteredSearchBoards extends FilteredSearchManager { constructor(store, updateUrl = false, cantEdit = []) { @@ -8,6 +10,8 @@ export default class FilteredSearchBoards extends FilteredSearchManager { page: 'boards', isGroupDecendent: true, stateFiltersSelector: '.issues-state-filters', + isGroup: isEE(), + filteredSearchTokenKeys: IssuableFilteredSearchTokenKeys, }); this.store = store; -- cgit v1.2.1 From f3ad51f8a57df96bcc69b0821355ef29c3df2ac8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 8 Apr 2019 14:41:05 +0200 Subject: Improve performance of PR import This removes unneeded `.reload` call which makes AR to load ALL objects, and create its in-memory representation. --- .../unreleased/fix-pull-request-importer.yml | 5 ++ lib/gitlab/import/merge_request_helpers.rb | 2 +- .../gitlab/import/merge_request_helpers_spec.rb | 73 ++++++++++++++++++++++ 3 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/fix-pull-request-importer.yml create mode 100644 spec/lib/gitlab/import/merge_request_helpers_spec.rb diff --git a/changelogs/unreleased/fix-pull-request-importer.yml b/changelogs/unreleased/fix-pull-request-importer.yml new file mode 100644 index 00000000000..5f642a0710b --- /dev/null +++ b/changelogs/unreleased/fix-pull-request-importer.yml @@ -0,0 +1,5 @@ +--- +title: Improve performance of PR import +merge_request: 27121 +author: +type: performance diff --git a/lib/gitlab/import/merge_request_helpers.rb b/lib/gitlab/import/merge_request_helpers.rb index b3fe1fc0685..4bc39868389 100644 --- a/lib/gitlab/import/merge_request_helpers.rb +++ b/lib/gitlab/import/merge_request_helpers.rb @@ -22,7 +22,7 @@ module Gitlab # additional work that is strictly necessary. merge_request_id = insert_and_return_id(attributes, project.merge_requests) - merge_request = project.merge_requests.reload.find(merge_request_id) + merge_request = project.merge_requests.reset.find(merge_request_id) [merge_request, false] end diff --git a/spec/lib/gitlab/import/merge_request_helpers_spec.rb b/spec/lib/gitlab/import/merge_request_helpers_spec.rb new file mode 100644 index 00000000000..cc0f2baf905 --- /dev/null +++ b/spec/lib/gitlab/import/merge_request_helpers_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Import::MergeRequestHelpers, type: :helper do + set(:project) { create(:project, :repository) } + set(:user) { create(:user) } + + describe '.create_merge_request_without_hooks' do + let(:iid) { 42 } + + let(:attributes) do + { + iid: iid, + title: 'My Pull Request', + description: 'This is my pull request', + source_project_id: project.id, + target_project_id: project.id, + source_branch: 'master-42', + target_branch: 'master', + state: :merged, + author_id: user.id, + assignee_id: user.id + } + end + + subject { helper.create_merge_request_without_hooks(project, attributes, iid) } + + context 'when merge request does not exist' do + it 'returns a new object' do + expect(subject.first).not_to be_nil + expect(subject.second).to eq(false) + end + + it 'does load all existing objects' do + 5.times do |iid| + MergeRequest.create!( + attributes.merge(iid: iid, source_branch: iid.to_s)) + end + + # does ensure that we only load object twice + # 1. by #insert_and_return_id + # 2. by project.merge_requests.find + expect_any_instance_of(MergeRequest).to receive(:attributes) + .twice.times.and_call_original + + expect(subject.first).not_to be_nil + expect(subject.second).to eq(false) + end + end + + context 'when merge request does exist' do + before do + MergeRequest.create!(attributes) + end + + it 'returns an existing object' do + expect(subject.first).not_to be_nil + expect(subject.second).to eq(true) + end + end + + context 'when project is deleted' do + before do + project.delete + end + + it 'returns an existing object' do + expect(subject.first).to be_nil + end + end + end +end -- cgit v1.2.1 From 32cc246f63b7c63f82f32776af1d66d50ad42517 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 8 Apr 2019 15:00:36 +0100 Subject: Fixed EE differences in note_body.vue --- app/assets/javascripts/notes/components/note_body.vue | 9 ++++++--- app/assets/javascripts/notes/mixins/get_discussion.js | 7 +++++++ 2 files changed, 13 insertions(+), 3 deletions(-) create mode 100644 app/assets/javascripts/notes/mixins/get_discussion.js diff --git a/app/assets/javascripts/notes/components/note_body.vue b/app/assets/javascripts/notes/components/note_body.vue index ff303d0f55a..fbf75ed0e41 100644 --- a/app/assets/javascripts/notes/components/note_body.vue +++ b/app/assets/javascripts/notes/components/note_body.vue @@ -1,6 +1,7 @@ -