diff --git a/.rubocop_todo/gitlab/no_code_coverage_comment.yml b/.rubocop_todo/gitlab/no_code_coverage_comment.yml new file mode 100644 index 00000000000..f4d50fda11b --- /dev/null +++ b/.rubocop_todo/gitlab/no_code_coverage_comment.yml @@ -0,0 +1,20 @@ +--- +Gitlab/NoCodeCoverageComment: + Details: grace period + Exclude: + - 'app/models/integration.rb' + - 'app/services/ci/job_artifacts/destroy_batch_service.rb' + - 'app/workers/database/batched_background_migration/single_database_worker.rb' + - 'config/initializers/net_http_response_patch.rb' + - 'ee/app/models/concerns/geo/replicable_model.rb' + - 'ee/app/services/namespaces/free_user_cap/remove_group_group_links_outside_hierarchy_service.rb' + - 'ee/app/workers/namespaces/free_user_cap/remediation_worker.rb' + - 'ee/lib/gitlab/geo/replicator.rb' + - 'lib/gitlab/auth/o_auth/session.rb' + - 'lib/gitlab/cycle_analytics/summary/defaults.rb' + - 'lib/gitlab/database/background_migration/health_status/signals.rb' + - 'lib/gitlab/seeder.rb' + - 'lib/gitlab/webpack/dev_server_middleware.rb' + - 'lib/tasks/dev.rake' + - 'lib/tasks/gems.rake' + - 'lib/tasks/gitlab/db.rake' diff --git a/Gemfile b/Gemfile index 8cf345a5829..026d7bee543 100644 --- a/Gemfile +++ b/Gemfile @@ -277,7 +277,7 @@ gem 'sanitize', '~> 6.0' gem 'babosa', '~> 1.0.4' # Sanitizes SVG input -gem 'loofah', '~> 2.18.0' +gem 'loofah', '~> 2.19.0' # Working with license # Detects the open source license the repository includes diff --git a/Gemfile.checksum b/Gemfile.checksum index 31845fc2f3b..4480ebef05e 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -307,7 +307,7 @@ {"name":"locale","version":"2.1.3","platform":"ruby","checksum":"b6ddee011e157817cb98e521b3ce7cb626424d5882f1e844aafdee3e8b212725"}, {"name":"lockbox","version":"0.6.2","platform":"ruby","checksum":"0136677875c3d6e27cef87cd7bd66610404e2b3cd7f07f1ac8ed34e48f18dc3c"}, {"name":"lograge","version":"0.11.2","platform":"ruby","checksum":"4cbd1554b86f545d795eff15a0c24fd25057d2ac4e1caa5fc186168b3da932ef"}, -{"name":"loofah","version":"2.18.0","platform":"ruby","checksum":"61975a247a6aeb8f09ac5a3430305451efc4525c0b9b79c05feaec35a8b9d5a3"}, +{"name":"loofah","version":"2.19.0","platform":"ruby","checksum":"302791371f473611e342f9e469e7f2fbf1155bb1b3a978a83ac7df625298feba"}, {"name":"lookbook","version":"1.0.3","platform":"ruby","checksum":"c53e130a37588e32f66be3b9418b1efcb51bef69946b276edd3b4348a71cbcd6"}, {"name":"lru_redux","version":"1.1.0","platform":"ruby","checksum":"ee71d0ccab164c51de146c27b480a68b3631d5b4297b8ffe8eda1c72de87affb"}, {"name":"lumberjack","version":"1.2.7","platform":"ruby","checksum":"a5c6aae6b4234f1420dbcd80b23e3bca0817bd239440dde097ebe3fa63c63b1f"}, diff --git a/Gemfile.lock b/Gemfile.lock index 33331a956f7..54209a20cbd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -826,7 +826,7 @@ GEM activesupport (>= 4) railties (>= 4) request_store (~> 1.0) - loofah (2.18.0) + loofah (2.19.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) lookbook (1.0.3) @@ -1668,7 +1668,7 @@ DEPENDENCIES licensee (~> 9.15) lockbox (~> 0.6.2) lograge (~> 0.5) - loofah (~> 2.18.0) + loofah (~> 2.19.0) lookbook (~> 1.0) lru_redux mail (= 2.7.1) diff --git a/app/services/ci/pipeline_artifacts/destroy_all_expired_service.rb b/app/services/ci/pipeline_artifacts/destroy_all_expired_service.rb index 17c039885e5..adc761eff38 100644 --- a/app/services/ci/pipeline_artifacts/destroy_all_expired_service.rb +++ b/app/services/ci/pipeline_artifacts/destroy_all_expired_service.rb @@ -3,20 +3,26 @@ module Ci module PipelineArtifacts class DestroyAllExpiredService + include ::Gitlab::ExclusiveLeaseHelpers include ::Gitlab::LoopHelpers include ::Gitlab::Utils::StrongMemoize BATCH_SIZE = 100 - LOOP_TIMEOUT = 5.minutes LOOP_LIMIT = 1000 + LOOP_TIMEOUT = 5.minutes + LOCK_TIMEOUT = 10.minutes + EXCLUSIVE_LOCK_KEY = 'expired_pipeline_artifacts:destroy:lock' def initialize @removed_artifacts_count = 0 + @start_at = Time.current end def execute - loop_until(timeout: LOOP_TIMEOUT, limit: LOOP_LIMIT) do - destroy_artifacts_batch + in_lock(EXCLUSIVE_LOCK_KEY, ttl: LOCK_TIMEOUT, retries: 1) do + destroy_unlocked_pipeline_artifacts if ::Feature.enabled?(:ci_destroy_unlocked_pipeline_artifacts) + + legacy_destroy_pipeline_artifacts end @removed_artifacts_count @@ -24,10 +30,30 @@ module Ci private + def destroy_unlocked_pipeline_artifacts + loop_until(timeout: LOOP_TIMEOUT, limit: LOOP_LIMIT) do + artifacts = Ci::PipelineArtifact.expired_before(@start_at).artifact_unlocked.limit(BATCH_SIZE) + + break if artifacts.empty? + + destroy_batch(artifacts) + end + end + + def legacy_destroy_pipeline_artifacts + loop_until(timeout: LOOP_TIMEOUT, limit: LOOP_LIMIT) do + destroy_artifacts_batch + end + end + def destroy_artifacts_batch artifacts = ::Ci::PipelineArtifact.unlocked.expired.limit(BATCH_SIZE).to_a return false if artifacts.empty? + destroy_batch(artifacts) + end + + def destroy_batch(artifacts) artifacts.each(&:destroy!) increment_stats(artifacts.size) diff --git a/config/feature_flags/development/ci_destroy_unlocked_pipeline_artifacts.yml b/config/feature_flags/development/ci_destroy_unlocked_pipeline_artifacts.yml new file mode 100644 index 00000000000..39c6eb889db --- /dev/null +++ b/config/feature_flags/development/ci_destroy_unlocked_pipeline_artifacts.yml @@ -0,0 +1,8 @@ +--- +name: ci_destroy_unlocked_pipeline_artifacts +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/98633 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/375011 +milestone: '15.5' +type: development +group: group::pipeline insights +default_enabled: false diff --git a/db/post_migrate/20220923052531_remove_tmp_index_merge_request_reviewers_on_attention_requested_state.rb b/db/post_migrate/20220923052531_remove_tmp_index_merge_request_reviewers_on_attention_requested_state.rb new file mode 100644 index 00000000000..27d9c1641bc --- /dev/null +++ b/db/post_migrate/20220923052531_remove_tmp_index_merge_request_reviewers_on_attention_requested_state.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class RemoveTmpIndexMergeRequestReviewersOnAttentionRequestedState < Gitlab::Database::Migration[2.0] + INDEX_NAME = "tmp_index_merge_request_reviewers_on_attention_requested_state" + ATTENTION_REQUESTED_STATE = 2 + + disable_ddl_transaction! + + def up + remove_concurrent_index_by_name :merge_request_reviewers, INDEX_NAME + end + + def down + add_concurrent_index :merge_request_reviewers, [:id], + where: "state = #{ATTENTION_REQUESTED_STATE}", + name: INDEX_NAME + end +end diff --git a/db/schema_migrations/20220923052531 b/db/schema_migrations/20220923052531 new file mode 100644 index 00000000000..db7d24075f1 --- /dev/null +++ b/db/schema_migrations/20220923052531 @@ -0,0 +1 @@ +198d1447a8a857ea18409fe99e5a5a616d966b480bb6fc8f05613a651fdcd8a9 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index f5d673f249e..f104cd4dcf7 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -30860,8 +30860,6 @@ CREATE INDEX tmp_index_issues_on_issue_type_and_id ON issues USING btree (issue_ CREATE INDEX tmp_index_members_on_state ON members USING btree (state) WHERE (state = 2); -CREATE INDEX tmp_index_merge_request_reviewers_on_attention_requested_state ON merge_request_reviewers USING btree (id) WHERE (state = 2); - CREATE INDEX tmp_index_migrated_container_registries ON container_repositories USING btree (project_id) WHERE ((migration_state = 'import_done'::text) OR (created_at >= '2022-01-23 00:00:00'::timestamp without time zone)); CREATE UNIQUE INDEX tmp_index_on_tmp_project_id_on_namespaces ON namespaces USING btree (tmp_project_id); diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index d8fd02e1779..c13bd285f2e 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -17622,7 +17622,7 @@ Represents a requirement. | `author` | [`UserCore!`](#usercore) | Author of the requirement. | | `createdAt` | [`Time!`](#time) | Timestamp of when the requirement was created. | | `description` | [`String`](#string) | Description of the requirement. | -| `descriptionHtml` | [`String`](#string) | The GitLab Flavored Markdown rendering of `description`. | +| `descriptionHtml` | [`String`](#string) | GitLab Flavored Markdown rendering of `description`. | | `id` | [`ID!`](#id) | ID of the requirement. | | `iid` | [`ID!`](#id) | Internal ID of the requirement. | | `lastTestReportManuallyCreated` | [`Boolean`](#boolean) | Indicates if latest test report was created by user. | @@ -17630,7 +17630,7 @@ Represents a requirement. | `project` | [`Project!`](#project) | Project to which the requirement belongs. | | `state` | [`RequirementState!`](#requirementstate) | State of the requirement. | | `title` | [`String`](#string) | Title of the requirement. | -| `titleHtml` | [`String`](#string) | The GitLab Flavored Markdown rendering of `title`. | +| `titleHtml` | [`String`](#string) | GitLab Flavored Markdown rendering of `title`. | | `updatedAt` | [`Time!`](#time) | Timestamp of when the requirement was last updated. | | `userPermissions` | [`RequirementPermissions!`](#requirementpermissions) | Permissions for the current user on the resource. | diff --git a/rubocop/cop/gitlab/no_code_coverage_comment.rb b/rubocop/cop/gitlab/no_code_coverage_comment.rb new file mode 100644 index 00000000000..3b989930026 --- /dev/null +++ b/rubocop/cop/gitlab/no_code_coverage_comment.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Gitlab + # Discourages the use of `# :nocov:` to exclude code from coverage report. + # + # The nocov token can be configured via `CommentToken` option and defaults + # to `'nocov'`. + # + # @example CommentToken: 'nocov' (default) + # + # # bad + # # :nocov: + # def method + # end + # # :nocov: + # + # # good + # def method + # end + # + class NoCodeCoverageComment < RuboCop::Cop::Base + include RangeHelp + + MSG = 'The use of %s is discouraged. All code must have tests. See https://docs.gitlab.com/ee/development/contributing/merge_request_workflow.html#testing' + + DEFAULT_COMMENT_TOKEN = 'nocov' + + def on_new_investigation + super + + nocov_token = cop_config.fetch('CommentToken', DEFAULT_COMMENT_TOKEN) + nocov_comment = ":#{nocov_token}:" + # See https://github.com/simplecov-ruby/simplecov/blob/v0.21.2/lib/simplecov/lines_classifier.rb#L16 + regexp = /^(?:\s*)#(?:\s*)(?::#{nocov_token}:)/ + + processed_source.comments.each do |comment| + register_offense(comment, nocov_comment) if regexp.match?(comment.text) + end + end + + private + + def register_offense(comment, nocov_comment) + range = range_of_offense(comment, nocov_comment) + message = format(MSG, nocov_comment: nocov_comment) + + add_offense(range, message: message) + end + + def range_of_offense(comment, name) + start_pos = comment_start(comment) + token_indentation(comment, name) + range_between(start_pos, start_pos + name.size) + end + + def comment_start(comment) + comment.loc.expression.begin_pos + end + + def token_indentation(comment, name) + comment.text.index(name) + end + end + end + end +end diff --git a/spec/factories/ci/pipeline_artifacts.rb b/spec/factories/ci/pipeline_artifacts.rb index d096f149c3a..bdd390126dd 100644 --- a/spec/factories/ci/pipeline_artifacts.rb +++ b/spec/factories/ci/pipeline_artifacts.rb @@ -17,6 +17,11 @@ FactoryBot.define do association :pipeline, :unlocked, factory: :ci_pipeline end + trait :artifact_unlocked do + association :pipeline, :unlocked, factory: :ci_pipeline + locked { :unlocked } + end + trait :checksummed do verification_checksum { 'abc' } end diff --git a/spec/rubocop/cop/gitlab/no_code_coverage_comment_spec.rb b/spec/rubocop/cop/gitlab/no_code_coverage_comment_spec.rb new file mode 100644 index 00000000000..f0c0297d266 --- /dev/null +++ b/spec/rubocop/cop/gitlab/no_code_coverage_comment_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'rubocop_spec_helper' +require_relative '../../../../rubocop/cop/gitlab/no_code_coverage_comment' + +RSpec.describe RuboCop::Cop::Gitlab::NoCodeCoverageComment do + let(:msg) { format(described_class::MSG, nocov_comment: nocov_comment) } + let(:nocov_comment) { ":#{comment_token}:" } + + shared_examples 'nocov check' do + it 'flags related code comments' do + expect_offense(<<~RUBY, nocov_token: comment_token, msg: msg) + # :%{nocov_token}: + ^^^{nocov_token} %{msg} + def method + end + #:%{nocov_token}: + ^^^{nocov_token} %{msg} + + def other_method + if expr + # :%{nocov_token}: With some additional comments + ^^^{nocov_token} %{msg} + value << line.strip + # :%{nocov_token}: + ^^^{nocov_token} %{msg} + end + end + RUBY + end + + it 'ignores unrelated comments' do + expect_no_offenses(<<~RUBY) + # Other comments are ignored :#{comment_token}: + # + # # :#{comment_token}: + RUBY + end + end + + context 'with nocov as default comment token' do + it_behaves_like 'nocov check' do + let(:comment_token) { described_class::DEFAULT_COMMENT_TOKEN } + end + end + + context 'with configured comment token' do + it_behaves_like 'nocov check' do + let(:comment_token) { 'skipit' } + + let(:config) do + RuboCop::Config.new( + 'Gitlab/NoCodeCoverageComment' => { + 'CommentToken' => comment_token + } + ) + end + end + end +end diff --git a/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb b/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb index eb664043567..d6499203231 100644 --- a/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb +++ b/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Ci::PipelineArtifacts::DestroyAllExpiredService do +RSpec.describe Ci::PipelineArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_shared_state do let(:service) { described_class.new } describe '.execute' do @@ -85,6 +85,53 @@ RSpec.describe Ci::PipelineArtifacts::DestroyAllExpiredService do is_expected.to eq(0) end end + + context 'with unlocked pipeline artifacts' do + let_it_be(:not_expired_artifact) { create(:ci_pipeline_artifact, :artifact_unlocked, expire_at: 2.days.from_now) } + + before do + create_list(:ci_pipeline_artifact, 2, :artifact_unlocked, expire_at: 1.week.ago) + end + + context 'with ci_destroy_unlocked_pipeline_artifacts feature flag enabled' do + before do + allow(service).to receive(:legacy_destroy_pipeline_artifacts) + end + + it 'destroys all expired artifacts' do + expect { subject }.to change { Ci::PipelineArtifact.count }.by(-2) + expect(not_expired_artifact.reload).to be_present + end + + context 'when the loop limit is reached' do + before do + stub_const('::Ci::PipelineArtifacts::DestroyAllExpiredService::LOOP_LIMIT', 1) + stub_const('::Ci::PipelineArtifacts::DestroyAllExpiredService::BATCH_SIZE', 1) + end + + it 'destroys one artifact' do + expect { subject }.to change { Ci::PipelineArtifact.count }.by(-1) + expect(not_expired_artifact.reload).to be_present + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(1) + end + end + end + + context 'with ci_destroy_unlocked_pipeline_artifacts feature flag disabled' do + before do + stub_feature_flags(ci_destroy_unlocked_pipeline_artifacts: false) + end + + it 'destroys all expired artifacts' do + expect(service).not_to receive(:destroy_unlocked_pipeline_artifacts) + expect { subject }.to change { Ci::PipelineArtifact.count }.by(-2) + expect(not_expired_artifact.reload).to be_present + end + end + end end describe '.destroy_artifacts_batch' do