diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-04-06 15:08:23 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-04-06 15:08:23 +0000 |
commit | 5472bef68de87deeb67594a98e7eb35ff83929ec (patch) | |
tree | 66e0267a3727f69ec8550d2e9b64152687d717b5 | |
parent | 39c98649d20e08428f507e0728b0bd87a299e5cf (diff) | |
download | gitlab-ce-5472bef68de87deeb67594a98e7eb35ff83929ec.tar.gz |
Add latest changes from gitlab-org/gitlab@master
62 files changed, 1154 insertions, 381 deletions
diff --git a/.gitlab/ci/review.gitlab-ci.yml b/.gitlab/ci/review.gitlab-ci.yml index 13f1541f002..c2aa84494fc 100644 --- a/.gitlab/ci/review.gitlab-ci.yml +++ b/.gitlab/ci/review.gitlab-ci.yml @@ -1,3 +1,8 @@ +include: + - project: gitlab-org/modelops/applied-ml/review-recommender/ci-templates + ref: v0.2.1 + file: recommender/Reviewers.gitlab-ci.yml + review-cleanup: extends: - .default-retry @@ -65,3 +70,10 @@ danger-review-local: - .review:rules:danger-local script: - run_timed_command danger_as_local + +reviewers-recommender: + extends: + - .default-retry + - .review:rules:reviewers-recommender + stage: test + needs: [] diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index e808a0297a6..17cb6778a5f 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -1637,6 +1637,10 @@ - <<: *if-merge-request changes: *danger-patterns +.review:rules:reviewers-recommender: + rules: + - <<: *if-merge-request + ############### # Setup rules # ############### diff --git a/app/assets/javascripts/groups/components/group_item.vue b/app/assets/javascripts/groups/components/group_item.vue index 707008ec493..924c8c9db3e 100644 --- a/app/assets/javascripts/groups/components/group_item.vue +++ b/app/assets/javascripts/groups/components/group_item.vue @@ -131,7 +131,7 @@ export default { > <div class="folder-toggle-wrap gl-mr-2 d-flex align-items-center"> <item-caret :is-group-open="group.isOpen" /> - <item-type-icon :item-type="group.type" :is-group-open="group.isOpen" /> + <item-type-icon :item-type="group.type" /> </div> <gl-loading-icon v-if="group.isChildrenLoading" diff --git a/app/assets/javascripts/groups/components/item_stats.vue b/app/assets/javascripts/groups/components/item_stats.vue index 3620c884c5f..00d597a0ccd 100644 --- a/app/assets/javascripts/groups/components/item_stats.vue +++ b/app/assets/javascripts/groups/components/item_stats.vue @@ -55,7 +55,7 @@ export default { :title="__('Subgroups')" :value="item.subgroupCount" css-class="number-subgroups gl-ml-5" - icon-name="folder-o" + icon-name="group" data-testid="subgroups-count" /> <item-stats-value @@ -63,7 +63,7 @@ export default { :title="__('Projects')" :value="item.projectCount" css-class="number-projects gl-ml-5" - icon-name="bookmark" + icon-name="project" data-testid="projects-count" /> <item-stats-value diff --git a/app/assets/javascripts/groups/components/item_type_icon.vue b/app/assets/javascripts/groups/components/item_type_icon.vue index c3787c2df21..f0b363ea455 100644 --- a/app/assets/javascripts/groups/components/item_type_icon.vue +++ b/app/assets/javascripts/groups/components/item_type_icon.vue @@ -11,18 +11,13 @@ export default { type: String, required: true, }, - isGroupOpen: { - type: Boolean, - required: true, - default: false, - }, }, computed: { iconClass() { if (this.itemType === ITEM_TYPE.GROUP) { - return this.isGroupOpen ? 'folder-open' : 'folder-o'; + return 'group'; } - return 'bookmark'; + return 'project'; }, }, }; diff --git a/app/controllers/jira_connect/application_controller.rb b/app/controllers/jira_connect/application_controller.rb index 9b3bff062dd..352e78d6255 100644 --- a/app/controllers/jira_connect/application_controller.rb +++ b/app/controllers/jira_connect/application_controller.rb @@ -22,8 +22,6 @@ class JiraConnect::ApplicationController < ApplicationController def verify_qsh_claim! payload, _ = decode_auth_token! - return if request.format.json? && payload['qsh'] == 'context-qsh' - # Make sure `qsh` claim matches the current request render_403 unless payload['qsh'] == Atlassian::Jwt.create_query_string_hash(request.url, request.method, jira_connect_base_url) rescue StandardError diff --git a/app/controllers/jira_connect/subscriptions_controller.rb b/app/controllers/jira_connect/subscriptions_controller.rb index 77180c12269..ec6ba07a125 100644 --- a/app/controllers/jira_connect/subscriptions_controller.rb +++ b/app/controllers/jira_connect/subscriptions_controller.rb @@ -21,7 +21,7 @@ class JiraConnect::SubscriptionsController < JiraConnect::ApplicationController end before_action :allow_rendering_in_iframe, only: :index - before_action :verify_qsh_claim! + before_action :verify_qsh_claim!, only: :index before_action :authenticate_user!, only: :create def index diff --git a/app/controllers/profiles/chat_names_controller.rb b/app/controllers/profiles/chat_names_controller.rb index 8cfec247b7a..ae757c30d1c 100644 --- a/app/controllers/profiles/chat_names_controller.rb +++ b/app/controllers/profiles/chat_names_controller.rb @@ -4,7 +4,7 @@ class Profiles::ChatNamesController < Profiles::ApplicationController before_action :chat_name_token, only: [:new] before_action :chat_name_params, only: [:new, :create, :deny] - feature_category :users + feature_category :integrations def index @chat_names = current_user.chat_names diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 184db7516eb..6c9044b5089 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -156,6 +156,18 @@ module Ci def process_build(build, params) unless build.pending? @metrics.increment_queue_operation(:build_not_pending) + + if Feature.enabled?(:ci_pending_builds_table_resiliency, default_enabled: :yaml) + ## + # If this build can not be picked because we had stale data in + # `ci_pending_builds` table, we need to respond with 409 to retry + # this operation. + # + if ::Ci::UpdateBuildQueueService.new.remove!(build) + return Result.new(nil, nil, false) + end + end + return end diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index 5a011a8cac6..a525ea179e0 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -37,14 +37,19 @@ module Ci raise InvalidQueueTransition unless transition.from == 'pending' - transition.within_transaction do - removed = build.all_queuing_entries.delete_all + transition.within_transaction { remove!(build) } + end - if removed > 0 - metrics.increment_queue_operation(:build_queue_pop) + ## + # Force recemove build from the queue, without checking a transition state + # + def remove!(build) + removed = build.all_queuing_entries.delete_all - build.id - end + if removed > 0 + metrics.increment_queue_operation(:build_queue_pop) + + build.id end end diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb index 758fa2e67f1..88afea92d26 100644 --- a/app/services/members/create_service.rb +++ b/app/services/members/create_service.rb @@ -14,7 +14,7 @@ module Members super @errors = [] - @invites = invites_from_params&.split(',')&.uniq&.flatten + @invites = invites_from_params @source = params[:source] end @@ -43,7 +43,9 @@ module Members attr_reader :source, :errors, :invites, :member_created_namespace_id, :members def invites_from_params - params[:user_ids] + return params[:user_ids] if params[:user_ids].is_a?(Array) + + params[:user_ids]&.to_s&.split(',')&.uniq&.flatten || [] end def validate_invite_source! @@ -87,6 +89,7 @@ module Members end end + # overridden def add_error_for_member(member) prefix = "#{member.user.username}: " if member.user.present? @@ -117,7 +120,8 @@ module Members def create_tasks_to_be_done return if params[:tasks_to_be_done].blank? || params[:tasks_project_id].blank? - valid_members = members.select { |member| member.valid? && member.member_task.valid? } + # Only create task issues for existing users. Tasks for new users are created when they signup. + valid_members = members.select { |member| member.valid? && member.member_task.valid? && member.user.present? } return unless valid_members.present? # We can take the first `member_task` here, since all tasks will have the same attributes needed diff --git a/app/services/members/invite_service.rb b/app/services/members/invite_service.rb index 85acb720f0f..a5af17d4491 100644 --- a/app/services/members/invite_service.rb +++ b/app/services/members/invite_service.rb @@ -7,6 +7,8 @@ module Members def initialize(*args) super + @invites += parsed_emails + @errors = {} end @@ -14,38 +16,63 @@ module Members alias_method :formatted_errors, :errors - def invites_from_params - params[:email] + def parsed_emails + # can't put this in the initializer since `invites_from_params` is called in super class + # and needs it + @parsed_emails ||= (formatted_param(params[:email]) || []) + end + + def formatted_param(parameter) + parameter&.split(',')&.uniq&.flatten end def validate_invitable! super + return if params[:email].blank? + # we need the below due to add_users hitting Members::CreatorService.parse_users_list and ignoring invalid emails # ideally we wouldn't need this, but we can't really change the add_users method - valid, invalid = invites.partition { |email| Member.valid_email?(email) } - @invites = valid + invalid_emails.each { |email| errors[email] = s_('AddMember|Invite email is invalid') } + end + + def invalid_emails + parsed_emails.each_with_object([]) do |email, invalid| + next if Member.valid_email?(email) - invalid.each { |email| errors[email] = s_('AddMember|Invite email is invalid') } + invalid << email + @invites.delete(email) + end end override :blank_invites_message def blank_invites_message - s_('AddMember|Emails cannot be blank') + s_('AddMember|Invites cannot be blank') end override :add_error_for_member def add_error_for_member(member) - errors[invite_email(member)] = member.errors.full_messages.to_sentence + errors[invited_object(member)] = member.errors.full_messages.to_sentence end - override :create_tasks_to_be_done - def create_tasks_to_be_done - # Only create task issues for existing users. Tasks for new users are created when they signup. - end + def invited_object(member) + return member.invite_email if member.invite_email - def invite_email(member) - member.invite_email || member.user.email + # There is a case where someone was invited by email, but the `user` record exists. + # The member record returned will not have an invite_email attribute defined since + # the CreatorService finds `user` record sometimes by email. + # At that point we lose the info of whether this invite was done by `user` or by email. + # Here we will give preference to check invites by user_id first. + # There is also a case where a user could be invited by their email and + # at the same time via the API in the same request. + # This would would mean the same user is invited as user_id and email. + # However, that isn't as likely from the UI at least since the token generator checks + # for that case and doesn't allow email being used if the user exists as a record already. + if member.user_id.to_s.in?(invites) + member.user.username + else + member.user.all_emails.detect { |email| email.in?(invites) } + end end end end diff --git a/app/services/projects/import_export/export_service.rb b/app/services/projects/import_export/export_service.rb index b91b7f34d42..72492b6f5a5 100644 --- a/app/services/projects/import_export/export_service.rb +++ b/app/services/projects/import_export/export_service.rb @@ -23,6 +23,13 @@ module Projects cleanup end + def exporters + [ + version_saver, avatar_saver, project_tree_saver, uploads_saver, + repo_saver, wiki_repo_saver, lfs_saver, snippets_repo_saver, design_repo_saver + ] + end + protected def extra_attributes_for_measurement @@ -59,30 +66,23 @@ module Projects end def save_export_archive - Gitlab::ImportExport::Saver.save(exportable: project, shared: shared) - end - - def exporters - [ - version_saver, avatar_saver, project_tree_saver, uploads_saver, - repo_saver, wiki_repo_saver, lfs_saver, snippets_repo_saver, design_repo_saver - ] + @export_saver ||= Gitlab::ImportExport::Saver.save(exportable: project, shared: shared) end def version_saver - Gitlab::ImportExport::VersionSaver.new(shared: shared) + @version_saver ||= Gitlab::ImportExport::VersionSaver.new(shared: shared) end def avatar_saver - Gitlab::ImportExport::AvatarSaver.new(project: project, shared: shared) + @avatar_saver ||= Gitlab::ImportExport::AvatarSaver.new(project: project, shared: shared) end def project_tree_saver - tree_saver_class.new(project: project, - current_user: current_user, - shared: shared, - params: params, - logger: logger) + @project_tree_saver ||= tree_saver_class.new(project: project, + current_user: current_user, + shared: shared, + params: params, + logger: logger) end def tree_saver_class @@ -90,27 +90,31 @@ module Projects end def uploads_saver - Gitlab::ImportExport::UploadsSaver.new(project: project, shared: shared) + @uploads_saver ||= Gitlab::ImportExport::UploadsSaver.new(project: project, shared: shared) end def repo_saver - Gitlab::ImportExport::RepoSaver.new(exportable: project, shared: shared) + @repo_saver ||= Gitlab::ImportExport::RepoSaver.new(exportable: project, shared: shared) end def wiki_repo_saver - Gitlab::ImportExport::WikiRepoSaver.new(exportable: project, shared: shared) + @wiki_repo_saver ||= Gitlab::ImportExport::WikiRepoSaver.new(exportable: project, shared: shared) end def lfs_saver - Gitlab::ImportExport::LfsSaver.new(project: project, shared: shared) + @lfs_saver ||= Gitlab::ImportExport::LfsSaver.new(project: project, shared: shared) end def snippets_repo_saver - Gitlab::ImportExport::SnippetsRepoSaver.new(current_user: current_user, project: project, shared: shared) + @snippets_repo_saver ||= Gitlab::ImportExport::SnippetsRepoSaver.new( + current_user: current_user, + project: project, + shared: shared + ) end def design_repo_saver - Gitlab::ImportExport::DesignRepoSaver.new(exportable: project, shared: shared) + @design_repo_saver ||= Gitlab::ImportExport::DesignRepoSaver.new(exportable: project, shared: shared) end def cleanup diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 5e0de3376d7..74d926e182d 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -606,15 +606,6 @@ :weight: 1 :idempotent: :tags: [] -- :name: cronjob:quality_test_data_cleanup - :worker_name: Quality::TestDataCleanupWorker - :feature_category: :quality_management - :has_external_dependencies: - :urgency: :low - :resource_boundary: :unknown - :weight: 1 - :idempotent: true - :tags: [] - :name: cronjob:releases_manage_evidence :worker_name: Releases::ManageEvidenceWorker :feature_category: :release_evidence diff --git a/app/workers/project_export_worker.rb b/app/workers/project_export_worker.rb index 8b87f362285..ee892d43313 100644 --- a/app/workers/project_export_worker.rb +++ b/app/workers/project_export_worker.rb @@ -21,7 +21,10 @@ class ProjectExportWorker # rubocop:disable Scalability/IdempotentWorker export_job&.start - ::Projects::ImportExport::ExportService.new(project, current_user, params).execute(after_export) + export_service = ::Projects::ImportExport::ExportService.new(project, current_user, params) + export_service.execute(after_export) + + log_exporters_duration(export_service) export_job&.finish rescue ActiveRecord::RecordNotFound => e @@ -46,4 +49,13 @@ class ProjectExportWorker # rubocop:disable Scalability/IdempotentWorker def log_failure(project_id, ex) logger.error("Failed to export project #{project_id}: #{ex.message}") end + + def log_exporters_duration(export_service) + export_service.exporters.each do |exporter| + exporter_key = "#{exporter.class.name.demodulize.underscore}_duration_s".to_sym # e.g. uploads_saver_duration_s + exporter_duration = exporter.duration_s&.round(6) + + log_extra_metadata_on_done(exporter_key, exporter_duration) + end + end end diff --git a/app/workers/quality/test_data_cleanup_worker.rb b/app/workers/quality/test_data_cleanup_worker.rb deleted file mode 100644 index 68b36cacbbf..00000000000 --- a/app/workers/quality/test_data_cleanup_worker.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -module Quality - class TestDataCleanupWorker - include ApplicationWorker - - data_consistency :always - feature_category :quality_management - urgency :low - - include CronjobQueue - idempotent! - - KEEP_RECENT_DATA_DAY = 3 - GROUP_PATH_PATTERN = 'test-group-fulfillment' - GROUP_OWNER_EMAIL_PATTERN = %w(test-user- gitlab-qa-user qa-user-).freeze - - # Remove test groups generated in E2E tests on gstg - # rubocop: disable CodeReuse/ActiveRecord - def perform - return unless Gitlab.staging? - - Group.where('path like ?', "#{GROUP_PATH_PATTERN}%").where('created_at < ?', KEEP_RECENT_DATA_DAY.days.ago).each do |group| - next unless GROUP_OWNER_EMAIL_PATTERN.any? { |pattern| group.owners.first.email.include?(pattern) } - - with_context(namespace: group, user: group.owners.first) do - Groups::DestroyService.new(group, group.owners.first).execute - end - end - end - # rubocop: enable CodeReuse/ActiveRecord - end -end diff --git a/config/feature_flags/development/ci_pending_builds_table_resiliency.yml b/config/feature_flags/development/ci_pending_builds_table_resiliency.yml new file mode 100644 index 00000000000..2e53bf2c9a1 --- /dev/null +++ b/config/feature_flags/development/ci_pending_builds_table_resiliency.yml @@ -0,0 +1,8 @@ +--- +name: ci_pending_builds_table_resiliency +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84359 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/357988 +milestone: '14.10' +type: development +group: group::pipeline execution +default_enabled: true diff --git a/config/metrics/counts_28d/20210216181057_projects_with_packages.yml b/config/metrics/counts_28d/20210216181057_projects_with_packages.yml index 3398dacaa92..17749102974 100644 --- a/config/metrics/counts_28d/20210216181057_projects_with_packages.yml +++ b/config/metrics/counts_28d/20210216181057_projects_with_packages.yml @@ -1,5 +1,5 @@ --- -data_category: optional +data_category: operational key_path: usage_activity_by_stage_monthly.package.projects_with_packages description: The total number of projects in a given month with at least one package product_section: ops diff --git a/db/migrate/20220401110443_add_on_hold_until_column_for_batched_migration.rb b/db/migrate/20220401110443_add_on_hold_until_column_for_batched_migration.rb new file mode 100644 index 00000000000..4771b50d566 --- /dev/null +++ b/db/migrate/20220401110443_add_on_hold_until_column_for_batched_migration.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddOnHoldUntilColumnForBatchedMigration < Gitlab::Database::Migration[1.0] + def change + add_column :batched_background_migrations, :on_hold_until, :timestamptz, + comment: 'execution of this migration is on hold until this time' + end +end diff --git a/db/post_migrate/20220315181136_backfill_work_item_type_id_on_issues.rb b/db/post_migrate/20220315181136_backfill_work_item_type_id_on_issues.rb index 22a6ba73aee..8838a27f233 100644 --- a/db/post_migrate/20220315181136_backfill_work_item_type_id_on_issues.rb +++ b/db/post_migrate/20220315181136_backfill_work_item_type_id_on_issues.rb @@ -2,6 +2,7 @@ class BackfillWorkItemTypeIdOnIssues < Gitlab::Database::Migration[1.0] MIGRATION = 'BackfillWorkItemTypeIdForIssues' + BATCH_CLASS_NAME = 'BackfillIssueWorkItemTypeBatchingStrategy' BATCH_SIZE = 10_000 MAX_BATCH_SIZE = 30_000 SUB_BATCH_SIZE = 100 @@ -27,7 +28,8 @@ class BackfillWorkItemTypeIdOnIssues < Gitlab::Database::Migration[1.0] job_interval: INTERVAL, batch_size: BATCH_SIZE, max_batch_size: MAX_BATCH_SIZE, - sub_batch_size: SUB_BATCH_SIZE + sub_batch_size: SUB_BATCH_SIZE, + batch_class_name: BATCH_CLASS_NAME ) end end diff --git a/db/post_migrate/20220404194649_replace_work_item_type_backfill_next_batch_strategy.rb b/db/post_migrate/20220404194649_replace_work_item_type_backfill_next_batch_strategy.rb new file mode 100644 index 00000000000..1f2c0715f04 --- /dev/null +++ b/db/post_migrate/20220404194649_replace_work_item_type_backfill_next_batch_strategy.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class ReplaceWorkItemTypeBackfillNextBatchStrategy < Gitlab::Database::Migration[1.0] + JOB_CLASS_NAME = 'BackfillWorkItemTypeIdForIssues' + NEW_STRATEGY_CLASS = 'BackfillIssueWorkItemTypeBatchingStrategy' + OLD_STRATEGY_CLASS = 'PrimaryKeyBatchingStrategy' + + class InlineBatchedMigration < ApplicationRecord + self.table_name = :batched_background_migrations + end + + def up + InlineBatchedMigration.where(job_class_name: JOB_CLASS_NAME) + .update_all(batch_class_name: NEW_STRATEGY_CLASS) + end + + def down + InlineBatchedMigration.where(job_class_name: JOB_CLASS_NAME) + .update_all(batch_class_name: OLD_STRATEGY_CLASS) + end +end diff --git a/db/schema_migrations/20220401110443 b/db/schema_migrations/20220401110443 new file mode 100644 index 00000000000..30cc8592140 --- /dev/null +++ b/db/schema_migrations/20220401110443 @@ -0,0 +1 @@ +c35e6149ee37321cd446284a2f0405378f3e34e3c9d11b7d128e9afa6c83281d
\ No newline at end of file diff --git a/db/schema_migrations/20220404194649 b/db/schema_migrations/20220404194649 new file mode 100644 index 00000000000..4b6cc920cd6 --- /dev/null +++ b/db/schema_migrations/20220404194649 @@ -0,0 +1 @@ +7897da66c2a941a6a09db6f62fa9069caef235603663077e5dd342a72ac5c7c3
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 6c5acec9cd4..fdd878a1d1b 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -11716,6 +11716,7 @@ CREATE TABLE batched_background_migrations ( pause_ms integer DEFAULT 100 NOT NULL, max_batch_size integer, started_at timestamp with time zone, + on_hold_until timestamp with time zone, CONSTRAINT check_5bb0382d6f CHECK ((char_length(column_name) <= 63)), CONSTRAINT check_6b6a06254a CHECK ((char_length(table_name) <= 63)), CONSTRAINT check_batch_size_in_range CHECK ((batch_size >= sub_batch_size)), @@ -11726,6 +11727,8 @@ CREATE TABLE batched_background_migrations ( CONSTRAINT check_positive_sub_batch_size CHECK ((sub_batch_size > 0)) ); +COMMENT ON COLUMN batched_background_migrations.on_hold_until IS 'execution of this migration is on hold until this time'; + CREATE SEQUENCE batched_background_migrations_id_seq START WITH 1 INCREMENT BY 1 diff --git a/doc/user/clusters/agent/install/index.md b/doc/user/clusters/agent/install/index.md index 8d3c135b8c3..b1bec59984e 100644 --- a/doc/user/clusters/agent/install/index.md +++ b/doc/user/clusters/agent/install/index.md @@ -105,6 +105,10 @@ The one-liner installation is the simplest process, but you need Docker installed locally. If you don't have it, you can either install it or opt to the [advanced installation method](#advanced-installation). +Use the one-liner process for simple use cases or to get started with the agent for Kubernetes. +For production use, opt for the [advanced installation method](#advanced-installation) +as it gives you more customization options and access to all settings. + To install the agent on your cluster using the one-liner installation: 1. In your computer, open a terminal and [connect to your cluster](https://kubernetes.io/docs/tasks/access-application-cluster/access-cluster/). diff --git a/lib/api/namespaces.rb b/lib/api/namespaces.rb index d2468fb1c2e..1f3516e0667 100644 --- a/lib/api/namespaces.rb +++ b/lib/api/namespaces.rb @@ -6,8 +6,6 @@ module API before { authenticate! } - feature_category :subgroups - helpers do params :optional_list_params_ee do # EE::API::Namespaces would override this helper @@ -32,7 +30,7 @@ module API use :pagination use :optional_list_params_ee end - get do + get feature_category: :subgroups do owned_only = params[:owned_only] == true namespaces = current_user.admin ? Namespace.all : current_user.namespaces(owned_only: owned_only) @@ -54,7 +52,7 @@ module API params do requires :id, type: String, desc: "Namespace's ID or path" end - get ':id', requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do + get ':id', requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS, feature_category: :subgroups do user_namespace = find_namespace!(params[:id]) present user_namespace, with: Entities::Namespace, current_user: current_user @@ -67,7 +65,7 @@ module API requires :namespace, type: String, desc: "Namespace's path" optional :parent_id, type: Integer, desc: "The ID of the parent namespace. If no ID is specified, only top-level namespaces are considered." end - get ':namespace/exists', requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do + get ':namespace/exists', requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS, feature_category: :subgroups do namespace_path = params[:namespace] exists = Namespace.without_project_namespaces.by_parent(params[:parent_id]).filter_by_path(namespace_path).exists? diff --git a/lib/api/users.rb b/lib/api/users.rb index 301d6e93c3d..37ef6a95235 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -1094,7 +1094,7 @@ module API requires :credit_card_mask_number, type: String, desc: 'The last 4 digits of credit card number' requires :credit_card_type, type: String, desc: 'The credit card network name' end - put ":user_id/credit_card_validation", feature_category: :users do + put ":user_id/credit_card_validation", feature_category: :purchase do authenticated_as_admin! user = find_user(params[:user_id]) diff --git a/lib/gitlab/background_migration/batching_strategies/backfill_issue_work_item_type_batching_strategy.rb b/lib/gitlab/background_migration/batching_strategies/backfill_issue_work_item_type_batching_strategy.rb new file mode 100644 index 00000000000..06036eebcb9 --- /dev/null +++ b/lib/gitlab/background_migration/batching_strategies/backfill_issue_work_item_type_batching_strategy.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + module BatchingStrategies + # Batching class to use for back-filling issue's work_item_type_id for a single issue type. + # Batches will be scoped to records where the foreign key is NULL and only of a given issue type + # + # If no more batches exist in the table, returns nil. + class BackfillIssueWorkItemTypeBatchingStrategy < PrimaryKeyBatchingStrategy + def apply_additional_filters(relation, job_arguments:) + issue_type = job_arguments.first + + relation.where(issue_type: issue_type) + end + end + end + end +end diff --git a/lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy.rb b/lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy.rb index 405544da223..e7a68b183b8 100644 --- a/lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy.rb +++ b/lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy.rb @@ -23,7 +23,7 @@ module Gitlab quoted_column_name = model_class.connection.quote_column_name(column_name) relation = model_class.where("#{quoted_column_name} >= ?", batch_min_value) - relation = apply_additional_filters(relation) + relation = apply_additional_filters(relation, job_arguments: job_arguments) next_batch_bounds = nil relation.each_batch(of: batch_size, column: column_name) do |batch| # rubocop:disable Lint/UnreachableLoop @@ -40,12 +40,14 @@ module Gitlab # # Example: # - # class TypeIsNotNull < PrimaryKeyBatchingStrategy - # def apply_additional_filters(relation) - # relation.where.not(type: nil) + # class MatchingType < PrimaryKeyBatchingStrategy + # def apply_additional_filters(relation, job_arguments:) + # type = job_arguments.first + # + # relation.where(type: type) # end # end - def apply_additional_filters(relation) + def apply_additional_filters(relation, job_arguments: []) relation end end diff --git a/lib/gitlab/database/background_migration/batched_migration.rb b/lib/gitlab/database/background_migration/batched_migration.rb index e07be8cae32..d94bf060d05 100644 --- a/lib/gitlab/database/background_migration/batched_migration.rb +++ b/lib/gitlab/database/background_migration/batched_migration.rb @@ -24,6 +24,10 @@ module Gitlab scope :queue_order, -> { order(id: :asc) } scope :queued, -> { with_statuses(:active, :paused) } + + # on_hold_until is a temporary runtime status which puts execution "on hold" + scope :executable, -> { with_status(:active).where('on_hold_until IS NULL OR on_hold_until < NOW()') } + scope :for_configuration, ->(job_class_name, table_name, column_name, job_arguments) do where(job_class_name: job_class_name, table_name: table_name, column_name: column_name) .where("job_arguments = ?", job_arguments.to_json) # rubocop:disable Rails/WhereEquals @@ -72,7 +76,7 @@ module Gitlab end def self.active_migration - with_status(:active).queue_order.first + executable.queue_order.first end def self.successful_rows_counts(migrations) @@ -178,6 +182,10 @@ module Gitlab BatchOptimizer.new(self).optimize! end + def hold!(until_time: 10.minutes.from_now) + update!(on_hold_until: until_time) + end + private def validate_batched_jobs_status diff --git a/lib/gitlab/import_export/avatar_saver.rb b/lib/gitlab/import_export/avatar_saver.rb index 7534ab5a9ce..db90886ad11 100644 --- a/lib/gitlab/import_export/avatar_saver.rb +++ b/lib/gitlab/import_export/avatar_saver.rb @@ -3,19 +3,23 @@ module Gitlab module ImportExport class AvatarSaver + include DurationMeasuring + def initialize(project:, shared:) @project = project @shared = shared end def save - return true unless @project.avatar.exists? + with_duration_measuring do + break true unless @project.avatar.exists? - Gitlab::ImportExport::UploadsManager.new( - project: @project, - shared: @shared, - relative_export_path: 'avatar' - ).save + Gitlab::ImportExport::UploadsManager.new( + project: @project, + shared: @shared, + relative_export_path: 'avatar' + ).save + end rescue StandardError => e @shared.error(e) false diff --git a/lib/gitlab/import_export/duration_measuring.rb b/lib/gitlab/import_export/duration_measuring.rb new file mode 100644 index 00000000000..c192be6ae29 --- /dev/null +++ b/lib/gitlab/import_export/duration_measuring.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module ImportExport + module DurationMeasuring + extend ActiveSupport::Concern + + included do + attr_reader :duration_s + + def with_duration_measuring + result = nil + + @duration_s = Benchmark.realtime do + result = yield + end + + result + end + end + end + end +end diff --git a/lib/gitlab/import_export/lfs_saver.rb b/lib/gitlab/import_export/lfs_saver.rb index 47acd49d529..22a7a8dd7cd 100644 --- a/lib/gitlab/import_export/lfs_saver.rb +++ b/lib/gitlab/import_export/lfs_saver.rb @@ -4,6 +4,7 @@ module Gitlab module ImportExport class LfsSaver include Gitlab::ImportExport::CommandLineUtil + include DurationMeasuring attr_accessor :lfs_json, :project, :shared @@ -16,17 +17,19 @@ module Gitlab end def save - project.lfs_objects.find_in_batches(batch_size: BATCH_SIZE) do |batch| - batch.each do |lfs_object| - save_lfs_object(lfs_object) - end + with_duration_measuring do + project.lfs_objects.find_in_batches(batch_size: BATCH_SIZE) do |batch| + batch.each do |lfs_object| + save_lfs_object(lfs_object) + end - append_lfs_json_for_batch(batch) - end + append_lfs_json_for_batch(batch) + end - write_lfs_json + write_lfs_json - true + true + end rescue StandardError => e shared.error(e) diff --git a/lib/gitlab/import_export/project/tree_saver.rb b/lib/gitlab/import_export/project/tree_saver.rb index aafed850afa..63c5afa9595 100644 --- a/lib/gitlab/import_export/project/tree_saver.rb +++ b/lib/gitlab/import_export/project/tree_saver.rb @@ -4,6 +4,8 @@ module Gitlab module ImportExport module Project class TreeSaver + include DurationMeasuring + attr_reader :full_path def initialize(project:, current_user:, shared:, params: {}, logger: Gitlab::Import::Logger) @@ -15,9 +17,11 @@ module Gitlab end def save - stream_export + with_duration_measuring do + stream_export - true + true + end rescue StandardError => e @shared.error(e) false diff --git a/lib/gitlab/import_export/repo_saver.rb b/lib/gitlab/import_export/repo_saver.rb index fae07039139..454e84bbc04 100644 --- a/lib/gitlab/import_export/repo_saver.rb +++ b/lib/gitlab/import_export/repo_saver.rb @@ -4,6 +4,7 @@ module Gitlab module ImportExport class RepoSaver include Gitlab::ImportExport::CommandLineUtil + include DurationMeasuring attr_reader :exportable, :shared @@ -13,9 +14,12 @@ module Gitlab end def save - return true unless repository_exists? # it's ok to have no repo + with_duration_measuring do + # it's ok to have no repo + break true unless repository_exists? - bundle_to_disk + bundle_to_disk + end end def repository diff --git a/lib/gitlab/import_export/snippets_repo_saver.rb b/lib/gitlab/import_export/snippets_repo_saver.rb index d3b0fe1c18c..ca0d38272e5 100644 --- a/lib/gitlab/import_export/snippets_repo_saver.rb +++ b/lib/gitlab/import_export/snippets_repo_saver.rb @@ -4,6 +4,7 @@ module Gitlab module ImportExport class SnippetsRepoSaver include Gitlab::ImportExport::CommandLineUtil + include DurationMeasuring def initialize(current_user:, project:, shared:) @project = project @@ -12,13 +13,15 @@ module Gitlab end def save - create_snippets_repo_directory + with_duration_measuring do + create_snippets_repo_directory - @project.snippets.find_each.all? do |snippet| - Gitlab::ImportExport::SnippetRepoSaver.new(project: @project, - shared: @shared, - repository: snippet.repository) - .save + @project.snippets.find_each.all? do |snippet| + Gitlab::ImportExport::SnippetRepoSaver.new(project: @project, + shared: @shared, + repository: snippet.repository) + .save + end end end diff --git a/lib/gitlab/import_export/uploads_saver.rb b/lib/gitlab/import_export/uploads_saver.rb index 9f58609fa17..05132fd3edd 100644 --- a/lib/gitlab/import_export/uploads_saver.rb +++ b/lib/gitlab/import_export/uploads_saver.rb @@ -3,16 +3,20 @@ module Gitlab module ImportExport class UploadsSaver + include DurationMeasuring + def initialize(project:, shared:) @project = project @shared = shared end def save - Gitlab::ImportExport::UploadsManager.new( - project: @project, - shared: @shared - ).save + with_duration_measuring do + Gitlab::ImportExport::UploadsManager.new( + project: @project, + shared: @shared + ).save + end rescue StandardError => e @shared.error(e) false diff --git a/lib/gitlab/import_export/version_saver.rb b/lib/gitlab/import_export/version_saver.rb index e8f68f93af0..db5040ec0f6 100644 --- a/lib/gitlab/import_export/version_saver.rb +++ b/lib/gitlab/import_export/version_saver.rb @@ -4,17 +4,20 @@ module Gitlab module ImportExport class VersionSaver include Gitlab::ImportExport::CommandLineUtil + include DurationMeasuring def initialize(shared:) @shared = shared end def save - mkdir_p(@shared.export_path) + with_duration_measuring do + mkdir_p(@shared.export_path) - File.write(version_file, Gitlab::ImportExport.version, mode: 'w') - File.write(gitlab_version_file, Gitlab::VERSION, mode: 'w') - File.write(gitlab_revision_file, Gitlab.revision, mode: 'w') + File.write(version_file, Gitlab::ImportExport.version, mode: 'w') + File.write(gitlab_version_file, Gitlab::VERSION, mode: 'w') + File.write(gitlab_revision_file, Gitlab.revision, mode: 'w') + end rescue StandardError => e @shared.error(e) false diff --git a/lib/tasks/dev.rake b/lib/tasks/dev.rake index fe5755e8e71..42b12cd0ae3 100644 --- a/lib/tasks/dev.rake +++ b/lib/tasks/dev.rake @@ -27,6 +27,32 @@ namespace :dev do Rails.application.eager_load! end + # If there are any clients connected to the DB, PostgreSQL won't let + # you drop the database. It's possible that Sidekiq, Puma, or + # some other client will be hanging onto a connection, preventing + # the DROP DATABASE from working. To workaround this problem, this + # method terminates all the connections so that a subsequent DROP + # will work. + desc "Used to drop all connections in development" + task :terminate_all_connections do + # In production, we might want to prevent ourselves from shooting + # ourselves in the foot, so let's only do this in a test or + # development environment. + unless Rails.env.production? + cmd = <<~SQL + SELECT pg_terminate_backend(pg_stat_activity.pid) + FROM pg_stat_activity + WHERE datname = current_database() + AND pid <> pg_backend_pid(); + SQL + + Gitlab::Database::EachDatabase.each_database_connection(include_shared: false) do |connection| + connection.execute(cmd) + rescue ActiveRecord::NoDatabaseError + end + end + end + databases = ActiveRecord::Tasks::DatabaseTasks.setup_initial_database_yaml namespace :copy_db do @@ -37,6 +63,8 @@ namespace :dev do desc "Copies the #{name} database from the main database" task name => :environment do + Rake::Task["dev:terminate_all_connections"].invoke + db_config = ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, name: name) ApplicationRecord.connection.create_database(db_config.database, template: ApplicationRecord.connection_db_config.database) diff --git a/lib/tasks/gitlab/db/validate_config.rake b/lib/tasks/gitlab/db/validate_config.rake index 911d9a3d1b2..5ea5cc364b7 100644 --- a/lib/tasks/gitlab/db/validate_config.rake +++ b/lib/tasks/gitlab/db/validate_config.rake @@ -20,7 +20,9 @@ namespace :gitlab do begin ActiveRecord::Base.establish_connection(db_config) # rubocop: disable Database/EstablishConnection ActiveRecord::Base.connection.select_one("SELECT system_identifier, current_database() FROM pg_control_system()") - rescue ActiveRecord::NoDatabaseError, ActiveRecord::ConnectionNotEstablished, PG::ConnectionBad + rescue ActiveRecord::ConnectionNotEstablished, PG::ConnectionBad => err + warn "WARNING: Could not establish database connection for #{db_config.name}: #{err.message}" + rescue ActiveRecord::NoDatabaseError end { diff --git a/lib/tasks/gitlab/setup.rake b/lib/tasks/gitlab/setup.rake index ca10bccf5bf..006dfad3a95 100644 --- a/lib/tasks/gitlab/setup.rake +++ b/lib/tasks/gitlab/setup.rake @@ -30,7 +30,7 @@ namespace :gitlab do # In production, we might want to prevent ourselves from shooting # ourselves in the foot, so let's only do this in a test or # development environment. - terminate_all_connections unless Rails.env.production? + Rake::Task["dev:terminate_all_connections"].invoke unless Rails.env.production? Rake::Task["db:reset"].invoke Rake::Task["db:seed_fu"].invoke @@ -38,24 +38,4 @@ namespace :gitlab do puts "Quitting...".color(:red) exit 1 end - - # If there are any clients connected to the DB, PostgreSQL won't let - # you drop the database. It's possible that Sidekiq, Puma, or - # some other client will be hanging onto a connection, preventing - # the DROP DATABASE from working. To workaround this problem, this - # method terminates all the connections so that a subsequent DROP - # will work. - def self.terminate_all_connections - cmd = <<~SQL - SELECT pg_terminate_backend(pg_stat_activity.pid) - FROM pg_stat_activity - WHERE datname = current_database() - AND pid <> pg_backend_pid(); - SQL - - Gitlab::Database::EachDatabase.each_database_connection(include_shared: false) do |connection| - connection.execute(cmd) - rescue ActiveRecord::NoDatabaseError - end - end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3c1ada93247..474d3c82e30 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2334,15 +2334,15 @@ msgstr "" msgid "AddContextCommits|Add/remove" msgstr "" -msgid "AddMember|Emails cannot be blank" -msgstr "" - msgid "AddMember|Invite email is invalid" msgstr "" msgid "AddMember|Invite limit of %{daily_invites} per day exceeded" msgstr "" +msgid "AddMember|Invites cannot be blank" +msgstr "" + msgid "AddMember|No invite source provided." msgstr "" diff --git a/spec/controllers/jira_connect/subscriptions_controller_spec.rb b/spec/controllers/jira_connect/subscriptions_controller_spec.rb index f5644a8c7c8..f548c1f399d 100644 --- a/spec/controllers/jira_connect/subscriptions_controller_spec.rb +++ b/spec/controllers/jira_connect/subscriptions_controller_spec.rb @@ -75,18 +75,6 @@ RSpec.describe JiraConnect::SubscriptionsController do expect(json_response).to include('login_path' => nil) end end - - context 'with context qsh' do - # The JSON endpoint will be requested by frontend using a JWT that Atlassian provides via Javascript. - # This JWT will likely use a context-qsh because Atlassian don't know for which endpoint it will be used. - # Read more about context JWT here: https://developer.atlassian.com/cloud/jira/platform/understanding-jwt-for-connect-apps/ - - let(:qsh) { 'context-qsh' } - - specify do - expect(response).to have_gitlab_http_status(:ok) - end - end end end end @@ -114,7 +102,7 @@ RSpec.describe JiraConnect::SubscriptionsController do end context 'with valid JWT' do - let(:claims) { { iss: installation.client_key, sub: 1234, qsh: 'context-qsh' } } + let(:claims) { { iss: installation.client_key, sub: 1234 } } let(:jwt) { Atlassian::Jwt.encode(claims, installation.shared_secret) } let(:jira_user) { { 'groups' => { 'items' => [{ 'name' => jira_group_name }] } } } let(:jira_group_name) { 'site-admins' } @@ -170,7 +158,7 @@ RSpec.describe JiraConnect::SubscriptionsController do .stub_request(:get, "#{installation.base_url}/rest/api/3/user?accountId=1234&expand=groups") .to_return(body: jira_user.to_json, status: 200, headers: { 'Content-Type' => 'application/json' }) - delete :destroy, params: { jwt: jwt, id: subscription.id, format: :json } + delete :destroy, params: { jwt: jwt, id: subscription.id } end context 'without JWT' do @@ -182,7 +170,7 @@ RSpec.describe JiraConnect::SubscriptionsController do end context 'with valid JWT' do - let(:claims) { { iss: installation.client_key, sub: 1234, qsh: 'context-qsh' } } + let(:claims) { { iss: installation.client_key, sub: 1234 } } let(:jwt) { Atlassian::Jwt.encode(claims, installation.shared_secret) } it 'deletes the subscription' do diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index aa264ad3377..152ae061605 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -118,14 +118,5 @@ FactoryBot.define do create(:crm_settings, group: group, enabled: true) end end - - trait :test_group do - path { "test-group-fulfillment#{SecureRandom.hex(4)}" } - created_at { 4.days.ago } - - after(:create) do |group| - group.add_owner(create(:user, email: "test-user-#{SecureRandom.hex(4)}@test.com")) - end - end end end diff --git a/spec/frontend/groups/components/item_type_icon_spec.js b/spec/frontend/groups/components/item_type_icon_spec.js index 9310943841e..fcfb4c848c6 100644 --- a/spec/frontend/groups/components/item_type_icon_spec.js +++ b/spec/frontend/groups/components/item_type_icon_spec.js @@ -8,7 +8,6 @@ describe('ItemTypeIcon', () => { const defaultProps = { itemType: ITEM_TYPE.GROUP, - isGroupOpen: false, }; const createComponent = (props = {}) => { @@ -34,20 +33,14 @@ describe('ItemTypeIcon', () => { }); it.each` - type | isGroupOpen | icon - ${ITEM_TYPE.GROUP} | ${true} | ${'folder-open'} - ${ITEM_TYPE.GROUP} | ${false} | ${'folder-o'} - ${ITEM_TYPE.PROJECT} | ${true} | ${'bookmark'} - ${ITEM_TYPE.PROJECT} | ${false} | ${'bookmark'} - `( - 'shows "$icon" icon when `itemType` is "$type" and `isGroupOpen` is $isGroupOpen', - ({ type, isGroupOpen, icon }) => { - createComponent({ - itemType: type, - isGroupOpen, - }); - expect(findGlIcon().props('name')).toBe(icon); - }, - ); + type | icon + ${ITEM_TYPE.GROUP} | ${'group'} + ${ITEM_TYPE.PROJECT} | ${'project'} + `('shows "$icon" icon when `itemType` is "$type"', ({ type, icon }) => { + createComponent({ + itemType: type, + }); + expect(findGlIcon().props('name')).toBe(icon); + }); }); }); diff --git a/spec/lib/gitlab/background_migration/batching_strategies/backfill_issue_work_item_type_batching_strategy_spec.rb b/spec/lib/gitlab/background_migration/batching_strategies/backfill_issue_work_item_type_batching_strategy_spec.rb new file mode 100644 index 00000000000..52bc2b6b18a --- /dev/null +++ b/spec/lib/gitlab/background_migration/batching_strategies/backfill_issue_work_item_type_batching_strategy_spec.rb @@ -0,0 +1,136 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::BatchingStrategies::BackfillIssueWorkItemTypeBatchingStrategy, + '#next_batch' do + # let! can't be used in migration specs because all tables but `work_item_types` are deleted after each spec + let!(:issue_type_enum) { { issue: 0, incident: 1, test_case: 2, requirement: 3, task: 4 } } + let!(:namespace) { table(:namespaces).create!(name: 'namespace', path: 'namespace') } + let!(:project) { table(:projects).create!(namespace_id: namespace.id) } + let!(:issues_table) { table(:issues) } + let!(:task_type) { table(:work_item_types).find_by!(namespace_id: nil, base_type: issue_type_enum[:task]) } + + let!(:issue1) { issues_table.create!(project_id: project.id, issue_type: issue_type_enum[:issue]) } + let!(:task1) { issues_table.create!(project_id: project.id, issue_type: issue_type_enum[:task]) } + let!(:issue2) { issues_table.create!(project_id: project.id, issue_type: issue_type_enum[:issue]) } + let!(:issue3) { issues_table.create!(project_id: project.id, issue_type: issue_type_enum[:issue]) } + let!(:task2) { issues_table.create!(project_id: project.id, issue_type: issue_type_enum[:task]) } + let!(:incident1) { issues_table.create!(project_id: project.id, issue_type: issue_type_enum[:incident]) } + # test_case is EE only, but enum values exist on the FOSS model + let!(:test_case1) { issues_table.create!(project_id: project.id, issue_type: issue_type_enum[:test_case]) } + + let!(:task3) do + issues_table.create!(project_id: project.id, issue_type: issue_type_enum[:task], work_item_type_id: task_type.id) + end + + let!(:task4) { issues_table.create!(project_id: project.id, issue_type: issue_type_enum[:task]) } + + let!(:batching_strategy) { described_class.new(connection: ActiveRecord::Base.connection) } + + context 'when issue_type is issue' do + let(:job_arguments) { [issue_type_enum[:issue], 'irrelevant_work_item_id'] } + + context 'when starting on the first batch' do + it 'returns the bounds of the next batch' do + batch_bounds = next_batch(issue1.id, 2) + + expect(batch_bounds).to match_array([issue1.id, issue2.id]) + end + end + + context 'when additional batches remain' do + it 'returns the bounds of the next batch' do + batch_bounds = next_batch(issue2.id, 2) + + expect(batch_bounds).to match_array([issue2.id, issue3.id]) + end + end + + context 'when on the final batch' do + it 'returns the bounds of the next batch' do + batch_bounds = next_batch(issue3.id, 2) + + expect(batch_bounds).to match_array([issue3.id, issue3.id]) + end + end + + context 'when no additional batches remain' do + it 'returns nil' do + batch_bounds = next_batch(issue3.id + 1, 1) + + expect(batch_bounds).to be_nil + end + end + end + + context 'when issue_type is incident' do + let(:job_arguments) { [issue_type_enum[:incident], 'irrelevant_work_item_id'] } + + context 'when starting on the first batch' do + it 'returns the bounds of the next batch with only one element' do + batch_bounds = next_batch(incident1.id, 2) + + expect(batch_bounds).to match_array([incident1.id, incident1.id]) + end + end + end + + context 'when issue_type is requirement and there are no matching records' do + let(:job_arguments) { [issue_type_enum[:requirement], 'irrelevant_work_item_id'] } + + context 'when starting on the first batch' do + it 'returns nil' do + batch_bounds = next_batch(1, 2) + + expect(batch_bounds).to be_nil + end + end + end + + context 'when issue_type is task' do + let(:job_arguments) { [issue_type_enum[:task], 'irrelevant_work_item_id'] } + + context 'when starting on the first batch' do + it 'returns the bounds of the next batch' do + batch_bounds = next_batch(task1.id, 2) + + expect(batch_bounds).to match_array([task1.id, task2.id]) + end + end + + context 'when additional batches remain' do + it 'returns the bounds of the next batch, does not skip records where FK is already set' do + batch_bounds = next_batch(task2.id, 2) + + expect(batch_bounds).to match_array([task2.id, task3.id]) + end + end + + context 'when on the final batch' do + it 'returns the bounds of the next batch' do + batch_bounds = next_batch(task4.id, 2) + + expect(batch_bounds).to match_array([task4.id, task4.id]) + end + end + + context 'when no additional batches remain' do + it 'returns nil' do + batch_bounds = next_batch(task4.id + 1, 1) + + expect(batch_bounds).to be_nil + end + end + end + + def next_batch(min_value, batch_size) + batching_strategy.next_batch( + :issues, + :id, + batch_min_value: min_value, + batch_size: batch_size, + job_arguments: job_arguments + ) + end +end diff --git a/spec/lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy_spec.rb b/spec/lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy_spec.rb index 9fab1922cb7..521e2067744 100644 --- a/spec/lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy_spec.rb +++ b/spec/lib/gitlab/background_migration/batching_strategies/primary_key_batching_strategy_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchi context 'when starting on the first batch' do it 'returns the bounds of the next batch' do - batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace1.id, batch_size: 3, job_arguments: nil) + batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace1.id, batch_size: 3, job_arguments: []) expect(batch_bounds).to eq([namespace1.id, namespace3.id]) end @@ -23,7 +23,7 @@ RSpec.describe Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchi context 'when additional batches remain' do it 'returns the bounds of the next batch' do - batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace2.id, batch_size: 3, job_arguments: nil) + batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace2.id, batch_size: 3, job_arguments: []) expect(batch_bounds).to eq([namespace2.id, namespace4.id]) end @@ -31,7 +31,7 @@ RSpec.describe Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchi context 'when on the final batch' do it 'returns the bounds of the next batch' do - batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace4.id, batch_size: 3, job_arguments: nil) + batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace4.id, batch_size: 3, job_arguments: []) expect(batch_bounds).to eq([namespace4.id, namespace4.id]) end @@ -39,7 +39,7 @@ RSpec.describe Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchi context 'when no additional batches remain' do it 'returns nil' do - batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace4.id + 1, batch_size: 1, job_arguments: nil) + batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace4.id + 1, batch_size: 1, job_arguments: []) expect(batch_bounds).to be_nil end @@ -48,8 +48,10 @@ RSpec.describe Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchi context 'additional filters' do let(:strategy_with_filters) do Class.new(described_class) do - def apply_additional_filters(relation) - relation.where.not(type: 'Project') + def apply_additional_filters(relation, job_arguments:) + min_id = job_arguments.first + + relation.where.not(type: 'Project').where('id >= ?', min_id) end end end @@ -58,7 +60,7 @@ RSpec.describe Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchi let!(:namespace5) { namespaces.create!(name: 'batchtest5', path: 'batch-test5', type: 'Project') } it 'applies additional filters' do - batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace4.id, batch_size: 3, job_arguments: nil) + batch_bounds = batching_strategy.next_batch(:namespaces, :id, batch_min_value: namespace4.id, batch_size: 3, job_arguments: [1]) expect(batch_bounds).to eq([namespace4.id, namespace4.id]) end diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb index 2b63f44ddcc..7a433be0e2f 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -79,12 +79,23 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m describe '.active_migration' do let!(:migration1) { create(:batched_background_migration, :finished) } - let!(:migration2) { create(:batched_background_migration, :active) } - let!(:migration3) { create(:batched_background_migration, :active) } - it 'returns the first active migration according to queue order' do - expect(described_class.active_migration).to eq(migration2) - create(:batched_background_migration_job, :succeeded, batched_migration: migration1, batch_size: 1000) + context 'without migrations on hold' do + let!(:migration2) { create(:batched_background_migration, :active) } + let!(:migration3) { create(:batched_background_migration, :active) } + + it 'returns the first active migration according to queue order' do + expect(described_class.active_migration).to eq(migration2) + end + end + + context 'with migrations are on hold' do + let!(:migration2) { create(:batched_background_migration, :active, on_hold_until: 10.minutes.from_now) } + let!(:migration3) { create(:batched_background_migration, :active, on_hold_until: 2.minutes.ago) } + + it 'returns the first active migration that is not on hold according to queue order' do + expect(described_class.active_migration).to eq(migration3) + end end end @@ -518,6 +529,20 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m end end + describe '#hold!', :freeze_time do + subject { create(:batched_background_migration) } + + let(:time) { 5.minutes.from_now } + + it 'updates on_hold_until property' do + expect { subject.hold!(until_time: time) }.to change { subject.on_hold_until }.from(nil).to(time) + end + + it 'defaults to 10 minutes' do + expect { subject.hold! }.to change { subject.on_hold_until }.from(nil).to(10.minutes.from_now) + end + end + describe '.for_configuration' do let!(:migration) do create( diff --git a/spec/lib/gitlab/import_export/duration_measuring_spec.rb b/spec/lib/gitlab/import_export/duration_measuring_spec.rb new file mode 100644 index 00000000000..cf8b6060741 --- /dev/null +++ b/spec/lib/gitlab/import_export/duration_measuring_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe Gitlab::ImportExport::DurationMeasuring do + subject do + Class.new do + include Gitlab::ImportExport::DurationMeasuring + + def test + with_duration_measuring do + 'test' + end + end + end.new + end + + it 'measures method execution duration' do + subject.test + + expect(subject.duration_s).not_to be_nil + end + + describe '#with_duration_measuring' do + it 'yields control' do + expect { |block| subject.with_duration_measuring(&block) }.to yield_control + end + + it 'returns result of the yielded block' do + return_value = 'return_value' + + expect(subject.with_duration_measuring { return_value }).to eq(return_value) + end + end +end diff --git a/spec/migrations/backfill_work_item_type_id_on_issues_spec.rb b/spec/migrations/backfill_work_item_type_id_on_issues_spec.rb index ca7bf909583..6798b0cc7e8 100644 --- a/spec/migrations/backfill_work_item_type_id_on_issues_spec.rb +++ b/spec/migrations/backfill_work_item_type_id_on_issues_spec.rb @@ -32,7 +32,8 @@ RSpec.describe BackfillWorkItemTypeIdOnIssues, :migration do interval: interval, batch_size: described_class::BATCH_SIZE, max_batch_size: described_class::MAX_BATCH_SIZE, - sub_batch_size: described_class::SUB_BATCH_SIZE + sub_batch_size: described_class::SUB_BATCH_SIZE, + batch_class_name: described_class::BATCH_CLASS_NAME ) end end diff --git a/spec/migrations/replace_work_item_type_backfill_next_batch_strategy_spec.rb b/spec/migrations/replace_work_item_type_backfill_next_batch_strategy_spec.rb new file mode 100644 index 00000000000..5e22fc06973 --- /dev/null +++ b/spec/migrations/replace_work_item_type_backfill_next_batch_strategy_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe ReplaceWorkItemTypeBackfillNextBatchStrategy, :migration do + describe '#up' do + it 'sets the new strategy for existing migrations' do + migrations = create_migrations(described_class::OLD_STRATEGY_CLASS, 2) + + expect do + migrate! + + migrations.each(&:reload) + end.to change { migrations.pluck(:batch_class_name).uniq }.from([described_class::OLD_STRATEGY_CLASS]) + .to([described_class::NEW_STRATEGY_CLASS]) + end + end + + describe '#down' do + it 'sets the old strategy for existing migrations' do + migrations = create_migrations(described_class::NEW_STRATEGY_CLASS, 2) + + expect do + migrate! + schema_migrate_down! + + migrations.each(&:reload) + end.to change { migrations.pluck(:batch_class_name).uniq }.from([described_class::NEW_STRATEGY_CLASS]) + .to([described_class::OLD_STRATEGY_CLASS]) + end + end + + def create_migrations(batch_class_name, count) + Array.new(2) { |index| create_background_migration(batch_class_name, [index]) } + end + + def create_background_migration(batch_class_name, job_arguments) + migrations_table = table(:batched_background_migrations) + + migrations_table.create!( + batch_class_name: batch_class_name, + job_class_name: described_class::JOB_CLASS_NAME, + max_value: 10, + batch_size: 5, + sub_batch_size: 1, + interval: 2.minutes, + table_name: :issues, + column_name: :id, + total_tuple_count: 10_000, + pause_ms: 100, + job_arguments: job_arguments + ) + end +end diff --git a/spec/requests/api/invitations_spec.rb b/spec/requests/api/invitations_spec.rb index 0b9524a7ee7..e6cb7f1a07e 100644 --- a/spec/requests/api/invitations_spec.rb +++ b/spec/requests/api/invitations_spec.rb @@ -240,7 +240,7 @@ RSpec.describe API::Invitations do params: { email: '', access_level: Member::MAINTAINER } expect(response).to have_gitlab_http_status(:created) - expect(json_response['message']).to eq('Emails cannot be blank') + expect(json_response['message']).to eq('Invites cannot be blank') end it 'returns 404 when the email list is not a valid format' do diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 2e1f86605e8..74adbc4efc8 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -785,6 +785,25 @@ module Ci include_examples 'handles runner assignment' end + + context 'when a conflicting data is stored in denormalized table' do + let!(:specific_runner) { create(:ci_runner, :project, projects: [project], tag_list: %w[conflict]) } + let!(:pending_job) { create(:ci_build, :pending, :queued, pipeline: pipeline, tag_list: %w[conflict]) } + + before do + pending_job.update_column(:status, :running) + end + + it 'removes queuing entry upon build assignment attempt' do + expect(pending_job.reload).to be_running + expect(pending_job.queuing_entry).to be_present + + result = described_class.new(specific_runner).execute + + expect(result).not_to be_valid + expect(pending_job.reload.queuing_entry).not_to be_present + end + end end context 'when not using pending builds table' do diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb index ef43866d8d4..d3f537a1aa0 100644 --- a/spec/services/ci/update_build_queue_service_spec.rb +++ b/spec/services/ci/update_build_queue_service_spec.rb @@ -103,6 +103,28 @@ RSpec.describe Ci::UpdateBuildQueueService do end end end + + describe '#remove!' do + context 'when pending build exists' do + before do + create(:ci_pending_build, build: build, project: build.project) + end + + it 'removes pending build in a transaction' do + dequeued = subject.remove!(build) + + expect(dequeued).to eq build.id + end + end + + context 'when pending build does not exist' do + it 'does nothing if there is no pending build to remove' do + dequeued = subject.remove!(build) + + expect(dequeued).to be_nil + end + end + end end describe 'shared runner builds tracking' do diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index 4396a0d3ec3..05975d4d875 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ let_it_be(:source, reload: true) { create(:project) } let_it_be(:user) { create(:user) } let_it_be(:member) { create(:user) } + let_it_be(:user_invited_by_id) { create(:user) } let_it_be(:user_ids) { member.id.to_s } let_it_be(:access_level) { Gitlab::Access::GUEST } @@ -49,6 +50,36 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) end + context 'when user_id is passed as an integer' do + let(:user_ids) { member.id } + + it 'successfully creates member' do + expect(execute_service[:status]).to eq(:success) + expect(source.users).to include member + expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) + end + end + + context 'with user_ids as an array of integers' do + let(:user_ids) { [member.id, user_invited_by_id.id] } + + it 'successfully creates members' do + expect(execute_service[:status]).to eq(:success) + expect(source.users).to include(member, user_invited_by_id) + expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) + end + end + + context 'with user_ids as an array of strings' do + let(:user_ids) { [member.id.to_s, user_invited_by_id.id.to_s] } + + it 'successfully creates members' do + expect(execute_service[:status]).to eq(:success) + expect(source.users).to include(member, user_invited_by_id) + expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true) + end + end + context 'when executing on a group' do let_it_be(:source) { create(:group) } @@ -191,6 +222,15 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_ ) end + context 'when it is an invite by email passed to user_ids' do + let(:user_ids) { 'email@example.org' } + + it 'does not create task issues' do + expect(TasksToBeDone::CreateWorker).not_to receive(:perform_async) + execute_service + end + end + context 'when passing many user ids' do before do stub_licensed_features(multiple_issue_assignees: false) diff --git a/spec/services/members/invite_service_spec.rb b/spec/services/members/invite_service_spec.rb index 8ceb9979f33..ab740138a8b 100644 --- a/spec/services/members/invite_service_spec.rb +++ b/spec/services/members/invite_service_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ let_it_be(:project, reload: true) { create(:project) } let_it_be(:user) { project.first_owner } let_it_be(:project_user) { create(:user) } + let_it_be(:user_invited_by_id) { create(:user) } let_it_be(:namespace) { project.namespace } let(:params) { {} } @@ -41,148 +42,422 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_ end end - context 'when email is not a valid email' do - let(:params) { { email: '_bogus_' } } + context 'when invites are passed as array' do + context 'with emails' do + let(:params) { { email: %w[email@example.org email2@example.org] } } - it 'returns an error' do - expect_not_to_create_members - expect(result[:message]['_bogus_']).to eq("Invite email is invalid") + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end + end + + context 'with user_ids as integers' do + let(:params) { { user_ids: [project_user.id, user_invited_by_id.id] } } + + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end end - it_behaves_like 'does not record an onboarding progress action' + context 'with user_ids as strings' do + let(:params) { { user_ids: [project_user.id.to_s, user_invited_by_id.id.to_s] } } + + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end + end + + context 'with a mixture of emails and user_ids' do + let(:params) do + { user_ids: [project_user.id, user_invited_by_id.id], email: %w[email@example.org email2@example.org] } + end + + it 'successfully creates members' do + expect_to_create_members(count: 4) + expect(result[:status]).to eq(:success) + end + end end - context 'when emails are passed as an array' do - let(:params) { { email: %w[email@example.org email2@example.org] } } + context 'with multiple invites passed as strings' do + context 'with emails' do + let(:params) { { email: 'email@example.org,email2@example.org' } } - it 'successfully creates members' do - expect_to_create_members(count: 2) - expect(result[:status]).to eq(:success) + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end + end + + context 'with user_ids' do + let(:params) { { user_ids: "#{project_user.id},#{user_invited_by_id.id}" } } + + it 'successfully creates members' do + expect_to_create_members(count: 2) + expect(result[:status]).to eq(:success) + end + end + + context 'with a mixture of emails and user_ids' do + let(:params) do + { user_ids: "#{project_user.id},#{user_invited_by_id.id}", email: 'email@example.org,email2@example.org' } + end + + it 'successfully creates members' do + expect_to_create_members(count: 4) + expect(result[:status]).to eq(:success) + end end end - context 'when emails are passed as an empty string' do - let(:params) { { email: '' } } + context 'when invites formats are mixed' do + context 'when user_ids is an array and emails is a string' do + let(:params) do + { user_ids: [project_user.id, user_invited_by_id.id], email: 'email@example.org,email2@example.org' } + end + + it 'successfully creates members' do + expect_to_create_members(count: 4) + expect(result[:status]).to eq(:success) + end + end + + context 'when user_ids is a string and emails is an array' do + let(:params) do + { user_ids: "#{project_user.id},#{user_invited_by_id.id}", email: %w[email@example.org email2@example.org] } + end - it 'returns an error' do - expect_not_to_create_members - expect(result[:message]).to eq('Emails cannot be blank') + it 'successfully creates members' do + expect_to_create_members(count: 4) + expect(result[:status]).to eq(:success) + end end end - context 'when email param is not included' do - it 'returns an error' do - expect_not_to_create_members - expect(result[:message]).to eq('Emails cannot be blank') + context 'when invites are passed in different formats' do + context 'when emails are passed as an empty string' do + let(:params) { { email: '' } } + + it 'returns an error' do + expect_not_to_create_members + expect(result[:message]).to eq('Invites cannot be blank') + end + end + + context 'when user_ids are passed as an empty string' do + let(:params) { { user_ids: '' } } + + it 'returns an error' do + expect_not_to_create_members + expect(result[:message]).to eq('Invites cannot be blank') + end + end + + context 'when user_ids and emails are both passed as empty strings' do + let(:params) { { user_ids: '', email: '' } } + + it 'returns an error' do + expect_not_to_create_members + expect(result[:message]).to eq('Invites cannot be blank') + end + end + + context 'when user_id is passed as an integer' do + let(:params) { { user_ids: project_user.id } } + + it 'successfully creates member' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + end + end + + context 'when invite params are not included' do + it 'returns an error' do + expect_not_to_create_members + expect(result[:message]).to eq('Invites cannot be blank') + end end end context 'when email is not a valid email format' do - let(:params) { { email: '_bogus_' } } + context 'with singular email' do + let(:params) { { email: '_bogus_' } } - it 'returns an error' do - expect { result }.not_to change(ProjectMember, :count) - expect(result[:status]).to eq(:error) - expect(result[:message][params[:email]]).to eq("Invite email is invalid") + it 'returns an error' do + expect_not_to_create_members + expect(result[:status]).to eq(:error) + expect(result[:message][params[:email]]).to eq("Invite email is invalid") + end + + it_behaves_like 'does not record an onboarding progress action' + end + + context 'with user_id and singular invalid email' do + let(:params) { { user_ids: project_user.id, email: '_bogus_' } } + + it 'has partial success' do + expect_to_create_members(count: 1) + expect(project.users).to include project_user + + expect(result[:status]).to eq(:error) + expect(result[:message][params[:email]]).to eq("Invite email is invalid") + end end end - context 'when duplicate email addresses are passed' do - let(:params) { { email: 'email@example.org,email@example.org' } } + context 'with duplicate invites' do + context 'with duplicate emails' do + let(:params) { { email: 'email@example.org,email@example.org' } } - it 'only creates one member per unique address' do - expect_to_create_members(count: 1) - expect(result[:status]).to eq(:success) + it 'only creates one member per unique invite' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + end + end + + context 'with duplicate user ids' do + let(:params) { { user_ids: "#{project_user.id},#{project_user.id}" } } + + it 'only creates one member per unique invite' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + end + end + + context 'with duplicate member by adding as user id and email' do + let(:params) { { user_ids: project_user.id, email: project_user.email } } + + it 'only creates one member per unique invite' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + end end end - context 'when observing email limits' do - let_it_be(:emails) { Array(1..101).map { |n| "email#{n}@example.com" } } + context 'when observing invite limits' do + context 'with emails and in general' do + let_it_be(:emails) { Array(1..101).map { |n| "email#{n}@example.com" } } - context 'when over the allowed default limit of emails' do - let(:params) { { email: emails } } + context 'when over the allowed default limit of emails' do + let(:params) { { email: emails } } - it 'limits the number of emails to 100' do - expect_not_to_create_members - expect(result[:message]).to eq('Too many users specified (limit is 100)') + it 'limits the number of emails to 100' do + expect_not_to_create_members + expect(result[:message]).to eq('Too many users specified (limit is 100)') + end + end + + context 'when over the allowed custom limit of emails' do + let(:params) { { email: 'email@example.org,email2@example.org', limit: 1 } } + + it 'limits the number of emails to the limit supplied' do + expect_not_to_create_members + expect(result[:message]).to eq('Too many users specified (limit is 1)') + end + end + + context 'when limit allowed is disabled via limit param' do + let(:params) { { email: emails, limit: -1 } } + + it 'does not limit number of emails' do + expect_to_create_members(count: 101) + expect(result[:status]).to eq(:success) + end end end - context 'when over the allowed custom limit of emails' do - let(:params) { { email: 'email@example.org,email2@example.org', limit: 1 } } + context 'with user_ids' do + let(:user_ids) { 1.upto(101).to_a.join(',') } + let(:params) { { user_ids: user_ids } } - it 'limits the number of emails to the limit supplied' do + it 'limits the number of users to 100' do expect_not_to_create_members - expect(result[:message]).to eq('Too many users specified (limit is 1)') + expect(result[:message]).to eq('Too many users specified (limit is 100)') end end + end - context 'when limit allowed is disabled via limit param' do - let(:params) { { email: emails, limit: -1 } } + context 'with an existing user' do + context 'with email' do + let(:params) { { email: project_user.email } } - it 'does not limit number of emails' do - expect_to_create_members(count: 101) + it 'adds an existing user to members' do + expect_to_create_members(count: 1) expect(result[:status]).to eq(:success) + expect(project.users).to include project_user end end - end - context 'when email belongs to an existing user' do - let(:params) { { email: project_user.email } } + context 'with user_id' do + let(:params) { { user_ids: project_user.id } } - it 'adds an existing user to members' do - expect_to_create_members(count: 1) - expect(result[:status]).to eq(:success) - expect(project.users).to include project_user + it_behaves_like 'records an onboarding progress action', :user_added + + it 'adds an existing user to members' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:success) + expect(project.users).to include project_user + end + + context 'when assigning tasks to be done' do + let(:params) do + { user_ids: project_user.id, tasks_to_be_done: %w(ci code), tasks_project_id: project.id } + end + + it 'creates 2 task issues', :aggregate_failures do + expect(TasksToBeDone::CreateWorker) + .to receive(:perform_async) + .with(anything, user.id, [project_user.id]) + .once + .and_call_original + expect { result }.to change { project.issues.count }.by(2) + + expect(project.issues).to all have_attributes(project: project, author: user) + end + end end end context 'when access level is not valid' do - let(:params) { { email: project_user.email, access_level: -1 } } + context 'with email' do + let(:params) { { email: project_user.email, access_level: -1 } } - it 'returns an error' do - expect_not_to_create_members - expect(result[:message][project_user.email]) - .to eq("Access level is not included in the list") + it 'returns an error' do + expect_not_to_create_members + expect(result[:message][project_user.email]).to eq("Access level is not included in the list") + end end - end - context 'when invite already exists for an included email' do - let!(:invited_member) { create(:project_member, :invited, project: project) } - let(:params) { { email: "#{invited_member.invite_email},#{project_user.email}" } } + context 'with user_id' do + let(:params) { { user_ids: user_invited_by_id.id, access_level: -1 } } - it 'adds new email and returns an error for the already invited email' do - expect_to_create_members(count: 1) - expect(result[:status]).to eq(:error) - expect(result[:message][invited_member.invite_email]) - .to eq("The member's email address has already been taken") - expect(project.users).to include project_user + it 'returns an error' do + expect_not_to_create_members + expect(result[:message][user_invited_by_id.username]).to eq("Access level is not included in the list") + end end - end - context 'when access request already exists for an included email' do - let!(:requested_member) { create(:project_member, :access_request, project: project) } - let(:params) { { email: "#{requested_member.user.email},#{project_user.email}" } } + context 'with a mix of user_id and email' do + let(:params) { { user_ids: user_invited_by_id.id, email: project_user.email, access_level: -1 } } - it 'adds new email and returns an error for the already invited email' do - expect_to_create_members(count: 1) - expect(result[:status]).to eq(:error) - expect(result[:message][requested_member.user.email]) - .to eq("User already exists in source") - expect(project.users).to include project_user + it 'returns errors' do + expect_not_to_create_members + expect(result[:message][project_user.email]).to eq("Access level is not included in the list") + expect(result[:message][user_invited_by_id.username]).to eq("Access level is not included in the list") + end end end - context 'when email is already a member on the project' do - let!(:existing_member) { create(:project_member, :guest, project: project) } - let(:params) { { email: "#{existing_member.user.email},#{project_user.email}" } } + context 'when member already exists' do + context 'with email' do + let!(:invited_member) { create(:project_member, :invited, project: project) } + let(:params) { { email: "#{invited_member.invite_email},#{project_user.email}" } } + + it 'adds new email and returns an error for the already invited email' do + expect_to_create_members(count: 1) + expect(result[:status]).to eq(:error) + expect(result[:message][invited_member.invite_email]) + .to eq("The member's email address has already been taken") + expect(project.users).to include project_user + end + end - it 'adds new email and returns an error for the already invited email' do - expect_to_create_members(count: 1) - expect(result[:status]).to eq(:error) - expect(result[:message][existing_member.user.email]) - .to eq("User already exists in source") - expect(project.users).to include project_user + context 'when email is already a member with a user on the project' do + let!(:existing_member) { create(:project_member, :guest, project: project) } + let(:params) { { email: "#{existing_member.user.email}" } } + + it 'returns an error for the already invited email' do + expect_not_to_create_members + expect(result[:message][existing_member.user.email]).to eq("User already exists in source") + end + + context 'when email belongs to an existing user as a secondary email' do + let(:secondary_email) { create(:email, email: 'secondary@example.com', user: existing_member.user) } + let(:params) { { email: "#{secondary_email.email}" } } + + it 'returns an error for the already invited email' do + expect_not_to_create_members + expect(result[:message][secondary_email.email]).to eq("User already exists in source") + end + end + end + + context 'with user_id that already exists' do + let!(:existing_member) { create(:project_member, project: project, user: project_user) } + let(:params) { { user_ids: existing_member.user_id } } + + it 'does not add the member again and is successful' do + expect_to_create_members(count: 0) + expect(project.users).to include project_user + end + end + + context 'with user_id that already exists with a lower access_level' do + let!(:existing_member) { create(:project_member, :developer, project: project, user: project_user) } + let(:params) { { user_ids: existing_member.user_id, access_level: ProjectMember::MAINTAINER } } + + it 'does not add the member again and updates the access_level' do + expect_to_create_members(count: 0) + expect(project.users).to include project_user + expect(existing_member.reset.access_level).to eq ProjectMember::MAINTAINER + end + end + + context 'with user_id that already exists with a higher access_level' do + let!(:existing_member) { create(:project_member, :developer, project: project, user: project_user) } + let(:params) { { user_ids: existing_member.user_id, access_level: ProjectMember::GUEST } } + + it 'does not add the member again and updates the access_level' do + expect_to_create_members(count: 0) + expect(project.users).to include project_user + expect(existing_member.reset.access_level).to eq ProjectMember::GUEST + end + end + + context 'with user_id that already exists in a parent group' do + let_it_be(:group) { create(:group) } + let_it_be(:group_member) { create(:group_member, :developer, source: group, user: project_user) } + let_it_be(:project, reload: true) { create(:project, group: group) } + let_it_be(:user) { project.creator } + + before_all do + project.add_maintainer(user) + end + + context 'when access_level is lower than inheriting member' do + let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::GUEST }} + + it 'does not add the member and returns an error' do + msg = "Access level should be greater than or equal " \ + "to Developer inherited membership from group #{group.name}" + + expect_not_to_create_members + expect(result[:message][project_user.username]).to eq msg + end + end + + context 'when access_level is the same as the inheriting member' do + let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::DEVELOPER }} + + it 'adds the member with correct access_level' do + expect_to_create_members(count: 1) + expect(project.users).to include project_user + expect(project.project_members.last.access_level).to eq ProjectMember::DEVELOPER + end + end + + context 'when access_level is greater than the inheriting member' do + let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::MAINTAINER }} + + it 'adds the member with correct access_level' do + expect_to_create_members(count: 1) + expect(project.users).to include project_user + expect(project.project_members.last.access_level).to eq ProjectMember::MAINTAINER + end + end end end diff --git a/spec/tasks/dev_rake_spec.rb b/spec/tasks/dev_rake_spec.rb index 0abed33ec87..af7bd4bdf0b 100644 --- a/spec/tasks/dev_rake_spec.rb +++ b/spec/tasks/dev_rake_spec.rb @@ -40,6 +40,58 @@ RSpec.describe 'dev rake tasks' do end end + describe 'terminate_all_connections' do + let(:connections) do + Gitlab::Database.database_base_models.values.filter_map do |model| + model.connection if Gitlab::Database.db_config_share_with(model.connection_db_config).nil? + end + end + + def expect_connections_to_be_terminated + expect(Gitlab::Database::EachDatabase).to receive(:each_database_connection) + .with(include_shared: false) + .and_call_original + + expect(connections).to all(receive(:execute).with(/SELECT pg_terminate_backend/)) + end + + def expect_connections_not_to_be_terminated + connections.each do |connection| + expect(connection).not_to receive(:execute) + end + end + + subject(:terminate_task) { run_rake_task('dev:terminate_all_connections') } + + it 'terminates all connections' do + expect_connections_to_be_terminated + + terminate_task + end + + context 'when in the production environment' do + it 'does not terminate connections' do + expect(Rails.env).to receive(:production?).and_return(true) + expect_connections_not_to_be_terminated + + terminate_task + end + end + + context 'when a database is not found' do + before do + skip_if_multiple_databases_not_setup + end + + it 'continues to next connection' do + expect(connections.first).to receive(:execute).and_raise(ActiveRecord::NoDatabaseError) + expect(connections.second).to receive(:execute).with(/SELECT pg_terminate_backend/) + + terminate_task + end + end + end + context 'multiple databases' do before do skip_if_multiple_databases_not_setup @@ -48,6 +100,8 @@ RSpec.describe 'dev rake tasks' do context 'with a valid database' do describe 'copy_db:ci' do before do + allow(Rake::Task['dev:terminate_all_connections']).to receive(:invoke) + configurations = instance_double(ActiveRecord::DatabaseConfigurations) allow(ActiveRecord::Base).to receive(:configurations).and_return(configurations) allow(configurations).to receive(:configs_for).with(env_name: Rails.env, name: 'ci').and_return(ci_configuration) @@ -63,6 +117,8 @@ RSpec.describe 'dev rake tasks' do template: ApplicationRecord.connection_db_config.database ) + expect(Rake::Task['dev:terminate_all_connections']).to receive(:invoke) + run_rake_task('dev:copy_db:ci') end diff --git a/spec/tasks/gitlab/setup_rake_spec.rb b/spec/tasks/gitlab/setup_rake_spec.rb index 6b4dde22dca..c31546fc259 100644 --- a/spec/tasks/gitlab/setup_rake_spec.rb +++ b/spec/tasks/gitlab/setup_rake_spec.rb @@ -6,6 +6,7 @@ RSpec.describe 'gitlab:setup namespace rake tasks', :silence_stdout do before do Rake.application.rake_require 'active_record/railties/databases' Rake.application.rake_require 'tasks/seed_fu' + Rake.application.rake_require 'tasks/dev' Rake.application.rake_require 'tasks/gitlab/setup' end @@ -22,12 +23,6 @@ RSpec.describe 'gitlab:setup namespace rake tasks', :silence_stdout do let(:server_service1) { double(:server_service) } let(:server_service2) { double(:server_service) } - let(:connections) do - Gitlab::Database.database_base_models.values.filter_map do |model| - model.connection if Gitlab::Database.db_config_share_with(model.connection_db_config).nil? - end - end - before do allow(Gitlab).to receive_message_chain('config.repositories.storages').and_return(storages) @@ -102,18 +97,6 @@ RSpec.describe 'gitlab:setup namespace rake tasks', :silence_stdout do end end - context 'when the database is not found when terminating connections' do - it 'continues setting up the database', :aggregate_failures do - expect_gitaly_connections_to_be_checked - - expect(connections).to all(receive(:execute).and_raise(ActiveRecord::NoDatabaseError)) - - expect_database_to_be_setup - - setup_task - end - end - def expect_gitaly_connections_to_be_checked expect(Gitlab::GitalyClient::ServerService).to receive(:new).with('name1').and_return(server_service1) expect(server_service1).to receive(:info) @@ -123,17 +106,11 @@ RSpec.describe 'gitlab:setup namespace rake tasks', :silence_stdout do end def expect_connections_to_be_terminated - expect(Gitlab::Database::EachDatabase).to receive(:each_database_connection) - .with(include_shared: false) - .and_call_original - - expect(connections).to all(receive(:execute).with(/SELECT pg_terminate_backend/)) + expect(Rake::Task['dev:terminate_all_connections']).to receive(:invoke) end def expect_connections_not_to_be_terminated - connections.each do |connection| - expect(connection).not_to receive(:execute) - end + expect(Rake::Task['dev:terminate_all_connections']).not_to receive(:invoke) end def expect_database_to_be_setup diff --git a/spec/workers/project_export_worker_spec.rb b/spec/workers/project_export_worker_spec.rb index 9923d8bde7f..dd0a921059d 100644 --- a/spec/workers/project_export_worker_spec.rb +++ b/spec/workers/project_export_worker_spec.rb @@ -4,4 +4,30 @@ require 'spec_helper' RSpec.describe ProjectExportWorker do it_behaves_like 'export worker' + + context 'exporters duration measuring' do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:worker) { described_class.new } + + subject { worker.perform(user.id, project.id) } + + before do + project.add_owner(user) + end + + it 'logs exporters execution duration' do + expect(worker).to receive(:log_extra_metadata_on_done).with(:version_saver_duration_s, anything) + expect(worker).to receive(:log_extra_metadata_on_done).with(:avatar_saver_duration_s, anything) + expect(worker).to receive(:log_extra_metadata_on_done).with(:tree_saver_duration_s, anything) + expect(worker).to receive(:log_extra_metadata_on_done).with(:uploads_saver_duration_s, anything) + expect(worker).to receive(:log_extra_metadata_on_done).with(:repo_saver_duration_s, anything) + expect(worker).to receive(:log_extra_metadata_on_done).with(:wiki_repo_saver_duration_s, anything) + expect(worker).to receive(:log_extra_metadata_on_done).with(:lfs_saver_duration_s, anything) + expect(worker).to receive(:log_extra_metadata_on_done).with(:snippets_repo_saver_duration_s, anything) + expect(worker).to receive(:log_extra_metadata_on_done).with(:design_repo_saver_duration_s, anything) + + subject + end + end end diff --git a/spec/workers/quality/test_data_cleanup_worker_spec.rb b/spec/workers/quality/test_data_cleanup_worker_spec.rb deleted file mode 100644 index a17e6e0cb1a..00000000000 --- a/spec/workers/quality/test_data_cleanup_worker_spec.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Quality::TestDataCleanupWorker do - subject { described_class.new } - - shared_examples 'successful deletion' do - before do - allow(Gitlab).to receive(:staging?).and_return(true) - end - - it 'removes test groups' do - expect { subject.perform }.to change(Group, :count).by(-test_group_count) - end - end - - describe "#perform" do - context 'with multiple test groups to remove' do - let(:test_group_count) { 5 } - let!(:groups_to_remove) { create_list(:group, test_group_count, :test_group) } - let!(:group_to_keep) { create(:group, path: 'test-group-fulfillment-keep', created_at: 1.day.ago) } - let!(:non_test_group) { create(:group) } - let(:non_test_owner_group) { create(:group, path: 'test-group-fulfillment1234', created_at: 4.days.ago) } - - before do - non_test_owner_group.add_owner(create(:user)) - end - - it_behaves_like 'successful deletion' - end - - context 'with paid groups' do - let(:test_group_count) { 1 } - let!(:paid_group) { create(:group, :test_group) } - - before do - allow(paid_group).to receive(:paid?).and_return(true) - end - - it_behaves_like 'successful deletion' - end - end -end diff --git a/workhorse/internal/proxy/proxy.go b/workhorse/internal/proxy/proxy.go index cd8376285e1..06e2c65a6a8 100644 --- a/workhorse/internal/proxy/proxy.go +++ b/workhorse/internal/proxy/proxy.go @@ -57,6 +57,14 @@ func NewProxy(myURL *url.URL, version string, roundTripper http.RoundTripper, op previousDirector := p.reverseProxy.Director p.reverseProxy.Director = func(request *http.Request) { previousDirector(request) + + // send original host along for the upstream + // to know it's being proxied under a different Host + // (for redirects and other stuff that depends on this) + request.Header.Set("X-Forwarded-Host", request.Host) + request.Header.Set("Forwarded", fmt.Sprintf("host=%s", request.Host)) + + // override the Host with the target request.Host = request.URL.Host } } diff --git a/workhorse/proxy_test.go b/workhorse/proxy_test.go index af79399cf44..02148c07522 100644 --- a/workhorse/proxy_test.go +++ b/workhorse/proxy_test.go @@ -41,6 +41,8 @@ func TestProxyRequest(t *testing.T) { require.Equal(t, "test", r.Header.Get("Custom-Header"), "custom header") require.Equal(t, testVersion, r.Header.Get("Gitlab-Workhorse"), "version header") require.Equal(t, inboundURL.Host, r.Host, "sent host header") + require.Empty(t, r.Header.Get("X-Forwarded-Host"), "X-Forwarded-Host header") + require.Empty(t, r.Header.Get("Forwarded"), "Forwarded header") require.Regexp( t, @@ -78,6 +80,8 @@ func TestProxyWithForcedTargetHostHeader(t *testing.T) { urlRegexp := regexp.MustCompile(fmt.Sprintf(`%s\z`, inboundURL.Path)) ts := testhelper.TestServerWithHandler(urlRegexp, func(w http.ResponseWriter, r *http.Request) { require.Equal(t, tsUrl.Host, r.Host, "upstream host header") + require.Equal(t, inboundURL.Host, r.Header.Get("X-Forwarded-Host"), "X-Forwarded-Host header") + require.Equal(t, fmt.Sprintf("host=%s", inboundURL.Host), r.Header.Get("Forwarded"), "Forwarded header") _, err := w.Write([]byte(`ok`)) require.NoError(t, err, "write ok response") |