diff --git a/.gitpod.yml b/.gitpod.yml index dc1dbc1472d..a28a11d6612 100644 --- a/.gitpod.yml +++ b/.gitpod.yml @@ -13,7 +13,10 @@ tasks: ( set -e cd /workspace/gitlab-development-kit - [[ ! -L /workspace/gitlab-development-kit/gitlab ]] && ln -fs /workspace/gitlab /workspace/gitlab-development-kit/gitlab + # GitLab FOSS + [[ -d /workspace/gitlab-foss ]] && ln -fs /workspace/gitlab-foss /workspace/gitlab-development-kit/gitlab + # GitLab + [[ -d /workspace/gitlab ]] && ln -fs /workspace/gitlab /workspace/gitlab-development-kit/gitlab mv /workspace/gitlab-development-kit/secrets.yml /workspace/gitlab-development-kit/gitlab/config # reconfigure GDK echo "$(date) – Reconfiguring GDK" | tee -a /workspace/startup.log @@ -47,9 +50,11 @@ tasks: if [ "$GITLAB_RUN_DB_MIGRATIONS" == true ]; then make gitlab-db-migrate fi - cd ../gitlab + cd /workspace/gitlab-development-kit/gitlab + # Install Lefthook + bundle exec lefthook install git checkout db/structure.sql - cd ../gitlab-development-kit + cd /workspace/gitlab-development-kit # Waiting for GitLab ... gp await-port 3000 printf "Waiting for GitLab at $(gp url 3000) ..." diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 4ec561014a8..b23cc4a302d 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -3,9 +3,6 @@ module SpammableActions extend ActiveSupport::Concern - include Recaptcha::Verify - include Gitlab::Utils::StrongMemoize - included do before_action :authorize_submit_spammable!, only: :mark_as_spam end @@ -21,9 +18,7 @@ module SpammableActions private def ensure_spam_config_loaded! - strong_memoize(:spam_config_loaded) do - Gitlab::Recaptcha.load_configurations! - end + Gitlab::Recaptcha.load_configurations! end def recaptcha_check_with_fallback(should_redirect = true, &fallback) @@ -50,33 +45,30 @@ module SpammableActions end def spammable_params - default_params = { request: request } - - recaptcha_check = recaptcha_response && - ensure_spam_config_loaded! && - verify_recaptcha(response: recaptcha_response) - - return default_params unless recaptcha_check - - { recaptcha_verified: true, - spam_log_id: params[:spam_log_id] }.merge(default_params) - end - - def recaptcha_response - # NOTE: This field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the recaptcha - # gem, which is called from the HAML `_recaptcha_form.html.haml` form. + # NOTE: For the legacy reCAPTCHA implementation based on the HTML/HAML form, the + # 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the + # recaptcha gem, which is called from the HAML `_recaptcha_form.html.haml` form. # - # It is used in the `Recaptcha::Verify#verify_recaptcha` if the `response` option is not - # passed explicitly. + # It is used in the `Recaptcha::Verify#verify_recaptcha` to extract the value from `params`, + # if the `response` option is not passed explicitly. # # Instead of relying on this behavior, we are extracting and passing it explicitly. This will # make it consistent with the newer, modern reCAPTCHA verification process as it will be # implemented via the GraphQL API and in Vue components via the native reCAPTCHA Javascript API, # which requires that the recaptcha response param be obtained and passed explicitly. # - # After this newer GraphQL/JS API process is fully supported by the backend, we can remove this - # (and other) HAML-specific support. - params['g-recaptcha-response'] + # It can also be expanded to multiple fields when we move to future alternative captcha + # implementations such as FriendlyCaptcha. See https://gitlab.com/gitlab-org/gitlab/-/issues/273480 + + # After this newer GraphQL/JS API process is fully supported by the backend, we can remove the + # check for the 'g-recaptcha-response' field and other HTML/HAML form-specific support. + captcha_response = params['g-recaptcha-response'] + + { + request: request, + spam_log_id: params[:spam_log_id], + captcha_response: captcha_response + } end def spammable diff --git a/app/finders/merge_request/metrics_finder.rb b/app/finders/merge_request/metrics_finder.rb new file mode 100644 index 00000000000..d93e53d1636 --- /dev/null +++ b/app/finders/merge_request/metrics_finder.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +class MergeRequest::MetricsFinder + include Gitlab::Allowable + + def initialize(current_user, params = {}) + @current_user = current_user + @params = params + end + + def execute + return klass.none if target_project.blank? || user_not_authorized? + + items = init_collection + items = by_target_project(items) + items = by_merged_after(items) + items = by_merged_before(items) + + items + end + + private + + attr_reader :current_user, :params + + def by_target_project(items) + items.by_target_project(target_project) + end + + def by_merged_after(items) + return items unless merged_after + + items.merged_after(merged_after) + end + + def by_merged_before(items) + return items unless merged_before + + items.merged_before(merged_before) + end + + def user_not_authorized? + !can?(current_user, :read_merge_request, target_project) + end + + def init_collection + klass.all + end + + def klass + MergeRequest::Metrics + end + + def target_project + params[:target_project] + end + + def merged_after + params[:merged_after] + end + + def merged_before + params[:merged_before] + end +end diff --git a/app/graphql/resolvers/project_merge_requests_resolver.rb b/app/graphql/resolvers/project_merge_requests_resolver.rb index 830649d5e52..21d9afc31ab 100644 --- a/app/graphql/resolvers/project_merge_requests_resolver.rb +++ b/app/graphql/resolvers/project_merge_requests_resolver.rb @@ -6,5 +6,38 @@ module Resolvers accept_assignee accept_author accept_reviewer + + def resolve(**args) + scope = super + + if only_count_is_selected_with_merged_at_filter?(args) && Feature.enabled?(:optimized_merge_request_count_with_merged_at_filter) + MergeRequest::MetricsFinder + .new(current_user, args.merge(target_project: project)) + .execute + else + scope + end + end + + def only_count_is_selected_with_merged_at_filter?(args) + return unless lookahead + + argument_names = args.except(:lookahead, :sort, :merged_before, :merged_after).keys + + # no extra filtering arguments are provided + return unless argument_names.empty? + return unless args[:merged_after] || args[:merged_before] + + # Detecting a specific query pattern: + # mergeRequests(mergedAfter: "X", mergedBefore: "Y") { + # count + # totalTimeToMerge + # } + allowed_selected_fields = [:count, :total_time_to_merge] + selected_fields = lookahead.selections.map(&:field).map(&:original_name) + + # only the allowed_selected_fields are present + (selected_fields - allowed_selected_fields).empty? + end end end diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index e44d3d96334..3bc8bd2661f 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -676,7 +676,7 @@ module Ci def number_of_warnings BatchLoader.for(id).batch(default_value: 0) do |pipeline_ids, loader| - ::Ci::Build.where(commit_id: pipeline_ids) + ::CommitStatus.where(commit_id: pipeline_ids) .latest .failed_but_allowed .group(:commit_id) diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index cc6bd1870b9..ae80692d598 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -118,7 +118,7 @@ module Ci def number_of_warnings BatchLoader.for(id).batch(default_value: 0) do |stage_ids, loader| - ::Ci::Build.where(stage_id: stage_ids) + ::CommitStatus.where(stage_id: stage_ids) .latest .failed_but_allowed .group(:stage_id) diff --git a/app/models/merge_request/metrics.rb b/app/models/merge_request/metrics.rb index d3fe256fb1b..5c611da0684 100644 --- a/app/models/merge_request/metrics.rb +++ b/app/models/merge_request/metrics.rb @@ -5,12 +5,14 @@ class MergeRequest::Metrics < ApplicationRecord belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id belongs_to :latest_closed_by, class_name: 'User' belongs_to :merged_by, class_name: 'User' + belongs_to :target_project, class_name: 'Project', inverse_of: :merge_requests before_save :ensure_target_project_id scope :merged_after, ->(date) { where(arel_table[:merged_at].gteq(date)) } scope :merged_before, ->(date) { where(arel_table[:merged_at].lteq(date)) } scope :with_valid_time_to_merge, -> { where(arel_table[:merged_at].gt(arel_table[:created_at])) } + scope :by_target_project, ->(project) { where(target_project_id: project) } def self.time_to_merge_expression Arel.sql('EXTRACT(epoch FROM SUM(AGE(merge_request_metrics.merged_at, merge_request_metrics.created_at)))') @@ -21,6 +23,12 @@ class MergeRequest::Metrics < ApplicationRecord def ensure_target_project_id self.target_project_id ||= merge_request.target_project_id end + + def self.total_time_to_merge + with_valid_time_to_merge + .pluck(time_to_merge_expression) + .first + end end MergeRequest::Metrics.prepend_if_ee('EE::MergeRequest::Metrics') diff --git a/app/models/project.rb b/app/models/project.rb index 3f46773bb8c..aad2e7d2259 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -218,6 +218,7 @@ class Project < ApplicationRecord # Merge Requests for target project should be removed with it has_many :merge_requests, foreign_key: 'target_project_id', inverse_of: :target_project + has_many :merge_request_metrics, foreign_key: 'target_project', class_name: 'MergeRequest::Metrics', inverse_of: :target_project has_many :source_of_merge_requests, foreign_key: 'source_project_id', class_name: 'MergeRequest' has_many :issues has_many :labels, class_name: 'ProjectLabel' diff --git a/app/services/captcha/captcha_verification_service.rb b/app/services/captcha/captcha_verification_service.rb new file mode 100644 index 00000000000..45a5a52367c --- /dev/null +++ b/app/services/captcha/captcha_verification_service.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Captcha + ## + # Encapsulates logic of checking captchas. + # + class CaptchaVerificationService + include Recaptcha::Verify + + ## + # Performs verification of a captcha response. + # + # 'captcha_response' parameter is the response from the user solving a client-side captcha. + # + # 'request' parameter is the request which submitted the captcha. + # + # NOTE: Currently only supports reCAPTCHA, and is not yet used in all places of the app in which + # captchas are verified, but these can be addressed in future MRs. See: + # https://gitlab.com/gitlab-org/gitlab/-/issues/273480 + def execute(captcha_response: nil, request:) + return false unless captcha_response + + @request = request + + Gitlab::Recaptcha.load_configurations! + + # NOTE: We could pass the model and let the recaptcha gem automatically add errors to it, + # but we do not, for two reasons: + # + # 1. We want control over when the errors are added + # 2. We want control over the wording and i18n of the message + # 3. We want a consistent interface and behavior when adding support for other captcha + # libraries which may not support automatically adding errors to the model. + verify_recaptcha(response: captcha_response) + end + + private + + # The recaptcha library's Recaptcha::Verify#verify_recaptcha method requires that + # 'request' be a readable attribute - it doesn't support passing it as an options argument. + attr_reader :request + end +end diff --git a/app/services/concerns/spam_check_methods.rb b/app/services/concerns/spam_check_methods.rb deleted file mode 100644 index 939f8f183ab..00000000000 --- a/app/services/concerns/spam_check_methods.rb +++ /dev/null @@ -1,39 +0,0 @@ -# frozen_string_literal: true - -# SpamCheckMethods -# -# Provide helper methods for checking if a given spammable object has -# potential spam data. -# -# Dependencies: -# - params with :request - -module SpamCheckMethods - # rubocop:disable Gitlab/ModuleWithInstanceVariables - def filter_spam_check_params - @request = params.delete(:request) - @api = params.delete(:api) - @recaptcha_verified = params.delete(:recaptcha_verified) - @spam_log_id = params.delete(:spam_log_id) - end - # rubocop:enable Gitlab/ModuleWithInstanceVariables - - # In order to be proceed to the spam check process, @spammable has to be - # a dirty instance, which means it should be already assigned with the new - # attribute values. - # rubocop:disable Gitlab/ModuleWithInstanceVariables - def spam_check(spammable, user, action:) - raise ArgumentError.new('Please provide an action, such as :create') unless action - - Spam::SpamActionService.new( - spammable: spammable, - request: @request, - user: user, - context: { action: action } - ).execute( - api: @api, - recaptcha_verified: @recaptcha_verified, - spam_log_id: @spam_log_id) - end - # rubocop:enable Gitlab/ModuleWithInstanceVariables -end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 44de8eb6389..d2285a375a1 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -2,20 +2,26 @@ module Issues class CreateService < Issues::BaseService - include SpamCheckMethods include ResolveDiscussions def execute(skip_system_notes: false) + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + @issue = BuildService.new(project, current_user, params).execute - filter_spam_check_params filter_resolve_discussion_params create(@issue, skip_system_notes: skip_system_notes) end def before_create(issue) - spam_check(issue, current_user, action: :create) + Spam::SpamActionService.new( + spammable: issue, + request: request, + user: current_user, + action: :create + ).execute(spam_params: spam_params) # current_user (defined in BaseService) is not available within run_after_commit block user = current_user @@ -46,8 +52,10 @@ module Issues private + attr_reader :request, :spam_params + def user_agent_detail_service - UserAgentDetailService.new(@issue, @request) + UserAgentDetailService.new(@issue, request) end # Applies label "incident" (creates it if missing) to incident issues. diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 127ed04cf51..2906bdf62a7 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -2,12 +2,14 @@ module Issues class UpdateService < Issues::BaseService - include SpamCheckMethods extend ::Gitlab::Utils::Override def execute(issue) handle_move_between_ids(issue) - filter_spam_check_params + + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + change_issue_duplicate(issue) move_issue_to_new_project(issue) || clone_issue(issue) || update_task_event(issue) || update(issue) end @@ -30,7 +32,14 @@ module Issues end def before_update(issue, skip_spam_check: false) - spam_check(issue, current_user, action: :update) unless skip_spam_check + return if skip_spam_check + + Spam::SpamActionService.new( + spammable: issue, + request: request, + user: current_user, + action: :update + ).execute(spam_params: spam_params) end def after_update(issue) @@ -126,6 +135,8 @@ module Issues private + attr_reader :request, :spam_params + def clone_issue(issue) target_project = params.delete(:target_clone_project) with_notes = params.delete(:clone_with_notes) diff --git a/app/services/snippets/base_service.rb b/app/services/snippets/base_service.rb index 278857b7933..415cfcb7d8f 100644 --- a/app/services/snippets/base_service.rb +++ b/app/services/snippets/base_service.rb @@ -2,8 +2,6 @@ module Snippets class BaseService < ::BaseService - include SpamCheckMethods - UPDATE_COMMIT_MSG = 'Update snippet' INITIAL_COMMIT_MSG = 'Initial commit' @@ -18,8 +16,6 @@ module Snippets input_actions = Array(@params.delete(:snippet_actions).presence) @snippet_actions = SnippetInputActionCollection.new(input_actions, allowed_actions: restricted_files_actions) - - filter_spam_check_params end private diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 0881be73eaf..9b0042c988a 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -3,20 +3,28 @@ module Snippets class CreateService < Snippets::BaseService def execute + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + @snippet = build_from_params return invalid_params_error(@snippet) unless valid_params? - unless visibility_allowed?(@snippet, @snippet.visibility_level) - return forbidden_visibility_error(@snippet) + unless visibility_allowed?(snippet, snippet.visibility_level) + return forbidden_visibility_error(snippet) end @snippet.author = current_user - spam_check(@snippet, current_user, action: :create) + Spam::SpamActionService.new( + spammable: @snippet, + request: request, + user: current_user, + action: :create + ).execute(spam_params: spam_params) if save_and_commit - UserAgentDetailService.new(@snippet, @request).create + UserAgentDetailService.new(@snippet, request).create Gitlab::UsageDataCounters::SnippetCounter.count(:create) move_temporary_files @@ -29,6 +37,8 @@ module Snippets private + attr_reader :snippet, :request, :spam_params + def build_from_params if project project.snippets.build(create_params) diff --git a/app/services/snippets/update_service.rb b/app/services/snippets/update_service.rb index b982ff98747..a723cf763d9 100644 --- a/app/services/snippets/update_service.rb +++ b/app/services/snippets/update_service.rb @@ -7,6 +7,9 @@ module Snippets UpdateError = Class.new(StandardError) def execute(snippet) + @request = params.delete(:request) + @spam_params = Spam::SpamActionService.filter_spam_params!(params) + return invalid_params_error(snippet) unless valid_params? if visibility_changed?(snippet) && !visibility_allowed?(snippet, visibility_level) @@ -14,7 +17,12 @@ module Snippets end update_snippet_attributes(snippet) - spam_check(snippet, current_user, action: :update) + Spam::SpamActionService.new( + spammable: snippet, + request: request, + user: current_user, + action: :update + ).execute(spam_params: spam_params) if save_and_commit(snippet) Gitlab::UsageDataCounters::SnippetCounter.count(:update) @@ -27,6 +35,8 @@ module Snippets private + attr_reader :request, :spam_params + def visibility_changed?(snippet) visibility_level && visibility_level.to_i != snippet.visibility_level end diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index b3d617256ff..ff32bc32d93 100644 --- a/app/services/spam/spam_action_service.rb +++ b/app/services/spam/spam_action_service.rb @@ -4,37 +4,69 @@ module Spam class SpamActionService include SpamConstants + ## + # Utility method to filter SpamParams from parameters, which will later be passed to #execute + # after the spammable is created/updated based on the remaining parameters. + # + # Takes a hash of parameters from an incoming request to modify a model (via a controller, + # service, or GraphQL mutation). + # + # Deletes the parameters which are related to spam and captcha processing, and returns + # them in a SpamParams parameters object. See: + # https://refactoring.com/catalog/introduceParameterObject.html + def self.filter_spam_params!(params) + # NOTE: The 'captcha_response' field can be expanded to multiple fields when we move to future + # alternative captcha implementations such as FriendlyCaptcha. See + # https://gitlab.com/gitlab-org/gitlab/-/issues/273480 + captcha_response = params.delete(:captcha_response) + + SpamParams.new( + api: params.delete(:api), + captcha_response: captcha_response, + spam_log_id: params.delete(:spam_log_id) + ) + end + attr_accessor :target, :request, :options attr_reader :spam_log - def initialize(spammable:, request:, user:, context: {}) + def initialize(spammable:, request:, user:, action:) @target = spammable @request = request @user = user - @context = context + @action = action @options = {} - - if @request - @options[:ip_address] = @request.env['action_dispatch.remote_ip'].to_s - @options[:user_agent] = @request.env['HTTP_USER_AGENT'] - @options[:referrer] = @request.env['HTTP_REFERRER'] - else - @options[:ip_address] = @target.ip_address - @options[:user_agent] = @target.user_agent - end end - def execute(api: false, recaptcha_verified:, spam_log_id:) - if recaptcha_verified - # If it's a request which is already verified through reCAPTCHA, - # update the spam log accordingly. - SpamLog.verify_recaptcha!(user_id: user.id, id: spam_log_id) + def execute(spam_params:) + if request + options[:ip_address] = request.env['action_dispatch.remote_ip'].to_s + options[:user_agent] = request.env['HTTP_USER_AGENT'] + options[:referrer] = request.env['HTTP_REFERRER'] else - return if allowlisted?(user) - return unless request - return unless check_for_spam? + # TODO: This code is never used, because we do not perform a verification if there is not a + # request. Why? Should it be deleted? Or should we check even if there is no request? + options[:ip_address] = target.ip_address + options[:user_agent] = target.user_agent + end - perform_spam_service_check(api) + recaptcha_verified = Captcha::CaptchaVerificationService.new.execute( + captcha_response: spam_params.captcha_response, + request: request + ) + + if recaptcha_verified + # If it's a request which is already verified through captcha, + # update the spam log accordingly. + SpamLog.verify_recaptcha!(user_id: user.id, id: spam_params.spam_log_id) + ServiceResponse.success(message: "Captcha was successfully verified") + else + return ServiceResponse.success(message: 'Skipped spam check because user was allowlisted') if allowlisted?(user) + return ServiceResponse.success(message: 'Skipped spam check because request was not present') unless request + return ServiceResponse.success(message: 'Skipped spam check because it was not required') unless check_for_spam? + + perform_spam_service_check(spam_params.api) + ServiceResponse.success(message: "Spam check performed, check #{target.class.name} spammable model for any errors or captcha requirement") end end @@ -42,13 +74,27 @@ module Spam private - attr_reader :user, :context + attr_reader :user, :action + + ## + # In order to be proceed to the spam check process, the target must be + # a dirty instance, which means it should be already assigned with the new + # attribute values. + def ensure_target_is_dirty + msg = "Target instance of #{target.class.name} must be dirty (must have changes to save)" + raise(msg) unless target.has_changes_to_save? + end def allowlisted?(user) user.try(:gitlab_employee?) || user.try(:gitlab_bot?) || user.try(:gitlab_service_user?) end + ## + # Performs the spam check using the spam verdict service, and modifies the target model + # accordingly based on the result. def perform_spam_service_check(api) + ensure_target_is_dirty + # since we can check for spam, and recaptcha is not verified, # ask the SpamVerdictService what to do with the target. spam_verdict_service.execute.tap do |result| @@ -79,7 +125,7 @@ module Spam description: target.spam_description, source_ip: options[:ip_address], user_agent: options[:user_agent], - noteable_type: notable_type, + noteable_type: noteable_type, via_api: api } ) @@ -88,14 +134,19 @@ module Spam end def spam_verdict_service + context = { + action: action, + target_type: noteable_type + } + SpamVerdictService.new(target: target, user: user, - request: @request, + request: request, options: options, - context: context.merge(target_type: notable_type)) + context: context) end - def notable_type + def noteable_type @notable_type ||= target.class.to_s end end diff --git a/app/services/spam/spam_params.rb b/app/services/spam/spam_params.rb new file mode 100644 index 00000000000..fef5355c7f3 --- /dev/null +++ b/app/services/spam/spam_params.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Spam + ## + # This class is a Parameter Object (https://refactoring.com/catalog/introduceParameterObject.html) + # which acts as an container abstraction for multiple parameter values related to spam and + # captcha processing for a request. + # + # Values contained are: + # + # api: A boolean flag indicating if the request was submitted via the REST or GraphQL API + # captcha_response: The response resulting from the user solving a captcha. Currently it is + # a scalar reCAPTCHA response string, but it can be expanded to an object in the future to + # support other captcha implementations such as FriendlyCaptcha. + # spam_log_id: The id of a SpamLog record. + class SpamParams + attr_reader :api, :captcha_response, :spam_log_id + + def initialize(api:, captcha_response:, spam_log_id:) + @api = api.present? + @captcha_response = captcha_response + @spam_log_id = spam_log_id + end + + def ==(other) + other.class == self.class && + other.api == self.api && + other.captcha_response == self.captcha_response && + other.spam_log_id == self.spam_log_id + end + end +end diff --git a/app/views/admin/application_settings/_ci_cd.html.haml b/app/views/admin/application_settings/_ci_cd.html.haml index 9f384519c3a..351c13d5ad1 100644 --- a/app/views/admin/application_settings/_ci_cd.html.haml +++ b/app/views/admin/application_settings/_ci_cd.html.haml @@ -58,6 +58,6 @@ = f.text_field :default_ci_config_path, class: 'form-control', placeholder: '.gitlab-ci.yml' %p.form-text.text-muted = _("The default CI configuration path for new projects.").html_safe - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'custom-ci-configuration-path'), target: '_blank' + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'custom-cicd-configuration-path'), target: '_blank' = f.submit _('Save changes'), class: "gl-button btn btn-success" diff --git a/app/views/groups/settings/ci_cd/_form.html.haml b/app/views/groups/settings/ci_cd/_form.html.haml index 8fad73f1249..635e3b64e39 100644 --- a/app/views/groups/settings/ci_cd/_form.html.haml +++ b/app/views/groups/settings/ci_cd/_form.html.haml @@ -4,10 +4,10 @@ = form_errors(group) %fieldset.builds-feature .form-group - = f.label :max_artifacts_size, _('Maximum artifacts size (MB)'), class: 'label-bold' + = f.label :max_artifacts_size, _('Maximum artifacts size'), class: 'label-bold' = f.number_field :max_artifacts_size, class: 'form-control' %p.form-text.text-muted - = _("Set the maximum file size for each job's artifacts") + = _("The maximum file size in megabytes for individual job artifacts.") = link_to sprite_icon('question-o'), help_page_path('user/admin_area/settings/continuous_integration', anchor: 'maximum-artifacts-size'), target: '_blank' = f.submit _('Save changes'), class: "btn btn-success" diff --git a/app/views/projects/settings/ci_cd/_form.html.haml b/app/views/projects/settings/ci_cd/_form.html.haml index d247e73a5b4..5e52eb1023c 100644 --- a/app/views/projects/settings/ci_cd/_form.html.haml +++ b/app/views/projects/settings/ci_cd/_form.html.haml @@ -3,90 +3,23 @@ = form_for @project, url: project_settings_ci_cd_path(@project, anchor: 'js-general-pipeline-settings') do |f| = form_errors(@project) %fieldset.builds-feature - .form-group - %h5.gl-mt-0 - = _("Git strategy for pipelines") - %p - = html_escape(_("Choose between %{code_open}clone%{code_close} or %{code_open}fetch%{code_close} to get the recent application code")) % { code_open: ''.html_safe, code_close: ''.html_safe } - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'git-strategy'), target: '_blank' - .form-check - = f.radio_button :build_allow_git_fetch, 'false', { class: 'form-check-input' } - = f.label :build_allow_git_fetch_false, class: 'form-check-label' do - %strong git clone - %br - %span - = _("Slower but makes sure the project workspace is pristine as it clones the repository from scratch for every job") - .form-check - = f.radio_button :build_allow_git_fetch, 'true', { class: 'form-check-input' } - = f.label :build_allow_git_fetch_true, class: 'form-check-label' do - %strong git fetch - %br - %span - = _("Faster as it re-uses the project workspace (falling back to clone if it doesn't exist)") - - %hr - .form-group - = f.fields_for :ci_cd_settings_attributes, @project.ci_cd_settings do |form| - = form.label :default_git_depth, _('Git shallow clone'), class: 'label-bold' - = form.number_field :default_git_depth, { class: 'form-control', min: 0, max: 1000 } - %p.form-text.text-muted - = _('The number of changes to be fetched from GitLab when cloning a repository. This can speed up Pipelines execution. Keep empty or set to 0 to disable shallow clone by default and make GitLab CI fetch all branches and tags each time.') - - %hr - .form-group - = f.label :build_timeout_human_readable, _('Timeout'), class: 'label-bold' - = f.text_field :build_timeout_human_readable, class: 'form-control' - %p.form-text.text-muted - = _('If any job surpasses this timeout threshold, it will be marked as failed. Human readable time input language is accepted like "1 hour". Values without specification represent seconds.') - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'timeout'), target: '_blank' - - - if can?(current_user, :update_max_artifacts_size, @project) - %hr - .form-group - = f.label :max_artifacts_size, _('Maximum artifacts size (MB)'), class: 'label-bold' - = f.number_field :max_artifacts_size, class: 'form-control' - %p.form-text.text-muted - = _("Set the maximum file size for each job's artifacts") - = link_to sprite_icon('question-o'), help_page_path('user/admin_area/settings/continuous_integration', anchor: 'maximum-artifacts-size'), target: '_blank' - - %hr - .form-group - = f.label :ci_config_path, _('Custom CI configuration path'), class: 'label-bold' - = f.text_field :ci_config_path, class: 'form-control', placeholder: '.gitlab-ci.yml' - %p.form-text.text-muted - = html_escape(_("The path to the CI configuration file. Defaults to %{code_open}.gitlab-ci.yml%{code_close}")) % { code_open: ''.html_safe, code_close: ''.html_safe } - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'custom-ci-configuration-path'), target: '_blank' - - %hr .form-group .form-check = f.check_box :public_builds, { class: 'form-check-input' } = f.label :public_builds, class: 'form-check-label' do %strong= _("Public pipelines") .form-text.text-muted - = _("Allow public access to pipelines and job details, including output logs and artifacts") + = _("Allow public access to pipelines and job details, including output logs and artifacts.") = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'visibility-of-pipelines'), target: '_blank' - .bs-callout.bs-callout-info - %p #{_("If enabled")}: - %ul - %li - = _("For public projects, anyone can view pipelines and access job details (output logs and artifacts)") - %li - = _("For internal projects, any logged in user except external users can view pipelines and access job details (output logs and artifacts)") - %li - = _("For private projects, any member (guest or higher) can view pipelines and access job details (output logs and artifacts)") - %p - = _("If disabled, the access level will depend on the user's permissions in the project.") - %hr .form-group .form-check = f.check_box :auto_cancel_pending_pipelines, { class: 'form-check-input' }, 'enabled', 'disabled' = f.label :auto_cancel_pending_pipelines, class: 'form-check-label' do - %strong= _("Auto-cancel redundant, pending pipelines") + %strong= _("Auto-cancel redundant pipelines") .form-text.text-muted - = _("New pipelines will cancel older, pending pipelines on the same branch") - = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'auto-cancel-pending-pipelines'), target: '_blank' + = _("New pipelines cause older pending pipelines on the same branch to be cancelled.") + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'auto-cancel-redundant-pipelines'), target: '_blank' .form-group .form-check @@ -95,10 +28,62 @@ = form.label :forward_deployment_enabled, class: 'form-check-label' do %strong= _("Skip outdated deployment jobs") .form-text.text-muted - = _("When a deployment job is successful, skip older deployment jobs that are still pending") + = _("When a deployment job is successful, skip older deployment jobs that are still pending.") = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'skip-outdated-deployment-jobs'), target: '_blank' + .form-group + = f.label :ci_config_path, _('CI/CD configuration file'), class: 'label-bold' + = f.text_field :ci_config_path, class: 'form-control', placeholder: '.gitlab-ci.yml' + %p.form-text.text-muted + = html_escape(_("The name of the CI/CD configuration file. A path relative to the root directory is optional (for example %{code_open}my/path/.myfile.yml%{code_close}).")) % { code_open: ''.html_safe, code_close: ''.html_safe } + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'custom-cicd-configuration-path'), target: '_blank' + %hr + .form-group + %h5.gl-mt-0 + = _("Git strategy") + %p + = _("Choose which Git strategy to use when fetching the project.") + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'git-strategy'), target: '_blank' + .form-check + = f.radio_button :build_allow_git_fetch, 'false', { class: 'form-check-input' } + = f.label :build_allow_git_fetch_false, class: 'form-check-label' do + %strong git clone + %br + %span + = _("For each job, clone the repository.") + .form-check + = f.radio_button :build_allow_git_fetch, 'true', { class: 'form-check-input' } + = f.label :build_allow_git_fetch_true, class: 'form-check-label' do + %strong git fetch + %br + %span + = html_escape(_("For each job, re-use the project workspace. If the workspace doesn't exist, use %{code_open}git clone%{code_close}.")) % { code_open: ''.html_safe, code_close: ''.html_safe } + + .form-group + = f.fields_for :ci_cd_settings_attributes, @project.ci_cd_settings do |form| + = form.label :default_git_depth, _('Git shallow clone'), class: 'label-bold' + = form.number_field :default_git_depth, { class: 'form-control', min: 0, max: 1000 } + %p.form-text.text-muted + = html_escape(_('The number of changes to fetch from GitLab when cloning a repository. Lower values can speed up pipeline execution. Set to %{code_open}0%{code_close} or blank to fetch all branches and tags for each job')) % { code_open: ''.html_safe, code_close: ''.html_safe } + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'git-shallow-clone'), target: '_blank' + + %hr + .form-group + = f.label :build_timeout_human_readable, _('Timeout'), class: 'label-bold' + = f.text_field :build_timeout_human_readable, class: 'form-control' + %p.form-text.text-muted + = html_escape(_('Jobs fail if they run longer than the timeout time. Input value is in seconds by default. Human readable input is also accepted, for example %{code_open}1 hour%{code_close}.')) % { code_open: ''.html_safe, code_close: ''.html_safe } + = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'timeout'), target: '_blank' + + - if can?(current_user, :update_max_artifacts_size, @project) + .form-group + = f.label :max_artifacts_size, _('Maximum artifacts size'), class: 'label-bold' + = f.number_field :max_artifacts_size, class: 'form-control' + %p.form-text.text-muted + = _("The maximum file size in megabytes for individual job artifacts.") + = link_to sprite_icon('question-o'), help_page_path('user/admin_area/settings/continuous_integration', anchor: 'maximum-artifacts-size'), target: '_blank' + .form-group = f.label :build_coverage_regex, _("Test coverage parsing"), class: 'label-bold' .input-group @@ -108,44 +93,8 @@ %span.input-group-append .input-group-text / %p.form-text.text-muted - = _("A regular expression that will be used to find the test coverage output in the job log. Leave blank to disable") + = html_escape(_('The regular expression used to find test coverage output in the job log. For example, use %{regex} for Simplecov (Ruby). Leave blank to disable.')) % { regex: '\(\d+.\d+%\)'.html_safe } = link_to sprite_icon('question-o'), help_page_path('ci/pipelines/settings', anchor: 'test-coverage-parsing'), target: '_blank' - .bs-callout.bs-callout-info - %p= _("Below are examples of regex for existing tools:") - %ul - %li - Simplecov (Ruby) - - %code \(\d+.\d+\%\) covered - %li - pytest-cov (Python) - - %code ^TOTAL.+?(\d+\%)$ - %li - Scoverage (Scala) - - %code Statement coverage[A-Za-z\.*]\s*:\s*([^%]+) - %li - phpunit --coverage-text --colors=never (PHP) - - %code ^\s*Lines:\s*\d+.\d+\% - %li - gcovr (C/C++) - - %code ^TOTAL.*\s+(\d+\%)$ - %li - tap --coverage-report=text-summary (NodeJS) - - %code ^Statements\s*:\s*([^%]+) - %li - nyc npm test (NodeJS) - - %code All files[^|]*\|[^|]*\s+([\d\.]+) - %li - excoveralls (Elixir) - - %code \[TOTAL\]\s+(\d+\.\d+)% - %li - mix test --cover (Elixir) - - %code \d+.\d+\%\s+\|\s+Total - %li - JaCoCo (Java/Kotlin) - %code Total.*?([0-9]{1,3})% - %li - go test -cover (Go) - %code coverage: \d+.\d+% of statements = f.submit _('Save changes'), class: "btn btn-success", data: { qa_selector: 'save_general_pipelines_changes_button' } diff --git a/app/views/projects/settings/ci_cd/show.html.haml b/app/views/projects/settings/ci_cd/show.html.haml index 157a87b4868..f7053561dfa 100644 --- a/app/views/projects/settings/ci_cd/show.html.haml +++ b/app/views/projects/settings/ci_cd/show.html.haml @@ -12,7 +12,7 @@ %button.btn.js-settings-toggle{ type: 'button' } = expanded ? _('Collapse') : _('Expand') %p - = _("Customize your pipeline configuration, view your pipeline status and coverage report.") + = _("Customize your pipeline configuration and coverage report.") .settings-content = render 'form' diff --git a/changelogs/unreleased/298822-ci-cd-general-pipelines-settings-ui-text.yml b/changelogs/unreleased/298822-ci-cd-general-pipelines-settings-ui-text.yml new file mode 100644 index 00000000000..279be572394 --- /dev/null +++ b/changelogs/unreleased/298822-ci-cd-general-pipelines-settings-ui-text.yml @@ -0,0 +1,5 @@ +--- +title: Update CI general pipeline settings UI text +merge_request: 51806 +author: +type: other diff --git a/changelogs/unreleased/fix-pipeline-and-stage-status.yml b/changelogs/unreleased/fix-pipeline-and-stage-status.yml new file mode 100644 index 00000000000..6a3f4e7e31d --- /dev/null +++ b/changelogs/unreleased/fix-pipeline-and-stage-status.yml @@ -0,0 +1,5 @@ +--- +title: Fix pipeline and stage show success without considering bridge status +merge_request: 52192 +author: Cong Chen @gentcys +type: fixed diff --git a/config/feature_flags/development/optimized_merge_request_count_with_merged_at_filter.yml b/config/feature_flags/development/optimized_merge_request_count_with_merged_at_filter.yml new file mode 100644 index 00000000000..f27e14aab6c --- /dev/null +++ b/config/feature_flags/development/optimized_merge_request_count_with_merged_at_filter.yml @@ -0,0 +1,8 @@ +--- +name: optimized_merge_request_count_with_merged_at_filter +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52113 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/299347 +milestone: '13.9' +type: development +group: group::optimize +default_enabled: false diff --git a/doc/administration/compliance.md b/doc/administration/compliance.md index 6079e838ac6..09fe447fd53 100644 --- a/doc/administration/compliance.md +++ b/doc/administration/compliance.md @@ -26,4 +26,4 @@ relevant compliance standards. |**[Audit events](audit_events.md)**
To maintain the integrity of your code, GitLab Enterprise Edition Premium gives admins the ability to view any modifications made within the GitLab server in an advanced audit events system, so you can control, analyze, and track every change.|Premium+|✓|Instance, Group, Project| |**[Auditor users](auditor_users.md)**
Auditor users are users who are given read-only access to all projects, groups, and other resources on the GitLab instance.|Premium+||Instance| |**[Credentials inventory](../user/admin_area/credentials_inventory.md)**
With a credentials inventory, GitLab administrators can keep track of the credentials used by all of the users in their GitLab instance. |Ultimate||Instance| -|**Separation of Duties using [Protected branches](../user/project/protected_branches.md#protected-branches-approval-by-code-owners) and [custom CI Configuration Paths](../ci/pipelines/settings.md#custom-ci-configuration-path)**
GitLab Silver and Premium users can leverage the GitLab cross-project YAML configurations to define deployers of code and developers of code. View the [Separation of Duties Deploy Project](https://gitlab.com/guided-explorations/separation-of-duties-deploy/blob/master/README.md) and [Separation of Duties Project](https://gitlab.com/guided-explorations/separation-of-duties/blob/master/README.md) to see how to use this set up to define these roles.|Premium+|✓|Project| +|**Separation of Duties using [Protected branches](../user/project/protected_branches.md#protected-branches-approval-by-code-owners) and [custom CI Configuration Paths](../ci/pipelines/settings.md#custom-cicd-configuration-path)**
GitLab Silver and Premium users can leverage the GitLab cross-project YAML configurations to define deployers of code and developers of code. View the [Separation of Duties Deploy Project](https://gitlab.com/guided-explorations/separation-of-duties-deploy/blob/master/README.md) and [Separation of Duties Project](https://gitlab.com/guided-explorations/separation-of-duties/blob/master/README.md) to see how to use this set up to define these roles.|Premium+|✓|Project| diff --git a/doc/ci/README.md b/doc/ci/README.md index 1a3b685eac6..3aca4e1d63d 100644 --- a/doc/ci/README.md +++ b/doc/ci/README.md @@ -105,7 +105,7 @@ GitLab CI/CD supports numerous configuration options: | Configuration | Description | |:----------------------------------------------------------------------------------------|:------------------------------------------------------------------------------------------| | [Schedule pipelines](pipelines/schedules.md) | Schedule pipelines to run as often as you need. | -| [Custom path for `.gitlab-ci.yml`](pipelines/settings.md#custom-ci-configuration-path) | Define a custom path for the CI/CD configuration file. | +| [Custom path for `.gitlab-ci.yml`](pipelines/settings.md#custom-cicd-configuration-path) | Define a custom path for the CI/CD configuration file. | | [Git submodules for CI/CD](git_submodules.md) | Configure jobs for using Git submodules. | | [SSH keys for CI/CD](ssh_keys/README.md) | Using SSH keys in your CI pipelines. | | [Pipeline triggers](triggers/README.md) | Trigger pipelines through the API. | diff --git a/doc/ci/environments/deployment_safety.md b/doc/ci/environments/deployment_safety.md index 4dac076ffb7..9ae79ef40e8 100644 --- a/doc/ci/environments/deployment_safety.md +++ b/doc/ci/environments/deployment_safety.md @@ -141,7 +141,7 @@ reference a file in another project with a completely different set of permissio In this scenario, the `gitlab-ci.yml` is publicly accessible, but can only be edited by users with appropriate permissions in the other project. -For more information, see [Custom CI configuration path](../pipelines/settings.md#custom-ci-configuration-path). +For more information, see [Custom CI/CD configuration path](../pipelines/settings.md#custom-cicd-configuration-path). ## Troubleshooting diff --git a/doc/ci/pipelines/settings.md b/doc/ci/pipelines/settings.md index 5a758bccd62..36eb343a5db 100644 --- a/doc/ci/pipelines/settings.md +++ b/doc/ci/pipelines/settings.md @@ -68,7 +68,7 @@ Project defined timeout (either specific timeout set by user or the default For information about setting a maximum artifact size for a project, see [Maximum artifacts size](../../user/admin_area/settings/continuous_integration.md#maximum-artifacts-size). -## Custom CI configuration path +## Custom CI/CD configuration path > - [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/12509) in GitLab 9.4. > - [Support for external `.gitlab-ci.yml` locations](https://gitlab.com/gitlab-org/gitlab/-/issues/14376) introduced in GitLab 12.6. @@ -80,7 +80,7 @@ To customize the path: 1. Go to the project's **Settings > CI / CD**. 1. Expand the **General pipelines** section. -1. Provide a value in the **Custom CI configuration path** field. +1. Provide a value in the **CI/CD configuration file** field. 1. Click **Save changes**. If the CI configuration is stored within the repository in a non-default @@ -131,8 +131,19 @@ averaged. ![Build status coverage](img/pipelines_test_coverage_build.png) -A few examples of known coverage tools for a variety of languages can be found -in the pipelines settings page. +| Coverage Tool | Sample regular expression | +|------------------------------------------------|---------------------------| +| Simplecov (Ruby) | `\(\d+.\d+\%\) covered` | +| pytest-cov (Python) | `^TOTAL.+?(\d+\%)$` | +| Scoverage (Scala) | `Statement coverage[A-Za-z\.*]\s*:\s*([^%]+)` | +| `phpunit --coverage-text --colors=never` (PHP) | `^\s*Lines:\s*\d+.\d+\%` | +| gcovr (C/C++) | `^TOTAL.*\s+(\d+\%)$` | +| `tap --coverage-report=text-summary` (NodeJS) | `^Statements\s*:\s*([^%]+)` | +| `nyc npm test` (NodeJS) | `All files[^|]*\|[^|]*\s+([\d\.]+)` | +| excoveralls (Elixir) | `\[TOTAL\]\s+(\d+\.\d+)%` | +| `mix test --cover` (Elixir) | `\d+.\d+\%\s+\|\s+Total` | +| JaCoCo (Java/Kotlin) | `Total.*?([0-9]{1,3})%` | +| `go test -cover` (Go) | `coverage: \d+.\d+% of statements` | ### Code Coverage history @@ -198,7 +209,7 @@ If **Public pipelines** is disabled: - For **private** projects, only project members (reporter or higher) can view the pipelines or access the related features. -## Auto-cancel pending pipelines +## Auto-cancel redundant pipelines > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/9362) in GitLab 9.1. @@ -206,7 +217,7 @@ You can set pending or running pipelines to cancel automatically when a new pipe 1. Go to **Settings > CI / CD**. 1. Expand **General Pipelines**. -1. Check the **Auto-cancel redundant, pending pipelines** checkbox. +1. Check the **Auto-cancel redundant pipelines** checkbox. 1. Click **Save changes**. Use the [`interruptible`](../yaml/README.md#interruptible) keyword to indicate if a diff --git a/doc/ci/unit_test_reports.md b/doc/ci/unit_test_reports.md index 1fec1f77bc3..58308f71217 100644 --- a/doc/ci/unit_test_reports.md +++ b/doc/ci/unit_test_reports.md @@ -290,6 +290,22 @@ javascript: - junit.xml ``` +### Flutter / Dart example + +This example `.gitlab-ci.yml` file uses the [JUnit Report](https://pub.dev/packages/junitreport) package to convert the `flutter test` output into JUnit report XML format: + +```yaml +test: + stage: test + script: + - flutter test --machine | tojunit -o report.xml + artifacts: + when: always + reports: + junit: + - report.xml +``` + ## Viewing Unit test reports on GitLab > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/24792) in GitLab 12.5 behind a feature flag (`junit_pipeline_view`), disabled by default. diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index 3928bca4c60..37335bb7781 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -618,7 +618,7 @@ variables, an `Insufficient permissions to set pipeline variables` error occurs. The setting is `disabled` by default. -If you [store your CI configurations in a different repository](../../ci/pipelines/settings.md#custom-ci-configuration-path), +If you [store your CI configurations in a different repository](../../ci/pipelines/settings.md#custom-cicd-configuration-path), use this setting for strict control over all aspects of the environment the pipeline runs in. diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 61280a15585..56634ade3f4 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -3845,7 +3845,7 @@ The trigger token is different than the [`trigger`](#trigger) keyword. Use `interruptible` to indicate that a running job should be canceled if made redundant by a newer pipeline run. Defaults to `false` (uninterruptible). Jobs that have not started yet (pending) are considered interruptible and safe to be cancelled. -This value is used only if the [automatic cancellation of redundant pipelines feature](../pipelines/settings.md#auto-cancel-pending-pipelines) +This value is used only if the [automatic cancellation of redundant pipelines feature](../pipelines/settings.md#auto-cancel-redundant-pipelines) is enabled. When enabled, a pipeline is immediately canceled when a new pipeline starts on the same branch if either of the following is true: diff --git a/doc/development/module_with_instance_variables.md b/doc/development/module_with_instance_variables.md index 75575105178..16c7a807053 100644 --- a/doc/development/module_with_instance_variables.md +++ b/doc/development/module_with_instance_variables.md @@ -121,71 +121,7 @@ module Gitlab end ``` -Now the cop doesn't complain. Here's a bad example which we could rewrite: - -``` ruby -module SpamCheckService - def filter_spam_check_params - @request = params.delete(:request) - @api = params.delete(:api) - @recaptcha_verified = params.delete(:recaptcha_verified) - @spam_log_id = params.delete(:spam_log_id) - end - - def spam_check(spammable, user) - spam_service = SpamService.new(spammable, @request) - - spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do - user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true) - end - end -end -``` - -There are several implicit dependencies here. First, `params` should be -defined before use. Second, `filter_spam_check_params` should be called -before `spam_check`. These are all implicit and the includer could be using -those instance variables without awareness. - -This should be rewritten like: - -``` ruby -class SpamCheckService - def initialize(request:, api:, recaptcha_verified:, spam_log_id:) - @request = request - @api = api - @recaptcha_verified = recaptcha_verified - @spam_log_id = spam_log_id - end - - def spam_check(spammable, user) - spam_service = SpamService.new(spammable, @request) - - spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do - user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true) - end - end -end -``` - -And use it like: - -``` ruby -class UpdateSnippetService < BaseService - def execute - # ... - spam = SpamCheckService.new(params.slice!(:request, :api, :recaptcha_verified, :spam_log_id)) - - spam.check(snippet, current_user) - # ... - end -end -``` - -This way, all those instance variables are isolated in `SpamCheckService` -rather than whatever includes the module, and those modules which were also -included, making it much easier to track down any issues, -and reducing the chance of having name conflicts. +Now the cop doesn't complain. ## How to disable this cop diff --git a/doc/user/admin_area/settings/continuous_integration.md b/doc/user/admin_area/settings/continuous_integration.md index 6418be13ee9..5f65277813d 100644 --- a/doc/user/admin_area/settings/continuous_integration.md +++ b/doc/user/admin_area/settings/continuous_integration.md @@ -156,7 +156,7 @@ Area of your GitLab instance (`.gitlab-ci.yml` if not set): 1. Input the new path in the **Default CI configuration path** field. 1. Hit **Save changes** for the changes to take effect. -It is also possible to specify a [custom CI configuration path for a specific project](../../../ci/pipelines/settings.md#custom-ci-configuration-path). +It is also possible to specify a [custom CI/CD configuration path for a specific project](../../../ci/pipelines/settings.md#custom-cicd-configuration-path).