diff options
31 files changed, 238 insertions, 103 deletions
diff --git a/.gitlab/issue_templates/Security developer workflow.md b/.gitlab/issue_templates/Security developer workflow.md index beb066cdfc4..0c2f02a99cb 100644 --- a/.gitlab/issue_templates/Security developer workflow.md +++ b/.gitlab/issue_templates/Security developer workflow.md @@ -13,6 +13,7 @@ Set the title to: `Description of the original issue` - [ ] Mark this [issue as related] to the Security Release Tracking Issue. You can find it on the topic of the `#releases` Slack channel. - Fill out the [Links section](#links): - [ ] Next to **Issue on GitLab**, add a link to the `gitlab-org/gitlab` issue that describes the security vulnerability. +- [ ] Add one of the `~severity::x` labels to the issue and all associated merge requests. ## Development diff --git a/changelogs/unreleased/rename_to_promote_smoother_transition.yml b/changelogs/unreleased/rename_to_promote_smoother_transition.yml new file mode 100644 index 00000000000..1d75a069564 --- /dev/null +++ b/changelogs/unreleased/rename_to_promote_smoother_transition.yml @@ -0,0 +1,5 @@ +--- +title: Rename jobs to promote a smoother transition between Klar and Trivy based scanners +merge_request: 57593 +author: +type: changed diff --git a/changelogs/unreleased/repository-set-cache-races.yml b/changelogs/unreleased/repository-set-cache-races.yml new file mode 100644 index 00000000000..e3365ab7c2c --- /dev/null +++ b/changelogs/unreleased/repository-set-cache-races.yml @@ -0,0 +1,5 @@ +--- +title: Fix two data races in the branch names cache +merge_request: 57607 +author: +type: fixed diff --git a/config/feature_flags/ops/prometheus_metrics_method_instrumentation.yml b/config/feature_flags/ops/prometheus_metrics_method_instrumentation.yml new file mode 100644 index 00000000000..d5375a98ea0 --- /dev/null +++ b/config/feature_flags/ops/prometheus_metrics_method_instrumentation.yml @@ -0,0 +1,8 @@ +--- +name: prometheus_metrics_method_instrumentation +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/4304 +rollout_issue_url: +milestone: '10.5' +type: ops +group: +default_enabled: false diff --git a/config/feature_flags/ops/prometheus_metrics_view_instrumentation.yml b/config/feature_flags/ops/prometheus_metrics_view_instrumentation.yml new file mode 100644 index 00000000000..ebc31ede176 --- /dev/null +++ b/config/feature_flags/ops/prometheus_metrics_view_instrumentation.yml @@ -0,0 +1,8 @@ +--- +name: prometheus_metrics_view_instrumentation +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/4304 +rollout_issue_url: +milestone: '10.5' +type: ops +group: +default_enabled: false diff --git a/lib/bulk_imports/groups/graphql/get_labels_query.rb b/lib/bulk_imports/groups/graphql/get_labels_query.rb index 2d246a217a7..41c50cc9a36 100644 --- a/lib/bulk_imports/groups/graphql/get_labels_query.rb +++ b/lib/bulk_imports/groups/graphql/get_labels_query.rb @@ -12,7 +12,7 @@ module BulkImports group(fullPath: $full_path) { labels(first: 100, after: $cursor, onlyGroupLabels: true) { page_info: pageInfo { - end_cursor: endCursor + next_page: endCursor has_next_page: hasNextPage } nodes { diff --git a/lib/bulk_imports/groups/graphql/get_members_query.rb b/lib/bulk_imports/groups/graphql/get_members_query.rb index f3a0d77acc2..6e73201361f 100644 --- a/lib/bulk_imports/groups/graphql/get_members_query.rb +++ b/lib/bulk_imports/groups/graphql/get_members_query.rb @@ -11,7 +11,7 @@ module BulkImports group(fullPath: $full_path) { group_members: groupMembers(relations: DIRECT, first: 100, after: $cursor) { page_info: pageInfo { - end_cursor: endCursor + next_page: endCursor has_next_page: hasNextPage } nodes { diff --git a/lib/bulk_imports/groups/graphql/get_milestones_query.rb b/lib/bulk_imports/groups/graphql/get_milestones_query.rb index c254b35054a..bf1943eb15b 100644 --- a/lib/bulk_imports/groups/graphql/get_milestones_query.rb +++ b/lib/bulk_imports/groups/graphql/get_milestones_query.rb @@ -12,7 +12,7 @@ module BulkImports group(fullPath: $full_path) { milestones(first: 100, after: $cursor, includeDescendants: false) { page_info: pageInfo { - end_cursor: endCursor + next_page: endCursor has_next_page: hasNextPage } nodes { diff --git a/lib/bulk_imports/pipeline/extracted_data.rb b/lib/bulk_imports/pipeline/extracted_data.rb index 15b5caa1a47..c9e54b61dd3 100644 --- a/lib/bulk_imports/pipeline/extracted_data.rb +++ b/lib/bulk_imports/pipeline/extracted_data.rb @@ -18,7 +18,7 @@ module BulkImports end def next_page - @page_info&.dig('end_cursor') + @page_info&.dig('next_page') end def each(&block) diff --git a/lib/feature.rb b/lib/feature.rb index 7c926b25587..ad243719096 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -57,7 +57,7 @@ class Feature # use `default_enabled: true` to default the flag to being `enabled` # unless set explicitly. The default is `disabled` # TODO: remove the `default_enabled:` and read it from the `defintion_yaml` - # check: https://gitlab.com/gitlab-org/gitlab/-/issues/30228 + # check: https://gitlab.com/gitlab-org/gitlab/-/issues/271275 def enabled?(key, thing = nil, type: :development, default_enabled: false) if check_feature_flags_definition? if thing && !thing.respond_to?(:flipper_id) @@ -65,11 +65,11 @@ class Feature "The thing '#{thing.class.name}' for feature flag '#{key}' needs to include `FeatureGate` or implement `flipper_id`" end - Feature::Definition.valid_usage!(key, type: type, default_enabled: default_enabled) + Feature::Definition.valid_usage!(key, type: type, default_enabled: :yaml) end - # If `default_enabled: :yaml` we fetch the value from the YAML definition instead. - default_enabled = Feature::Definition.default_enabled?(key) if default_enabled == :yaml + # TODO: Remove rubocop disable comment once `default_enabled` argument is removed https://gitlab.com/gitlab-org/gitlab/-/issues/271275 + default_enabled = Feature::Definition.default_enabled?(key) # rubocop:disable Lint/ShadowedArgument # During setup the database does not exist yet. So we haven't stored a value # for the feature yet and return the default. diff --git a/lib/gitlab/ci/templates/Security/Container-Scanning.gitlab-ci.yml b/lib/gitlab/ci/templates/Security/Container-Scanning.gitlab-ci.yml index 953ca62e511..c628e30b2c7 100644 --- a/lib/gitlab/ci/templates/Security/Container-Scanning.gitlab-ci.yml +++ b/lib/gitlab/ci/templates/Security/Container-Scanning.gitlab-ci.yml @@ -24,7 +24,7 @@ variables: container_scanning: gl-container-scanning-report.json dependencies: [] -container_scanning_deprecated: +container_scanning: extends: .cs_common variables: # By default, use the latest clair vulnerabilities database, however, allow it to be overridden here with a specific image @@ -44,7 +44,7 @@ container_scanning_deprecated: $GITLAB_FEATURES =~ /\bcontainer_scanning\b/ && $CS_MAJOR_VERSION =~ /^[0-3]$/ -container_scanning: +container_scanning_new: extends: .cs_common variables: CS_PROJECT: 'container-scanning' diff --git a/lib/gitlab/repository_cache_adapter.rb b/lib/gitlab/repository_cache_adapter.rb index eb7c9bccf96..d0230c035cc 100644 --- a/lib/gitlab/repository_cache_adapter.rb +++ b/lib/gitlab/repository_cache_adapter.rb @@ -60,14 +60,17 @@ module Gitlab define_method("#{name}_include?") do |value| ivar = "@#{name}_include" memoized = instance_variable_get(ivar) || {} + lookup = proc { __send__(name).include?(value) } # rubocop:disable GitlabSecurity/PublicSend next memoized[value] if memoized.key?(value) memoized[value] = - if strong_memoized?(name) || !redis_set_cache.exist?(name) - __send__(name).include?(value) # rubocop:disable GitlabSecurity/PublicSend + if strong_memoized?(name) + lookup.call else - redis_set_cache.include?(name, value) + result, exists = redis_set_cache.try_include?(name, value) + + exists ? result : lookup.call end instance_variable_set(ivar, memoized)[value] diff --git a/lib/gitlab/repository_set_cache.rb b/lib/gitlab/repository_set_cache.rb index 69c1688767c..def7b58a852 100644 --- a/lib/gitlab/repository_set_cache.rb +++ b/lib/gitlab/repository_set_cache.rb @@ -36,11 +36,18 @@ module Gitlab end def fetch(key, &block) - if exist?(key) - read(key) - else - write(key, yield) + full_key = cache_key(key) + + smembers, exists = with do |redis| + redis.multi do + redis.smembers(full_key) + redis.exists(full_key) + end end + + return smembers if exists + + write(key, yield) end end end diff --git a/lib/gitlab/set_cache.rb b/lib/gitlab/set_cache.rb index 591265d014e..0f2b7b194c9 100644 --- a/lib/gitlab/set_cache.rb +++ b/lib/gitlab/set_cache.rb @@ -51,6 +51,19 @@ module Gitlab with { |redis| redis.sismember(cache_key(key), value) } end + # Like include?, but also tells us if the cache was populated when it ran + # by returning two booleans: [member_exists, set_exists] + def try_include?(key, value) + full_key = cache_key(key) + + with do |redis| + redis.multi do + redis.sismember(full_key, value) + redis.exists(full_key) + end + end + end + def ttl(key) with { |redis| redis.ttl(cache_key(key)) } end diff --git a/spec/graphql/features/feature_flag_spec.rb b/spec/graphql/features/feature_flag_spec.rb index 30238cf9cb3..95a5bcab6f4 100644 --- a/spec/graphql/features/feature_flag_spec.rb +++ b/spec/graphql/features/feature_flag_spec.rb @@ -15,6 +15,7 @@ RSpec.describe 'Graphql Field feature flags' do before do skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check end subject { result } diff --git a/spec/graphql/types/base_field_spec.rb b/spec/graphql/types/base_field_spec.rb index 54b59317b55..6388ceb358c 100644 --- a/spec/graphql/types/base_field_spec.rb +++ b/spec/graphql/types/base_field_spec.rb @@ -128,6 +128,7 @@ RSpec.describe Types::BaseField do before do skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check end it 'returns false if the feature is not enabled' do diff --git a/spec/lib/api/helpers_spec.rb b/spec/lib/api/helpers_spec.rb index a8547b39b27..007ebac0073 100644 --- a/spec/lib/api/helpers_spec.rb +++ b/spec/lib/api/helpers_spec.rb @@ -183,6 +183,7 @@ RSpec.describe API::Helpers do before do skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check end context 'with feature enabled' do diff --git a/spec/lib/bulk_imports/groups/pipelines/labels_pipeline_spec.rb b/spec/lib/bulk_imports/groups/pipelines/labels_pipeline_spec.rb index eeed5c6d079..8af646d1101 100644 --- a/spec/lib/bulk_imports/groups/pipelines/labels_pipeline_spec.rb +++ b/spec/lib/bulk_imports/groups/pipelines/labels_pipeline_spec.rb @@ -94,7 +94,7 @@ RSpec.describe BulkImports::Groups::Pipelines::LabelsPipeline do def extracted_data(title:, has_next_page: false) page_info = { 'has_next_page' => has_next_page, - 'end_cursor' => has_next_page ? 'cursor' : nil + 'next_page' => has_next_page ? 'cursor' : nil } BulkImports::Pipeline::ExtractedData.new(data: [label_data(title)], page_info: page_info) diff --git a/spec/lib/bulk_imports/groups/pipelines/members_pipeline_spec.rb b/spec/lib/bulk_imports/groups/pipelines/members_pipeline_spec.rb index 0af45ae17d6..d8a667ec92a 100644 --- a/spec/lib/bulk_imports/groups/pipelines/members_pipeline_spec.rb +++ b/spec/lib/bulk_imports/groups/pipelines/members_pipeline_spec.rb @@ -103,7 +103,7 @@ RSpec.describe BulkImports::Groups::Pipelines::MembersPipeline do page_info = { 'has_next_page' => has_next_page, - 'end_cursor' => has_next_page ? 'cursor' : nil + 'next_page' => has_next_page ? 'cursor' : nil } BulkImports::Pipeline::ExtractedData.new(data: data, page_info: page_info) diff --git a/spec/lib/bulk_imports/groups/pipelines/milestones_pipeline_spec.rb b/spec/lib/bulk_imports/groups/pipelines/milestones_pipeline_spec.rb index 3ce81026834..6eba6f34a7f 100644 --- a/spec/lib/bulk_imports/groups/pipelines/milestones_pipeline_spec.rb +++ b/spec/lib/bulk_imports/groups/pipelines/milestones_pipeline_spec.rb @@ -111,7 +111,7 @@ RSpec.describe BulkImports::Groups::Pipelines::MilestonesPipeline do def extracted_data(title:, has_next_page: false) page_info = { 'has_next_page' => has_next_page, - 'end_cursor' => has_next_page ? 'cursor' : nil + 'next_page' => has_next_page ? 'cursor' : nil } BulkImports::Pipeline::ExtractedData.new( diff --git a/spec/lib/bulk_imports/pipeline/extracted_data_spec.rb b/spec/lib/bulk_imports/pipeline/extracted_data_spec.rb index 25c5178227a..9c79b3f4c9e 100644 --- a/spec/lib/bulk_imports/pipeline/extracted_data_spec.rb +++ b/spec/lib/bulk_imports/pipeline/extracted_data_spec.rb @@ -9,7 +9,7 @@ RSpec.describe BulkImports::Pipeline::ExtractedData do let(:page_info) do { 'has_next_page' => has_next_page, - 'end_cursor' => cursor + 'next_page' => cursor } end diff --git a/spec/lib/bulk_imports/pipeline/runner_spec.rb b/spec/lib/bulk_imports/pipeline/runner_spec.rb index 9cadc06d613..7235b7c95cd 100644 --- a/spec/lib/bulk_imports/pipeline/runner_spec.rb +++ b/spec/lib/bulk_imports/pipeline/runner_spec.rb @@ -245,7 +245,7 @@ RSpec.describe BulkImports::Pipeline::Runner do data: { foo: :bar }, page_info: { 'has_next_page' => has_next_page, - 'end_cursor' => has_next_page ? 'cursor' : nil + 'next_page' => has_next_page ? 'cursor' : nil } ) end diff --git a/spec/lib/feature/gitaly_spec.rb b/spec/lib/feature/gitaly_spec.rb index 696427bb8b6..6a93d3e3240 100644 --- a/spec/lib/feature/gitaly_spec.rb +++ b/spec/lib/feature/gitaly_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Feature::Gitaly do before do skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check end describe ".enabled?" do diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 3e158391d7f..f08f1dda621 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -123,12 +123,35 @@ RSpec.describe Feature, stub_feature_flags: false do end describe '.enabled?' do - it 'returns false for undefined feature' do - expect(described_class.enabled?(:some_random_feature_flag)).to be_falsey + let(:disabled_ff_definition) do + Feature::Definition.new( + 'development/disabled_feature_flag.yml', + name: 'disabled_feature_flag', + type: 'development', + default_enabled: false + ) end - it 'returns true for undefined feature with default_enabled' do - expect(described_class.enabled?(:some_random_feature_flag, default_enabled: true)).to be_truthy + let(:enabled_ff_definition) do + Feature::Definition.new( + 'development/enabled_feature_flag.yml', + name: 'enabled_feature_flag', + type: 'development', + default_enabled: true + ) + end + + before do + allow(Feature::Definition).to receive(:definitions) do + { + disabled_ff_definition.key => disabled_ff_definition, + enabled_ff_definition.key => enabled_ff_definition + } + end + end + + it 'raises an exception for undefined feature' do + expect { described_class.enabled?(:some_random_feature_flag) }.to raise_error Feature::InvalidFeatureFlagError end it 'returns false for existing disabled feature in the database' do @@ -146,39 +169,58 @@ RSpec.describe Feature, stub_feature_flags: false do it { expect(described_class.send(:l1_cache_backend)).to eq(Gitlab::ProcessMemoryCache.cache_backend) } it { expect(described_class.send(:l2_cache_backend)).to eq(Rails.cache) } - it 'caches the status in L1 and L2 caches', - :request_store, :use_clean_rails_memory_store_caching do + it 'caches the status in L1 and L2 caches', :request_store, :use_clean_rails_memory_store_caching, :aggregate_failures do described_class.enable(:enabled_feature_flag) - flipper_key = "flipper/v1/feature/enabled_feature_flag" + flipper_features_key = 'flipper/v1/features' + flipper_feature_key = 'flipper/v1/feature/enabled_feature_flag' - expect(described_class.send(:l2_cache_backend)) - .to receive(:fetch) - .once - .with(flipper_key, expires_in: 1.hour) - .and_call_original - - expect(described_class.send(:l1_cache_backend)) - .to receive(:fetch) - .once - .with(flipper_key, expires_in: 1.minute) - .and_call_original + allow(described_class.send(:l2_cache_backend)).to receive(:fetch).twice.and_call_original + allow(described_class.send(:l1_cache_backend)).to receive(:fetch).twice.and_call_original 2.times do expect(described_class.enabled?(:enabled_feature_flag)).to be_truthy end + + expect(described_class.send(:l2_cache_backend)).to have_received(:fetch).with(flipper_features_key, expires_in: 1.hour) + expect(described_class.send(:l2_cache_backend)).to have_received(:fetch).with(flipper_feature_key, expires_in: 1.hour) + + expect(described_class.send(:l1_cache_backend)).to have_received(:fetch).with(flipper_features_key, expires_in: 1.minute) + expect(described_class.send(:l1_cache_backend)).to have_received(:fetch).with(flipper_feature_key, expires_in: 1.minute) end - it 'returns the default value when the database does not exist' do - fake_default = double('fake default') + it 'returns the default value when the database does not exist', :aggregate_falures do + a_feature = Feature::Definition.new( + 'development/a_feature.yml', + name: 'a_feature', + type: 'development', + default_enabled: true + ) + + allow(Feature::Definition).to receive(:definitions) do + { a_feature.key => a_feature } + end + expect(ActiveRecord::Base).to receive(:connection) { raise ActiveRecord::NoDatabaseError, "No database" } - expect(described_class.enabled?(:a_feature, default_enabled: fake_default)).to eq(fake_default) + expect(described_class.enabled?(:a_feature)).to eq(true) end - context 'cached feature flag', :request_store do + context 'cached feature flag', :request_store, :use_clean_rails_memory_store_caching, :aggregate_failures do let(:flag) { :some_feature_flag } + let(:some_feature_flag) do + Feature::Definition.new( + "development/#{flag}.yml", + name: flag.to_s, + type: 'development', + default_enabled: true + ) + end before do + allow(Feature::Definition).to receive(:definitions) do + { some_feature_flag.key => some_feature_flag } + end + described_class.send(:flipper).memoize = false described_class.enabled?(flag) end @@ -273,63 +315,48 @@ RSpec.describe Feature, stub_feature_flags: false do .to raise_error(/The `type:` of/) end - it 'when invalid default_enabled is used' do - expect { described_class.enabled?(:my_feature_flag, default_enabled: true) } - .to raise_error(/The `default_enabled:` of/) + it 'reads the default from the YAML definition' do + expect(described_class.enabled?(:my_feature_flag)).to eq(false) end - context 'when `default_enabled: :yaml` is used in code' do - it 'reads the default from the YAML definition' do - expect(described_class.enabled?(:my_feature_flag, default_enabled: :yaml)).to eq(false) - end + context 'when YAML definition does not exist for an optional type' do + let(:optional_type) { described_class::Shared::TYPES.find { |name, attrs| attrs[:optional] }.first } - context 'when default_enabled is true in the YAML definition' do - let(:default_enabled) { true } - - it 'reads the default from the YAML definition' do - expect(described_class.enabled?(:my_feature_flag, default_enabled: :yaml)).to eq(true) + context 'when in dev or test environment' do + it 'raises an error for dev' do + expect { described_class.enabled?(:non_existent_flag, type: optional_type) } + .to raise_error( + Feature::InvalidFeatureFlagError, + "The feature flag YAML definition for 'non_existent_flag' does not exist") end end - context 'when YAML definition does not exist for an optional type' do - let(:optional_type) { described_class::Shared::TYPES.find { |name, attrs| attrs[:optional] }.first } - - context 'when in dev or test environment' do - it 'raises an error for dev' do - expect { described_class.enabled?(:non_existent_flag, type: optional_type, default_enabled: :yaml) } - .to raise_error( - Feature::InvalidFeatureFlagError, - "The feature flag YAML definition for 'non_existent_flag' does not exist") - end + context 'when in production' do + before do + allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(false) end - context 'when in production' do + context 'when database exists' do before do - allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(false) + allow(Gitlab::Database).to receive(:exists?).and_return(true) end - context 'when database exists' do - before do - allow(Gitlab::Database).to receive(:exists?).and_return(true) - end - - it 'checks the persisted status and returns false' do - expect(described_class).to receive(:get).with(:non_existent_flag).and_call_original + it 'checks the persisted status and returns false' do + expect(described_class).to receive(:get).with(:non_existent_flag).and_call_original - expect(described_class.enabled?(:non_existent_flag, type: optional_type, default_enabled: :yaml)).to eq(false) - end + expect(described_class.enabled?(:non_existent_flag, type: optional_type)).to eq(false) end + end - context 'when database does not exist' do - before do - allow(Gitlab::Database).to receive(:exists?).and_return(false) - end + context 'when database does not exist' do + before do + allow(Gitlab::Database).to receive(:exists?).and_return(false) + end - it 'returns false without checking the status in the database' do - expect(described_class).not_to receive(:get) + it 'returns false without checking the status in the database' do + expect(described_class).not_to receive(:get) - expect(described_class.enabled?(:non_existent_flag, type: optional_type, default_enabled: :yaml)).to eq(false) - end + expect(described_class.enabled?(:non_existent_flag, type: optional_type)).to eq(false) end end end @@ -337,13 +364,36 @@ RSpec.describe Feature, stub_feature_flags: false do end end - describe '.disable?' do - it 'returns true for undefined feature' do - expect(described_class.disabled?(:some_random_feature_flag)).to be_truthy + describe '.disabled?' do + let(:disabled_ff_definition) do + Feature::Definition.new( + 'development/disabled_feature_flag.yml', + name: 'disabled_feature_flag', + type: 'development', + default_enabled: false + ) + end + + let(:enabled_ff_definition) do + Feature::Definition.new( + 'development/enabled_feature_flag.yml', + name: 'enabled_feature_flag', + type: 'development', + default_enabled: true + ) + end + + before do + allow(Feature::Definition).to receive(:definitions) do + { + disabled_ff_definition.key => disabled_ff_definition, + enabled_ff_definition.key => enabled_ff_definition + } + end end - it 'returns false for undefined feature with default_enabled' do - expect(described_class.disabled?(:some_random_feature_flag, default_enabled: true)).to be_falsey + it 'raises an exception for undefined feature' do + expect { described_class.disabled?(:some_random_feature_flag) }.to raise_error Feature::InvalidFeatureFlagError end it 'returns true for existing disabled feature in the database' do diff --git a/spec/lib/gitlab/gon_helper_spec.rb b/spec/lib/gitlab/gon_helper_spec.rb index 3d3f381b6d2..694e81d6f82 100644 --- a/spec/lib/gitlab/gon_helper_spec.rb +++ b/spec/lib/gitlab/gon_helper_spec.rb @@ -12,6 +12,7 @@ RSpec.describe Gitlab::GonHelper do describe '#push_frontend_feature_flag' do before do skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check end it 'pushes a feature flag to the frontend' do diff --git a/spec/lib/gitlab/metrics/methods_spec.rb b/spec/lib/gitlab/metrics/methods_spec.rb index 71135a6e9c5..31b67a1e4d3 100644 --- a/spec/lib/gitlab/metrics/methods_spec.rb +++ b/spec/lib/gitlab/metrics/methods_spec.rb @@ -102,6 +102,11 @@ RSpec.describe Gitlab::Metrics::Methods do let(:feature_name) { :some_metric_feature } let(:metric) { call_fetch_metric_method(docstring: docstring, with_feature: feature_name) } + before do + skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check + end + context 'when feature is enabled' do before do stub_feature_flags(feature_name => true) diff --git a/spec/lib/gitlab/metrics_spec.rb b/spec/lib/gitlab/metrics_spec.rb index db5a23e2328..e83adf5bf67 100644 --- a/spec/lib/gitlab/metrics_spec.rb +++ b/spec/lib/gitlab/metrics_spec.rb @@ -54,6 +54,11 @@ RSpec.describe Gitlab::Metrics do end describe '.measure' do + before do + skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check + end + context 'without a transaction' do it 'returns the return value of the block' do val = described_class.measure(:foo) { 10 } diff --git a/spec/lib/gitlab/repository_cache_adapter_spec.rb b/spec/lib/gitlab/repository_cache_adapter_spec.rb index 625dcf11546..d14c3f44c6f 100644 --- a/spec/lib/gitlab/repository_cache_adapter_spec.rb +++ b/spec/lib/gitlab/repository_cache_adapter_spec.rb @@ -29,10 +29,19 @@ RSpec.describe Gitlab::RepositoryCacheAdapter do def project end + + def cached_methods + [:letters] + end + + def exists? + true + end end end let(:fake_repository) { klass.new } + let(:redis_set_cache) { fake_repository.redis_set_cache } context 'with an existing repository' do it 'caches the output, sorting the results' do @@ -42,47 +51,43 @@ RSpec.describe Gitlab::RepositoryCacheAdapter do expect(fake_repository.letters).to eq(%w(a b c)) end - expect(fake_repository.redis_set_cache.exist?(:letters)).to eq(true) + expect(redis_set_cache.exist?(:letters)).to eq(true) expect(fake_repository.instance_variable_get(:@letters)).to eq(%w(a b c)) end context 'membership checks' do context 'when the cache key does not exist' do it 'calls the original method and populates the cache' do - expect(fake_repository.redis_set_cache.exist?(:letters)).to eq(false) + expect(redis_set_cache.exist?(:letters)).to eq(false) expect(fake_repository).to receive(:_uncached_letters).once.and_call_original # This populates the cache and memoizes the full result expect(fake_repository.letters_include?('a')).to eq(true) expect(fake_repository.letters_include?('d')).to eq(false) - expect(fake_repository.redis_set_cache.exist?(:letters)).to eq(true) + expect(redis_set_cache.exist?(:letters)).to eq(true) end end context 'when the cache key exists' do before do - fake_repository.redis_set_cache.write(:letters, %w(b a c)) + redis_set_cache.write(:letters, %w(b a c)) end - it 'calls #include? on the set cache' do - expect(fake_repository.redis_set_cache) - .to receive(:include?).with(:letters, 'a').and_call_original - expect(fake_repository.redis_set_cache) - .to receive(:include?).with(:letters, 'd').and_call_original + it 'calls #try_include? on the set cache' do + expect(redis_set_cache).to receive(:try_include?).with(:letters, 'a').and_call_original + expect(redis_set_cache).to receive(:try_include?).with(:letters, 'd').and_call_original expect(fake_repository.letters_include?('a')).to eq(true) expect(fake_repository.letters_include?('d')).to eq(false) end it 'memoizes the result' do - expect(fake_repository.redis_set_cache) - .to receive(:include?).once.and_call_original + expect(redis_set_cache).to receive(:try_include?).once.and_call_original expect(fake_repository.letters_include?('a')).to eq(true) expect(fake_repository.letters_include?('a')).to eq(true) - expect(fake_repository.redis_set_cache) - .to receive(:include?).once.and_call_original + expect(redis_set_cache).to receive(:try_include?).once.and_call_original expect(fake_repository.letters_include?('d')).to eq(false) expect(fake_repository.letters_include?('d')).to eq(false) diff --git a/spec/lib/gitlab/repository_set_cache_spec.rb b/spec/lib/gitlab/repository_set_cache_spec.rb index 07f4d7c462d..6bdcc5ea2b7 100644 --- a/spec/lib/gitlab/repository_set_cache_spec.rb +++ b/spec/lib/gitlab/repository_set_cache_spec.rb @@ -132,4 +132,15 @@ RSpec.describe Gitlab::RepositorySetCache, :clean_gitlab_redis_cache do expect(cache.include?(:foo, 'bar')).to be(false) end end + + describe '#try_include?' do + it 'checks existence of the redis set and inclusion' do + expect(cache.try_include?(:foo, 'value')).to eq([false, false]) + + cache.write(:foo, ['value']) + + expect(cache.try_include?(:foo, 'value')).to eq([true, true]) + expect(cache.try_include?(:foo, 'bar')).to eq([false, true]) + end + end end diff --git a/spec/requests/api/features_spec.rb b/spec/requests/api/features_spec.rb index 0e163ec2154..04c2758f0e9 100644 --- a/spec/requests/api/features_spec.rb +++ b/spec/requests/api/features_spec.rb @@ -438,6 +438,8 @@ RSpec.describe API::Features, stub_feature_flags: false do context 'when the gate value was set' do before do + skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check Feature.enable(feature_name) end diff --git a/spec/requests/api/usage_data_spec.rb b/spec/requests/api/usage_data_spec.rb index d44f179eed8..0399b316b43 100644 --- a/spec/requests/api/usage_data_spec.rb +++ b/spec/requests/api/usage_data_spec.rb @@ -73,6 +73,7 @@ RSpec.describe API::UsageData do context 'with unknown event' do before do skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check end it 'returns status ok' do @@ -149,6 +150,7 @@ RSpec.describe API::UsageData do context 'with unknown event' do before do skip_feature_flags_yaml_validation + skip_default_enabled_yaml_check end it 'returns status ok' do |