From eb8607408da45fd1174e9f42579f433e61bdd260 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Mon, 21 Feb 2022 00:18:15 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/controllers/concerns/issuable_actions.rb | 10 +-- .../akismet_mark_as_spam_action.rb | 11 ++- .../concerns/spammable_actions/attributes.rb | 13 --- .../spammable_actions/captcha_check/common.rb | 32 +++---- .../html_format_actions_support.rb | 5 +- .../json_format_actions_support.rb | 5 +- app/controllers/projects/issues_controller.rb | 2 +- .../concerns/mutations/spam_protection.rb | 26 ++---- app/models/concerns/spammable.rb | 6 +- app/models/project.rb | 4 +- app/models/projects/topic.rb | 3 +- app/services/groups/destroy_service.rb | 4 + ...cate-based-integration-with-kubernetes.yml | 23 ++--- ...20203074916_add_topics_lower_name_index.rb | 15 ++++ db/schema_migrations/20220203074916 | 1 + db/structure.sql | 2 + doc/update/deprecations.md | 23 ++--- .../project/settings/project_access_tokens.md | 5 +- .../has_spam_action_response_fields.rb | 2 +- qa/Gemfile | 2 +- qa/Gemfile.lock | 10 +-- qa/qa/runtime/allure_report.rb | 2 +- .../formatters/allure_metadata_formatter.rb | 87 ++++++++++++++++++- .../formatters/test_stats_formatter.rb | 79 ++--------------- qa/qa/support/influxdb_tools.rb | 81 +++++++++++++++++ qa/qa/tools/reliable_report.rb | 28 +----- qa/spec/specs/allure_report_spec.rb | 5 +- .../allure_metadata_formatter_spec.rb | 3 +- .../formatters/test_stats_formatter_spec.rb | 3 +- .../admin/topics_controller_spec.rb | 16 ++++ .../akismet_mark_as_spam_action_spec.rb | 22 +++-- .../html_format_actions_support_spec.rb | 2 +- .../json_format_actions_support_spec.rb | 2 +- spec/models/project_spec.rb | 8 ++ spec/models/projects/topic_spec.rb | 2 +- spec/requests/api/topics_spec.rb | 7 ++ spec/services/groups/destroy_service_spec.rb | 11 +++ 37 files changed, 355 insertions(+), 207 deletions(-) delete mode 100644 app/controllers/concerns/spammable_actions/attributes.rb create mode 100644 db/migrate/20220203074916_add_topics_lower_name_index.rb create mode 100644 db/schema_migrations/20220203074916 create mode 100644 qa/qa/support/influxdb_tools.rb diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index eae087bca4d..5d4e93363fa 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -43,18 +43,18 @@ module IssuableActions if updated_issuable.is_a?(Spammable) respond_to do |format| format.html do - # NOTE: This redirect is intentionally only performed in the case where the updated - # issuable is a spammable, and intentionally is not performed in the non-spammable case. - # This preserves the legacy behavior of this action. if updated_issuable.valid? + # NOTE: This redirect is intentionally only performed in the case where the valid updated + # issuable is a spammable, and intentionally is not performed below in the + # valid non-spammable case. This preserves the legacy behavior of this action. redirect_to spammable_path else - with_captcha_check_html_format { render :edit } + with_captcha_check_html_format(spammable: spammable) { render :edit } end end format.json do - with_captcha_check_json_format { render_entity_json } + with_captcha_check_json_format(spammable: spammable) { render_entity_json } end end else diff --git a/app/controllers/concerns/spammable_actions/akismet_mark_as_spam_action.rb b/app/controllers/concerns/spammable_actions/akismet_mark_as_spam_action.rb index 234c591ffb7..044519004b2 100644 --- a/app/controllers/concerns/spammable_actions/akismet_mark_as_spam_action.rb +++ b/app/controllers/concerns/spammable_actions/akismet_mark_as_spam_action.rb @@ -2,7 +2,6 @@ module SpammableActions::AkismetMarkAsSpamAction extend ActiveSupport::Concern - include SpammableActions::Attributes included do before_action :authorize_submit_spammable!, only: :mark_as_spam @@ -22,7 +21,15 @@ module SpammableActions::AkismetMarkAsSpamAction access_denied! unless current_user.can_admin_all_resources? end + def spammable + # The class extending this module should define the #spammable method to return + # the Spammable model instance via: `alias_method :spammable , <:model_name>` + raise NotImplementedError, "#{self.class} should implement #{__method__}" + end + def spammable_path - raise NotImplementedError, "#{self.class} does not implement #{__method__}" + # The class extending this module should define the #spammable_path method to return + # the route helper pointing to the action to show the Spammable instance + raise NotImplementedError, "#{self.class} should implement #{__method__}" end end diff --git a/app/controllers/concerns/spammable_actions/attributes.rb b/app/controllers/concerns/spammable_actions/attributes.rb deleted file mode 100644 index d7060e47c07..00000000000 --- a/app/controllers/concerns/spammable_actions/attributes.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module SpammableActions - module Attributes - extend ActiveSupport::Concern - - private - - def spammable - raise NotImplementedError, "#{self.class} does not implement #{__method__}" - end - end -end diff --git a/app/controllers/concerns/spammable_actions/captcha_check/common.rb b/app/controllers/concerns/spammable_actions/captcha_check/common.rb index 7c047e02a1d..aaeb6b3ba83 100644 --- a/app/controllers/concerns/spammable_actions/captcha_check/common.rb +++ b/app/controllers/concerns/spammable_actions/captcha_check/common.rb @@ -1,23 +1,25 @@ # frozen_string_literal: true -module SpammableActions::CaptchaCheck - module Common - extend ActiveSupport::Concern +module SpammableActions + module CaptchaCheck + module Common + extend ActiveSupport::Concern - private + private - def with_captcha_check_common(captcha_render_lambda:, &block) - # If the Spammable indicates that CAPTCHA is not necessary (either due to it not being flagged - # as spam, or if spam/captcha is disabled for some reason), then we will go ahead and - # yield to the block containing the action's original behavior, then return. - return yield unless spammable.render_recaptcha? + def with_captcha_check_common(spammable:, captcha_render_lambda:, &block) + # If the Spammable indicates that CAPTCHA is not necessary (either due to it not being flagged + # as spam, or if spam/captcha is disabled for some reason), then we will go ahead and + # yield to the block containing the action's original behavior, then return. + return yield unless spammable.render_recaptcha? - # If we got here, we need to render the CAPTCHA instead of yielding to action's original - # behavior. We will present a CAPTCHA to be solved by executing the lambda which was passed - # as the `captcha_render_lambda:` argument. This lambda contains either the HTML-specific or - # JSON-specific behavior to cause the CAPTCHA modal to be rendered. - Gitlab::Recaptcha.load_configurations! - captcha_render_lambda.call + # If we got here, we need to render the CAPTCHA instead of yielding to action's original + # behavior. We will present a CAPTCHA to be solved by executing the lambda which was passed + # as the `captcha_render_lambda:` argument. This lambda contains either the HTML-specific or + # JSON-specific behavior to cause the CAPTCHA modal to be rendered. + Gitlab::Recaptcha.load_configurations! + captcha_render_lambda.call + end end end end diff --git a/app/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support.rb b/app/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support.rb index f687c0fcf2d..b254916cdd6 100644 --- a/app/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support.rb +++ b/app/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support.rb @@ -8,7 +8,6 @@ # which supports JSON format should be used instead. module SpammableActions::CaptchaCheck::HtmlFormatActionsSupport extend ActiveSupport::Concern - include SpammableActions::Attributes include SpammableActions::CaptchaCheck::Common included do @@ -17,9 +16,9 @@ module SpammableActions::CaptchaCheck::HtmlFormatActionsSupport private - def with_captcha_check_html_format(&block) + def with_captcha_check_html_format(spammable:, &block) captcha_render_lambda = -> { render :captcha_check } - with_captcha_check_common(captcha_render_lambda: captcha_render_lambda, &block) + with_captcha_check_common(spammable: spammable, captcha_render_lambda: captcha_render_lambda, &block) end # Convert spam/CAPTCHA values from form field params to headers, because all spam-related services diff --git a/app/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support.rb b/app/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support.rb index 0bfea05abc7..9c72efe53bb 100644 --- a/app/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support.rb +++ b/app/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support.rb @@ -9,17 +9,16 @@ # supports HTML format should be used instead. module SpammableActions::CaptchaCheck::JsonFormatActionsSupport extend ActiveSupport::Concern - include SpammableActions::Attributes include SpammableActions::CaptchaCheck::Common include Spam::Concerns::HasSpamActionResponseFields private - def with_captcha_check_json_format(&block) + def with_captcha_check_json_format(spammable:, &block) # NOTE: "409 - Conflict" seems to be the most appropriate HTTP status code for a response # which requires a CAPTCHA to be solved in order for the request to be resubmitted. # https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.10 captcha_render_lambda = -> { render json: spam_action_response_fields(spammable), status: :conflict } - with_captcha_check_common(captcha_render_lambda: captcha_render_lambda, &block) + with_captcha_check_common(spammable: spammable, captcha_render_lambda: captcha_render_lambda, &block) end end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 1b98810b09b..a493016c268 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -150,7 +150,7 @@ class Projects::IssuesController < Projects::ApplicationController redirect_to project_issue_path(@project, @issue) else # NOTE: this CAPTCHA support method is indirectly included via IssuableActions - with_captcha_check_html_format { render :new } + with_captcha_check_html_format(spammable: spammable) { render :new } end end diff --git a/app/graphql/mutations/concerns/mutations/spam_protection.rb b/app/graphql/mutations/concerns/mutations/spam_protection.rb index 341067710b2..e61f66c02a5 100644 --- a/app/graphql/mutations/concerns/mutations/spam_protection.rb +++ b/app/graphql/mutations/concerns/mutations/spam_protection.rb @@ -16,30 +16,16 @@ module Mutations private - def spam_action_response(object) + def check_spam_action_response!(object) fields = spam_action_response_fields(object) - # If the SpamActionService detected something as spam, - # this is non-recoverable and the needs_captcha_response - # should not be considered - kind = if fields[:spam] - :spam - elsif fields[:needs_captcha_response] - :needs_captcha_response - end - - [kind, fields] - end - - def check_spam_action_response!(object) - kind, fields = spam_action_response(object) - - case kind - when :needs_captcha_response + if fields[:spam] + # If the SpamActionService detected something as spam, this is non-recoverable and the + # needs_captcha_response and other CAPTCHA-related fields should not be returned + raise SpamDisallowedError.new(SPAM_DISALLOWED_MESSAGE, extensions: { spam: true }) + elsif fields[:needs_captcha_response] fields.delete :spam raise NeedsCaptchaResponseError.new(NEEDS_CAPTCHA_RESPONSE_MESSAGE, extensions: fields) - when :spam - raise SpamDisallowedError.new(SPAM_DISALLOWED_MESSAGE, extensions: { spam: true }) else nil end diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 4901cd832ff..54ce62201d7 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -12,7 +12,7 @@ module Spammable included do has_one :user_agent_detail, as: :subject, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - attr_accessor :spam + attr_writer :spam attr_accessor :needs_recaptcha attr_accessor :spam_log @@ -29,6 +29,10 @@ module Spammable delegate :ip_address, :user_agent, to: :user_agent_detail, allow_nil: true end + def spam + !!@spam # rubocop:disable Gitlab/ModuleWithInstanceVariables + end + def submittable_as_spam_by?(current_user) current_user && current_user.admin? && submittable_as_spam? end diff --git a/app/models/project.rb b/app/models/project.rb index 512c6ac1acb..efbb8b5322d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2826,7 +2826,9 @@ class Project < ApplicationRecord if @topic_list != self.topic_list self.topics.delete_all - self.topics = @topic_list.map { |topic| Projects::Topic.find_or_create_by(name: topic) } + self.topics = @topic_list.map do |topic| + Projects::Topic.where('lower(name) = ?', topic.downcase).order(total_projects_count: :desc).first_or_create(name: topic) + end end @topic_list = nil diff --git a/app/models/projects/topic.rb b/app/models/projects/topic.rb index 78bc2df2e1e..235246b1f1d 100644 --- a/app/models/projects/topic.rb +++ b/app/models/projects/topic.rb @@ -7,7 +7,8 @@ module Projects include Avatarable include Gitlab::SQL::Pattern - validates :name, presence: true, uniqueness: true, length: { maximum: 255 } + validates :name, presence: true, length: { maximum: 255 } + validates :name, uniqueness: { case_sensitive: false }, if: :name_changed? validates :description, length: { maximum: 1024 } has_many :project_topics, class_name: 'Projects::ProjectTopic' diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb index 5ffa746e109..c88c139a22e 100644 --- a/app/services/groups/destroy_service.rb +++ b/app/services/groups/destroy_service.rb @@ -11,11 +11,15 @@ module Groups # rubocop: disable CodeReuse/ActiveRecord def execute + # TODO - add a policy check here https://gitlab.com/gitlab-org/gitlab/-/issues/353082 + raise DestroyError, "You can't delete this group because you're blocked." if current_user.blocked? + group.prepare_for_destroy group.projects.includes(:project_feature).each do |project| # Execute the destruction of the models immediately to ensure atomic cleanup. success = ::Projects::DestroyService.new(project, current_user).execute + raise DestroyError, "Project #{project.id} can't be deleted" unless success end diff --git a/data/deprecations/14-5-certificate-based-integration-with-kubernetes.yml b/data/deprecations/14-5-certificate-based-integration-with-kubernetes.yml index 698d8b0dc69..22ee8ac2942 100644 --- a/data/deprecations/14-5-certificate-based-integration-with-kubernetes.yml +++ b/data/deprecations/14-5-certificate-based-integration-with-kubernetes.yml @@ -5,19 +5,22 @@ removal_date: "2022-11-22" # the date of the milestone release when this feature is planned to be removed breaking_change: true body: | - [We are deprecating the certificate-based integration with Kubernetes](https://about.gitlab.com/blog/2021/11/15/deprecating-the-cert-based-kubernetes-integration/). - The timeline of removal of the integration from the product is planned to happen in two steps, starting with milestone 15.0 and finishing in GitLab version 15.6. + [The certificate-based integration with Kubernetes will be deprecated and removed](https://about.gitlab.com/blog/2021/11/15/deprecating-the-cert-based-kubernetes-integration/). - In 15.0, we plan to introduce a feature flag that will allow GitLab Self-Managed customers to keep the certificate-based integration enabled, it will be disabled by default. We plan to remove this feature flag together with the underlying code in GitLab version 15.6. - The certificate-based integration will continue to receive security and - critical fixes, and features built on the integration will continue to work with supported Kubernetes - versions until the final removal in 15.6. + If you are a self-managed customer, in GitLab 15.0, a feature flag will be introduced so you can keep + certificate-based integration enabled. The flag will be disabled by default. + The flag and the related code will be removed in GitLab 15.6. - For a more robust, secure, forthcoming, and reliable integration with Kubernetes, we recommend the use of the - [Kubernetes Agent](https://docs.gitlab.com/ee/user/clusters/agent/) to connect Kubernetes clusters with GitLab. - We provide [migration plans](https://docs.gitlab.com/ee/user/infrastructure/clusters/migrate_to_gitlab_agent.html) in the documentation. + Until the final removal in 15.6, features built on the integration will continue to work, and + GitLab will continue to fix security and critical issues. - For updates and details around this deprecation, follow this [epic](https://gitlab.com/groups/gitlab-org/configure/-/epics/8). + If you use GitLab.com, certificate-based integrations will cease functioning in 15.0. + + For a more robust, secure, forthcoming, and reliable integration with Kubernetes, we recommend you use the + [agent for Kubernetes](https://docs.gitlab.com/ee/user/clusters/agent/) to connect Kubernetes clusters with GitLab. + See the documentation for [how to migrate](https://docs.gitlab.com/ee/user/infrastructure/clusters/migrate_to_gitlab_agent.html). + + For updates and details about this deprecation, follow [this epic](https://gitlab.com/groups/gitlab-org/configure/-/epics/8). stage: Configure tiers: [Free, Silver, Gold, Core, Premium, Ultimate] issue_url: 'https://gitlab.com/groups/gitlab-org/configure/-/epics/8' diff --git a/db/migrate/20220203074916_add_topics_lower_name_index.rb b/db/migrate/20220203074916_add_topics_lower_name_index.rb new file mode 100644 index 00000000000..721abf66c67 --- /dev/null +++ b/db/migrate/20220203074916_add_topics_lower_name_index.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddTopicsLowerNameIndex < Gitlab::Database::Migration[1.0] + INDEX_NAME = 'index_topics_on_lower_name' + + disable_ddl_transaction! + + def up + add_concurrent_index :topics, 'lower(name)', name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :topics, INDEX_NAME + end +end diff --git a/db/schema_migrations/20220203074916 b/db/schema_migrations/20220203074916 new file mode 100644 index 00000000000..fffd0dcc003 --- /dev/null +++ b/db/schema_migrations/20220203074916 @@ -0,0 +1 @@ +5bb52cc70aada72e0e569006fd05de0c0d7629559d78bfd361009c91482f02cf \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 998117d421f..f6f1ceb5706 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -28023,6 +28023,8 @@ CREATE UNIQUE INDEX index_token_with_ivs_on_hashed_token ON token_with_ivs USING CREATE INDEX index_topics_non_private_projects_count ON topics USING btree (non_private_projects_count DESC, id); +CREATE INDEX index_topics_on_lower_name ON topics USING btree (lower(name)); + CREATE UNIQUE INDEX index_topics_on_name ON topics USING btree (name); CREATE INDEX index_topics_on_name_trigram ON topics USING gin (name gin_trgm_ops); diff --git a/doc/update/deprecations.md b/doc/update/deprecations.md index 37036ac0863..8fe19eb200e 100644 --- a/doc/update/deprecations.md +++ b/doc/update/deprecations.md @@ -160,19 +160,22 @@ as a [breaking change](https://docs.gitlab.com/ee/development/contributing/#brea Before updating GitLab, review the details carefully to determine if you need to make any changes to your code, settings, or workflow. -[We are deprecating the certificate-based integration with Kubernetes](https://about.gitlab.com/blog/2021/11/15/deprecating-the-cert-based-kubernetes-integration/). -The timeline of removal of the integration from the product is planned to happen in two steps, starting with milestone 15.0 and finishing in GitLab version 15.6. +[The certificate-based integration with Kubernetes will be deprecated and removed](https://about.gitlab.com/blog/2021/11/15/deprecating-the-cert-based-kubernetes-integration/). -In 15.0, we plan to introduce a feature flag that will allow GitLab Self-Managed customers to keep the certificate-based integration enabled, it will be disabled by default. We plan to remove this feature flag together with the underlying code in GitLab version 15.6. -The certificate-based integration will continue to receive security and -critical fixes, and features built on the integration will continue to work with supported Kubernetes -versions until the final removal in 15.6. +If you are a self-managed customer, in GitLab 15.0, a feature flag will be introduced so you can keep +certificate-based integration enabled. The flag will be disabled by default. +The flag and the related code will be removed in GitLab 15.6. -For a more robust, secure, forthcoming, and reliable integration with Kubernetes, we recommend the use of the -[Kubernetes Agent](https://docs.gitlab.com/ee/user/clusters/agent/) to connect Kubernetes clusters with GitLab. -We provide [migration plans](https://docs.gitlab.com/ee/user/infrastructure/clusters/migrate_to_gitlab_agent.html) in the documentation. +Until the final removal in 15.6, features built on the integration will continue to work, and +GitLab will continue to fix security and critical issues. -For updates and details around this deprecation, follow this [epic](https://gitlab.com/groups/gitlab-org/configure/-/epics/8). +If you use GitLab.com, certificate-based integrations will cease functioning in 15.0. + +For a more robust, secure, forthcoming, and reliable integration with Kubernetes, we recommend you use the +[agent for Kubernetes](https://docs.gitlab.com/ee/user/clusters/agent/) to connect Kubernetes clusters with GitLab. +See the documentation for [how to migrate](https://docs.gitlab.com/ee/user/infrastructure/clusters/migrate_to_gitlab_agent.html). + +For updates and details about this deprecation, follow [this epic](https://gitlab.com/groups/gitlab-org/configure/-/epics/8). **Planned removal milestone: 15.6 (2022-11-22)** diff --git a/doc/user/project/settings/project_access_tokens.md b/doc/user/project/settings/project_access_tokens.md index aa30c576c7c..c2c77f00875 100644 --- a/doc/user/project/settings/project_access_tokens.md +++ b/doc/user/project/settings/project_access_tokens.md @@ -24,6 +24,8 @@ Project access tokens are similar to [group access tokens](../../group/settings/ and [personal access tokens](../../profile/personal_access_tokens.md), except they are associated with a project rather than a group or user. +In self-managed instances, project access tokens are subject to the same [maximum lifetime limits](../../admin_area/settings/account_and_limit_settings.md#limit-the-lifetime-of-personal-access-tokens) as personal access tokens if the limit is set. + You can use project access tokens: - On GitLab SaaS if you have the Premium license tier or higher. Project access tokens are not available with a [trial license](https://about.gitlab.com/free-trial/). @@ -43,7 +45,8 @@ To create a project access token: 1. On the top bar, select **Menu > Projects** and find your project. 1. On the left sidebar, select **Settings > Access Tokens**. 1. Enter a name. The token name is visible to any user with permissions to view the project. -1. Optional. Enter an expiry date for the token. The token will expire on that date at midnight UTC. +1. Optional. Enter an expiry date for the token. The token expires on that date at midnight UTC. An instance-wide [maximum lifetime](../../admin_area/settings/account_and_limit_settings.md#limit-the-lifetime-of-personal-access-tokens) setting can limit the maximum allowable lifetime in self-managed instances. + 1. Select a role for the token. 1. Select the [desired scopes](#scopes-for-a-project-access-token). 1. Select **Create project access token**. diff --git a/lib/spam/concerns/has_spam_action_response_fields.rb b/lib/spam/concerns/has_spam_action_response_fields.rb index 6688ae56cb0..b33c922f7e6 100644 --- a/lib/spam/concerns/has_spam_action_response_fields.rb +++ b/lib/spam/concerns/has_spam_action_response_fields.rb @@ -7,7 +7,7 @@ module Spam module HasSpamActionResponseFields extend ActiveSupport::Concern - # spam_action_response_fields(spammable) -> hash + # spam_action_response_fields(spammable) -> hash # # Takes a Spammable as an argument and returns response fields necessary to display a CAPTCHA on # the client. diff --git a/qa/Gemfile b/qa/Gemfile index 1eaf9c7cec0..27945950530 100644 --- a/qa/Gemfile +++ b/qa/Gemfile @@ -4,7 +4,7 @@ source 'https://rubygems.org' gem 'gitlab-qa', require: 'gitlab/qa' gem 'activesupport', '~> 6.1.4.6' # This should stay in sync with the root's Gemfile -gem 'allure-rspec', '~> 2.15.0' +gem 'allure-rspec', '~> 2.16.0' gem 'capybara', '~> 3.35.0' gem 'capybara-screenshot', '~> 1.0.23' gem 'rake', '~> 13' diff --git a/qa/Gemfile.lock b/qa/Gemfile.lock index 6afd205a6e1..f5dc995fea3 100644 --- a/qa/Gemfile.lock +++ b/qa/Gemfile.lock @@ -19,10 +19,10 @@ GEM rack-test (>= 1.1.0, < 2.0) rest-client (>= 2.0.2, < 3.0) rspec (~> 3.8) - allure-rspec (2.15.0) - allure-ruby-commons (= 2.15.0) + allure-rspec (2.16.1) + allure-ruby-commons (= 2.16.1) rspec-core (>= 3.8, < 4) - allure-ruby-commons (2.15.0) + allure-ruby-commons (2.16.1) mime-types (>= 3.3, < 4) oj (>= 3.10, < 4) require_all (>= 2, < 4) @@ -203,7 +203,7 @@ GEM octokit (4.21.0) faraday (>= 0.9) sawyer (~> 0.8.0, >= 0.5.3) - oj (3.13.8) + oj (3.13.11) os (1.1.4) parallel (1.19.2) parallel_tests (2.29.0) @@ -324,7 +324,7 @@ PLATFORMS DEPENDENCIES activesupport (~> 6.1.4.6) airborne (~> 0.3.4) - allure-rspec (~> 2.15.0) + allure-rspec (~> 2.16.0) capybara (~> 3.35.0) capybara-screenshot (~> 1.0.23) chemlab (~> 0.9) diff --git a/qa/qa/runtime/allure_report.rb b/qa/qa/runtime/allure_report.rb index 9ae04dbe111..10f47ca56ba 100644 --- a/qa/qa/runtime/allure_report.rb +++ b/qa/qa/runtime/allure_report.rb @@ -74,8 +74,8 @@ module QA # @return [void] def configure_rspec RSpec.configure do |config| - config.add_formatter(AllureRspecFormatter) config.add_formatter(QA::Support::Formatters::AllureMetadataFormatter) + config.add_formatter(AllureRspecFormatter) config.append_after do |example| Allure.add_attachment( diff --git a/qa/qa/support/formatters/allure_metadata_formatter.rb b/qa/qa/support/formatters/allure_metadata_formatter.rb index da35ffde1ae..ad342f51e2e 100644 --- a/qa/qa/support/formatters/allure_metadata_formatter.rb +++ b/qa/qa/support/formatters/allure_metadata_formatter.rb @@ -4,20 +4,41 @@ module QA module Support module Formatters class AllureMetadataFormatter < ::RSpec::Core::Formatters::BaseFormatter + include Support::InfluxdbTools + ::RSpec::Core::Formatters.register( self, - :example_started + :start, + :example_finished ) - # Starts example + # Starts test run + # Fetch flakiness data in mr pipelines to help identify unrelated flaky failures + # + # @param [RSpec::Core::Notifications::StartNotification] _start_notification + # @return [void] + def start(_start_notification) + return unless merge_request_iid # on main runs allure native history has pass rate already + + save_failures + log(:debug, "Fetched #{failures.length} flaky testcases!") + rescue StandardError => e + log(:error, "Failed to fetch flaky spec data for report: #{e}") + @failures = [] + end + + # Finished example + # Add additional metadata to report + # # @param [RSpec::Core::Notifications::ExampleNotification] example_notification # @return [void] - def example_started(example_notification) + def example_finished(example_notification) example = example_notification.example add_quarantine_issue_link(example) add_failure_issues_link(example) add_ci_job_link(example) + set_flaky_status(example) end private @@ -55,6 +76,66 @@ module QA example.add_link(name: "Job(#{Runtime::Env.ci_job_name})", url: Runtime::Env.ci_job_url) end + + # Mark test as flaky + # + # @param [RSpec::Core::Example] example + # @return [void] + def set_flaky_status(example) + return unless merge_request_iid + return unless example.execution_result.status == :failed && failures.key?(example.metadata[:testcase]) + + example.set_flaky + example.parameter("pass_rate", "#{failures[example.metadata[:testcase]].round(1)}%") + log(:debug, "Setting spec as flaky due to present failures in last 14 days!") + end + + # Failed spec testcases + # + # @return [Array] + def failures + @failures ||= influx_data.lazy.each_with_object({}) do |data, result| + # TODO: replace with mr_iid once stats are populated + records = data.records.reject { |r| r.values["_value"] == env("CI_PIPELINE_ID") } + + runs = records.count + failed = records.count { |r| r.values["status"] == "failed" } + pass_rate = 100 - ((failed.to_f / runs.to_f) * 100) + + # Consider spec with a pass rate less than 98% as flaky + result[records.last.values["testcase"]] = pass_rate if pass_rate < 98 + end.compact + end + + alias_method :save_failures, :failures + + # Records of previous failures for runs of same type + # + # @return [Array] + def influx_data + return [] unless run_type + + query_api.query(query: <<~QUERY).values + from(bucket: "#{Support::InfluxdbTools::INFLUX_TEST_METRICS_BUCKET}") + |> range(start: -14d) + |> filter(fn: (r) => r._measurement == "test-stats") + |> filter(fn: (r) => r.run_type == "#{run_type}" and + r.status != "pending" and + r.quarantined == "false" and + r._field == "pipeline_id" + ) + |> group(columns: ["testcase"]) + QUERY + end + + # Print log message + # + # @param [Symbol] level + # @param [String] message + # @return [void] + def log(level, message) + QA::Runtime::Logger.public_send(level, "[Allure]: #{message}") + end end end end diff --git a/qa/qa/support/formatters/test_stats_formatter.rb b/qa/qa/support/formatters/test_stats_formatter.rb index 430294b0bb6..d6402888298 100644 --- a/qa/qa/support/formatters/test_stats_formatter.rb +++ b/qa/qa/support/formatters/test_stats_formatter.rb @@ -4,6 +4,8 @@ module QA module Support module Formatters class TestStatsFormatter < RSpec::Core::Formatters::BaseFormatter + include Support::InfluxdbTools + RSpec::Core::Formatters.register(self, :stop) # Finish test execution @@ -11,9 +13,6 @@ module QA # @param [RSpec::Core::Notifications::ExamplesNotification] notification # @return [void] def stop(notification) - return log(:warn, 'Missing QA_INFLUXDB_URL, skipping metrics export!') unless influxdb_url - return log(:warn, 'Missing QA_INFLUXDB_TOKEN, skipping metrics export!') unless influxdb_token - push_test_stats(notification.examples) push_fabrication_stats end @@ -27,7 +26,7 @@ module QA def push_test_stats(examples) data = examples.map { |example| test_stats(example) }.compact - influx_client.write(data: data) + write_api.write(data: data) log(:debug, "Pushed #{data.length} test execution entries to influxdb") rescue StandardError => e log(:error, "Failed to push test execution stats to influxdb, error: #{e}") @@ -42,7 +41,7 @@ module QA end return if data.empty? - influx_client.write(data: data) + write_api.write(data: data) log(:debug, "Pushed #{data.length} resource fabrication entries to influxdb") rescue StandardError => e log(:error, "Failed to push fabrication stats to influxdb, error: #{e}") @@ -70,7 +69,7 @@ module QA retried: ((example.metadata[:retry_attempts] || 0) > 0).to_s, job_name: job_name, merge_request: merge_request, - run_type: env('QA_RUN_TYPE') || run_type, + run_type: run_type, stage: devops_stage(file_path), testcase: example.metadata[:testcase] }, @@ -83,7 +82,8 @@ module QA retry_attempts: example.metadata[:retry_attempts] || 0, job_url: QA::Runtime::Env.ci_job_url, pipeline_url: env('CI_PIPELINE_URL'), - pipeline_id: env('CI_PIPELINE_ID') + pipeline_id: env('CI_PIPELINE_ID'), + merge_request_iid: merge_request_iid } } rescue StandardError => e @@ -119,13 +119,6 @@ module QA } end - # Project name - # - # @return [String] - def project_name - @project_name ||= QA::Runtime::Env.ci_project_name - end - # Base ci job name # # @return [String] @@ -148,26 +141,7 @@ module QA # # @return [String] def merge_request - @merge_request ||= (!!env('CI_MERGE_REQUEST_IID') || !!env('TOP_UPSTREAM_MERGE_REQUEST_IID')).to_s - end - - # Test run type from staging (`gstg`, `gstg-cny`, `gstg-ref`), canary, preprod or production env - # - # @return [String, nil] - def run_type - return unless %w[staging staging-canary staging-ref canary preprod production].include?(project_name) - - @run_type ||= begin - test_subset = if env('NO_ADMIN') == 'true' - 'sanity-no-admin' - elsif env('SMOKE_ONLY') == 'true' - 'sanity' - else - 'full' - end - - "#{project_name}-#{test_subset}" - end + (!!merge_request_iid).to_s end # Print log message @@ -179,16 +153,6 @@ module QA QA::Runtime::Logger.public_send(level, "[influxdb exporter]: #{message}") end - # Return non empty environment variable value - # - # @param [String] name - # @return [String, nil] - def env(name) - return unless ENV[name] && !ENV[name].empty? - - ENV[name] - end - # Get spec devops stage # # @param [String] location @@ -196,33 +160,6 @@ module QA def devops_stage(file_path) file_path.match(%r{\d{1,2}_(\w+)/})&.captures&.first end - - # InfluxDb client - # - # @return [InfluxDB2::WriteApi] - def influx_client - @influx_client ||= InfluxDB2::Client.new( - influxdb_url, - influxdb_token, - bucket: 'e2e-test-stats', - org: 'gitlab-qa', - precision: InfluxDB2::WritePrecision::NANOSECOND - ).create_write_api - end - - # InfluxDb instance url - # - # @return [String] - def influxdb_url - @influxdb_url ||= env('QA_INFLUXDB_URL') - end - - # Influxdb token - # - # @return [String] - def influxdb_token - @influxdb_token ||= env('QA_INFLUXDB_TOKEN') - end end end end diff --git a/qa/qa/support/influxdb_tools.rb b/qa/qa/support/influxdb_tools.rb new file mode 100644 index 00000000000..21a77ec0e52 --- /dev/null +++ b/qa/qa/support/influxdb_tools.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +module QA + module Support + # Common tools for use with influxdb metrics setup + # + module InfluxdbTools + INFLUX_TEST_METRICS_BUCKET = "e2e-test-stats" + LIVE_ENVS = %w[staging staging-canary staging-ref canary preprod production].freeze + + private + + delegate :ci_project_name, to: "QA::Runtime::Env" + + # Query client + # + # @return [QueryApi] + def query_api + @query_api ||= influx_client.create_query_api + end + + # Write client + # + # @return [WriteApi] + def write_api + @write_api ||= influx_client.create_write_api + end + + # InfluxDb client + # + # @return [InfluxDB2::Client] + def influx_client + @influx_client ||= InfluxDB2::Client.new( + ENV["QA_INFLUXDB_URL"] || raise("Missing QA_INFLUXDB_URL env variable"), + ENV["QA_INFLUXDB_TOKEN"] || raise("Missing QA_INFLUXDB_TOKEN env variable"), + bucket: INFLUX_TEST_METRICS_BUCKET, + org: "gitlab-qa", + precision: InfluxDB2::WritePrecision::NANOSECOND + ) + end + + # Test run type + # Automatically infer for staging (`gstg`, `gstg-cny`, `gstg-ref`), canary, preprod or production env + # + # @return [String, nil] + def run_type + @run_type ||= begin + return env('QA_RUN_TYPE') if env('QA_RUN_TYPE') + return unless LIVE_ENVS.include?(ci_project_name) + + test_subset = if env('NO_ADMIN') == 'true' + 'sanity-no-admin' + elsif env('SMOKE_ONLY') == 'true' + 'sanity' + else + 'full' + end + + "#{ci_project_name}-#{test_subset}" + end + end + + # Merge request iid + # + # @return [String] + def merge_request_iid + env('CI_MERGE_REQUEST_IID') || env('TOP_UPSTREAM_MERGE_REQUEST_IID') + end + + # Return non empty environment variable value + # + # @param [String] name + # @return [String, nil] + def env(name) + return unless ENV[name] && !ENV[name].empty? + + ENV[name] + end + end + end +end diff --git a/qa/qa/tools/reliable_report.rb b/qa/qa/tools/reliable_report.rb index 27e54c2d8bf..2735881c5d6 100644 --- a/qa/qa/tools/reliable_report.rb +++ b/qa/qa/tools/reliable_report.rb @@ -8,6 +8,7 @@ require "colorize" module QA module Tools class ReliableReport + include Support::InfluxdbTools include Support::API # Project for report creation: https://gitlab.com/gitlab-org/gitlab @@ -15,10 +16,7 @@ module QA def initialize(range) @range = range.to_i - @influxdb_bucket = "e2e-test-stats" @slack_channel = "#quality-reports" - @influxdb_url = ENV["QA_INFLUXDB_URL"] || raise("Missing QA_INFLUXDB_URL env variable") - @influxdb_token = ENV["QA_INFLUXDB_TOKEN"] || raise("Missing QA_INFLUXDB_TOKEN env variable") end # Run reliable reporter @@ -91,7 +89,7 @@ module QA private - attr_reader :range, :influxdb_bucket, :slack_channel, :influxdb_url, :influxdb_token + attr_reader :range, :slack_channel # Markdown formatted report issue body # @@ -304,7 +302,7 @@ module QA # @return [String] def query(reliable) <<~QUERY - from(bucket: "#{influxdb_bucket}") + from(bucket: "#{Support::InfluxdbTools::INFLUX_TEST_METRICS_BUCKET}") |> range(start: -#{range}d) |> filter(fn: (r) => r._measurement == "test-stats") |> filter(fn: (r) => r.run_type == "staging-full" or @@ -325,26 +323,6 @@ module QA QUERY end - # Query client - # - # @return [QueryApi] - def query_api - @query_api ||= influx_client.create_query_api - end - - # InfluxDb client - # - # @return [InfluxDB2::Client] - def influx_client - @influx_client ||= InfluxDB2::Client.new( - influxdb_url, - influxdb_token, - bucket: influxdb_bucket, - org: "gitlab-qa", - precision: InfluxDB2::WritePrecision::NANOSECOND - ) - end - # Slack notifier # # @return [Slack::Notifier] diff --git a/qa/spec/specs/allure_report_spec.rb b/qa/spec/specs/allure_report_spec.rb index 06b09106140..86ceaf51cbb 100644 --- a/qa/spec/specs/allure_report_spec.rb +++ b/qa/spec/specs/allure_report_spec.rb @@ -76,9 +76,10 @@ describe QA::Runtime::AllureReport do end it 'adds rspec and metadata formatter' do + expect(rspec_config).to have_received(:add_formatter).with( + QA::Support::Formatters::AllureMetadataFormatter + ).ordered expect(rspec_config).to have_received(:add_formatter).with(AllureRspecFormatter).ordered - expect(rspec_config).to have_received(:add_formatter) - .with(QA::Support::Formatters::AllureMetadataFormatter).ordered end it 'configures attachments saving' do diff --git a/qa/spec/support/formatters/allure_metadata_formatter_spec.rb b/qa/spec/support/formatters/allure_metadata_formatter_spec.rb index 631d2eda54f..d84e190fd56 100644 --- a/qa/spec/support/formatters/allure_metadata_formatter_spec.rb +++ b/qa/spec/support/formatters/allure_metadata_formatter_spec.rb @@ -14,6 +14,7 @@ describe QA::Support::Formatters::AllureMetadataFormatter do add_link: nil, attempts: 0, file_path: 'file/path/spec.rb', + execution_result: instance_double("RSpec::Core::Example::ExecutionResult", status: :passed), metadata: { testcase: 'testcase', quarantine: { issue: 'issue' } @@ -31,7 +32,7 @@ describe QA::Support::Formatters::AllureMetadataFormatter do end it "adds additional data to report" do - formatter.example_started(rspec_example_notification) + formatter.example_finished(rspec_example_notification) aggregate_failures do expect(rspec_example).to have_received(:issue).with('Quarantine issue', 'issue') diff --git a/qa/spec/support/formatters/test_stats_formatter_spec.rb b/qa/spec/support/formatters/test_stats_formatter_spec.rb index 84fc3b83185..2463b588759 100644 --- a/qa/spec/support/formatters/test_stats_formatter_spec.rb +++ b/qa/spec/support/formatters/test_stats_formatter_spec.rb @@ -60,7 +60,8 @@ describe QA::Support::Formatters::TestStatsFormatter do retry_attempts: 0, job_url: ci_job_url, pipeline_url: ci_pipeline_url, - pipeline_id: ci_pipeline_id + pipeline_id: ci_pipeline_id, + merge_request_iid: nil } } end diff --git a/spec/controllers/admin/topics_controller_spec.rb b/spec/controllers/admin/topics_controller_spec.rb index 6d66cb43338..ea510f916da 100644 --- a/spec/controllers/admin/topics_controller_spec.rb +++ b/spec/controllers/admin/topics_controller_spec.rb @@ -88,6 +88,13 @@ RSpec.describe Admin::TopicsController do expect(errors).to contain_exactly(errors.full_message(:name, I18n.t('errors.messages.blank'))) end + it 'shows error message if topic not unique (case insensitive)' do + post :create, params: { projects_topic: { name: topic.name.upcase } } + + errors = assigns[:topic].errors + expect(errors).to contain_exactly(errors.full_message(:name, I18n.t('errors.messages.taken'))) + end + context 'as a normal user' do before do sign_in(user) @@ -116,6 +123,15 @@ RSpec.describe Admin::TopicsController do expect(errors).to contain_exactly(errors.full_message(:name, I18n.t('errors.messages.blank'))) end + it 'shows error message if topic not unique (case insensitive)' do + other_topic = create(:topic, name: 'other-topic') + + put :update, params: { id: topic.id, projects_topic: { name: other_topic.name.upcase } } + + errors = assigns[:topic].errors + expect(errors).to contain_exactly(errors.full_message(:name, I18n.t('errors.messages.taken'))) + end + context 'as a normal user' do before do sign_in(user) diff --git a/spec/controllers/concerns/spammable_actions/akismet_mark_as_spam_action_spec.rb b/spec/controllers/concerns/spammable_actions/akismet_mark_as_spam_action_spec.rb index 7c10dccdcb9..caa0fa2d437 100644 --- a/spec/controllers/concerns/spammable_actions/akismet_mark_as_spam_action_spec.rb +++ b/spec/controllers/concerns/spammable_actions/akismet_mark_as_spam_action_spec.rb @@ -7,12 +7,6 @@ RSpec.describe SpammableActions::AkismetMarkAsSpamAction do controller(ActionController::Base) do include SpammableActions::AkismetMarkAsSpamAction - - private - - def spammable_path - '/fake_spammable_path' - end end let(:spammable_type) { 'SpammableType' } @@ -22,7 +16,6 @@ RSpec.describe SpammableActions::AkismetMarkAsSpamAction do before do allow(Gitlab::Recaptcha).to receive(:load_configurations!) { true } routes.draw { get 'mark_as_spam' => 'anonymous#mark_as_spam' } - allow(controller).to receive(:spammable) { spammable } allow(controller).to receive(:current_user) { double(:current_user, admin?: admin) } allow(controller).to receive(:current_user).and_return(current_user) end @@ -31,6 +24,9 @@ RSpec.describe SpammableActions::AkismetMarkAsSpamAction do subject { post :mark_as_spam } before do + allow(controller).to receive(:spammable) { spammable } + allow(controller).to receive(:spammable_path) { '/fake_spammable_path' } + expect_next(Spam::AkismetMarkAsSpamService, target: spammable) .to receive(:execute).and_return(execute_result) end @@ -68,4 +64,16 @@ RSpec.describe SpammableActions::AkismetMarkAsSpamAction do end end end + + describe '#spammable' do + it 'raises when unimplemented' do + expect { controller.send(:spammable) }.to raise_error(NotImplementedError) + end + end + + describe '#spammable_path' do + it 'raises when unimplemented' do + expect { controller.send(:spammable_path) }.to raise_error(NotImplementedError) + end + end end diff --git a/spec/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support_spec.rb b/spec/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support_spec.rb index 53a78326397..c5d17e0232c 100644 --- a/spec/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support_spec.rb +++ b/spec/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support_spec.rb @@ -7,7 +7,7 @@ RSpec.describe SpammableActions::CaptchaCheck::HtmlFormatActionsSupport do include SpammableActions::CaptchaCheck::HtmlFormatActionsSupport def create - with_captcha_check_html_format { render :some_rendered_view } + with_captcha_check_html_format(spammable: spammable) { render :some_rendered_view } end end diff --git a/spec/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support_spec.rb b/spec/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support_spec.rb index d7a44351ad8..7796d9d1273 100644 --- a/spec/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support_spec.rb +++ b/spec/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support_spec.rb @@ -7,7 +7,7 @@ RSpec.describe SpammableActions::CaptchaCheck::JsonFormatActionsSupport do include SpammableActions::CaptchaCheck::JsonFormatActionsSupport def some_action - with_captcha_check_json_format { render :some_rendered_view } + with_captcha_check_json_format(spammable: spammable) { render :some_rendered_view } end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index c487c87db1c..c0687a7a116 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7504,6 +7504,14 @@ RSpec.describe Project, factory_default: :keep do expect(project.save).to be_falsy expect(project.reload.topics.map(&:name)).to eq(%w[topic1 topic2 topic3]) end + + it 'does not add new topic if name is not unique (case insensitive)' do + project.topic_list = 'topic1, TOPIC2, topic3' + + project.save! + + expect(project.reload.topics.map(&:name)).to eq(%w[topic1 topic2 topic3]) + end end context 'public topics counter' do diff --git a/spec/models/projects/topic_spec.rb b/spec/models/projects/topic_spec.rb index 397c65f4d5c..b2d407d3298 100644 --- a/spec/models/projects/topic_spec.rb +++ b/spec/models/projects/topic_spec.rb @@ -22,7 +22,7 @@ RSpec.describe Projects::Topic do describe 'validations' do it { is_expected.to validate_presence_of(:name) } - it { is_expected.to validate_uniqueness_of(:name) } + it { is_expected.to validate_uniqueness_of(:name).case_insensitive } it { is_expected.to validate_length_of(:name).is_at_most(255) } it { is_expected.to validate_length_of(:description).is_at_most(1024) } end diff --git a/spec/requests/api/topics_spec.rb b/spec/requests/api/topics_spec.rb index 70eee8a1af9..68c8f5fdc43 100644 --- a/spec/requests/api/topics_spec.rb +++ b/spec/requests/api/topics_spec.rb @@ -142,6 +142,13 @@ RSpec.describe API::Topics do expect(response).to have_gitlab_http_status(:bad_request) expect(json_response['error']).to eql('name is missing') end + + it 'returns 400 if name is not unique (case insensitive)' do + post api('/topics/', admin), params: { name: topic_1.name.downcase } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']['name']).to eq(['has already been taken']) + end end context 'as normal user' do diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index 5135be8fff5..dd1335ec4e6 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -112,6 +112,17 @@ RSpec.describe Groups::DestroyService do end end + context 'when group owner is blocked' do + before do + user.block! + end + + it 'returns a more descriptive error message' do + expect { destroy_group(group, user, false) } + .to raise_error(Groups::DestroyService::DestroyError, "You can't delete this group because you're blocked.") + end + end + describe 'repository removal' do before do destroy_group(group, user, false)