Merge branch 'ml-run-package-and-qa-on-feature-flag-config-changes' into 'master'

Run package-and-qa on feature flag config changes

See merge request gitlab-org/gitlab!68814

.gitlab/ci/qa.gitlab-ci.yml

@@ -57,12 +57,7 @@ update-qa-cache:
     - install_gitlab_gem
   script:
     - ./scripts/trigger-build omnibus
-
-package-and-qa:
-  extends:
-    - .package-and-qa-base
-    - .qa:rules:package-and-qa
-  # This job often times out, so temporarily use private runners and a long timeout: https://gitlab.com/gitlab-org/gitlab/-/issues/238563
+  # These jobs often time out, so temporarily use private runners and a long timeout: https://gitlab.com/gitlab-org/gitlab/-/issues/238563
   tags:
     - prm
   timeout: 4h
@@ -71,3 +66,34 @@ package-and-qa:
       artifacts: false
     - job: build-assets-image
       artifacts: false
+
+.package-and-qa-ff-base:
+  needs:
+    - detect-tests
+  variables:
+    CHANGED_FILES: tmp/changed_files.txt
+  script:
+    - export GITLAB_QA_OPTIONS="--set-feature-flags $(scripts/changed-feature-flags --files $(cat $CHANGED_FILES | tr ' ' ',') --state $QA_FF_STATE)"
+    - echo $GITLAB_QA_OPTIONS
+    - ./scripts/trigger-build omnibus
+
+package-and-qa:
+  extends:
+    - .package-and-qa-base
+    - .qa:rules:package-and-qa
+
+package-and-qa-ff-enabled:
+  extends:
+    - .package-and-qa-base
+    - .package-and-qa-ff-base
+    - .qa:rules:package-and-qa:feature-flags
+  variables:
+    QA_FF_STATE: "enable"
+
+package-and-qa-ff-disabled:
+  extends:
+    - .package-and-qa-base
+    - .package-and-qa-ff-base
+    - .qa:rules:package-and-qa:feature-flags
+  variables:
+    QA_FF_STATE: "disable"

.gitlab/ci/rules.gitlab-ci.yml

@@ -381,6 +381,9 @@
   - "config/helpers/**/*.js"
   - "vendor/assets/javascripts/**/*"
 
+.feature-flag-config-patterns: &feature-flag-config-patterns
+  - "{,ee/}config/feature_flags/**/*.yml"
+
 ################
 # Shared rules #
 ################
@@ -682,6 +685,9 @@
   rules:
     - <<: *if-not-ee
       when: never
+    - <<: *if-dot-com-gitlab-org-and-security-merge-request
+      changes: *feature-flag-config-patterns
+      when: never
     - <<: *if-dot-com-gitlab-org-and-security-merge-request
       changes: *ci-qa-patterns
       allow_failure: true
@@ -695,6 +701,14 @@
     - <<: *if-dot-com-gitlab-org-schedule
       allow_failure: true
 
+.qa:rules:package-and-qa:feature-flags:
+  rules:
+    - <<: *if-not-ee
+      when: never
+    - <<: *if-dot-com-gitlab-org-and-security-merge-request
+      changes: *feature-flag-config-patterns
+      allow_failure: true
+
 ###############
 # Rails rules #
 ###############

doc/development/testing_guide/end_to_end/feature_flags.md

@@ -79,11 +79,21 @@ for details.
 
 End-to-end tests should pass with a feature flag enabled before it is enabled on Staging or on GitLab.com. Tests that need to be updated should be identified as part of [quad-planning](https://about.gitlab.com/handbook/engineering/quality/quad-planning/). The relevant [counterpart Software Engineer in Test](https://about.gitlab.com/handbook/engineering/quality/#individual-contributors) is responsible for updating the tests or assisting another engineer to do so. However, if a change does not go through quad-planning and a required test update is not made, test failures could block deployment.
 
-If a test enables a feature flag as describe above, it is sufficient to run the `package-and-qa` job in a merge request containing the relevant changes.
-Or, if the feature flag and relevant changes have already been merged, you can confirm that the tests
-pass on `main`. The end-to-end tests run on `main` every two hours, and the results are posted to a [Test
-Session Report, which is available in the testcase-sessions project](https://gitlab.com/gitlab-org/quality/testcase-sessions/-/issues?label_name%5B%5D=found%3Amaster).
+### Automatic test execution when a feature flag definition changes
 
-If the relevant tests do not enable the feature flag themselves, you can check if the tests will need
-to be updated by opening a draft merge request that enables the flag by default and then running the `package-and-qa` job.
+If a merge request adds or edits a [feature flag definition file](../../feature_flags/index.md#feature-flag-definition-and-validation),
+two `package-and-qa` jobs will be included automatically in the merge request pipeline. One job will enable the defined
+feature flag and the other will disable it. The jobs execute the same suite of tests to confirm that they pass with if
+the feature flag is either enabled or disabled.
+
+### Test execution during feature development
+
+If an end-to-end test enables a feature flag, the end-to-end test suite can be used to test changes in a merge request
+by running the `package-and-qa` job in the merge request pipeline. If the feature flag and relevant changes have already been merged, you can confirm that the tests
+pass on the default branch. The end-to-end tests run on the default branch every two hours, and the results are posted to a [Test
+Session Report, which is available in the testcase-sessions project](https://gitlab.com/gitlab-org/quality/testcase-sessions/-/issues?label_name%5B%5D=found%3Amain).
+
+If the relevant tests do not enable the feature flag themselves, you can check if the tests will need to be updated by opening
+a draft merge request that enables the flag by default via a [feature flag definition file](../../feature_flags/index.md#feature-flag-definition-and-validation).
+That will [automatically execute the end-to-end test suite](#automatic-test-execution-when-a-feature-flag-definition-changes).
 The merge request can be closed once the tests pass. If you need assistance to update the tests, please contact the relevant [stable counterpart in the Quality department](https://about.gitlab.com/handbook/engineering/quality/#individual-contributors), or any Software Engineer in Test if there is no stable counterpart for your group.

qa/qa/runtime/feature.rb

@@ -5,15 +5,16 @@ require 'active_support/core_ext/object/blank'
 module QA
   module Runtime
     class Feature
+      SetFeatureError = Class.new(RuntimeError)
+      AuthorizationError = Class.new(RuntimeError)
+      UnknownScopeError = Class.new(RuntimeError)
+      UnknownStateError = Class.new(RuntimeError)
+
       class << self
         # Documentation: https://docs.gitlab.com/ee/api/features.html
 
         include Support::API
 
-        SetFeatureError = Class.new(RuntimeError)
-        AuthorizationError = Class.new(RuntimeError)
-        UnknownScopeError = Class.new(RuntimeError)
-
         def remove(key)
           request = Runtime::API::Request.new(api_client, "/features/#{key}")
           response = delete(request.url)
@@ -30,6 +31,23 @@ module QA
           set_and_verify(key, enable: false, **scopes)
         end
 
+        # Set one or more flags to their specified state.
+        #
+        # @param [Hash] flags The feature flags and desired values, e.g., { 'flag1' => 'enabled', 'flag2' => "disabled" }
+        # @param [Hash] scopes The scope (user, project, group) to apply the feature flag to.
+        def set(flags, **scopes)
+          flags.each_pair do |flag, state|
+            case state
+            when 'enabled', 'enable', 'true', 1, true
+              enable(flag, **scopes)
+            when 'disabled', 'disable', 'false', 0, false
+              disable(flag, **scopes)
+            else
+              raise UnknownStateError, "Unknown feature flag state: #{state}"
+            end
+          end
+        end
+
         def enabled?(key, **scopes)
           feature = JSON.parse(get_features).find { |flag| flag['name'] == key.to_s }
           feature && (feature['state'] == 'on' || feature['state'] == 'conditional' && scopes.present? && enabled_scope?(feature['gates'], **scopes))
@@ -47,15 +65,15 @@ module QA
           scopes.each do |key, value|
             case key
             when :project, :group, :user
-              actors = gates.filter { |i| i['key'] == 'actors' }.first['value']
-              break actors.include?("#{key.to_s.capitalize}:#{value.id}")
+              actors = gates.find { |i| i['key'] == 'actors' }['value']
+              return actors.include?("#{key.to_s.capitalize}:#{value.id}")
             when :feature_group
-              groups = gates.filter { |i| i['key'] == 'groups' }.first['value']
-              break groups.include?(value)
-            else
-              raise UnknownScopeError, "Unknown scope: #{key}"
+              groups = gates.find { |i| i['key'] == 'groups' }['value']
+              return groups.include?(value)
             end
           end
+
+          raise UnknownScopeError, "Unknown scope in: #{scopes}"
         end
 
         def get_features

qa/qa/scenario/bootable.rb

@@ -17,6 +17,22 @@ module QA
 
           arguments = OptionParser.new do |parser|
             options.to_a.each do |opt|
+              # The argument for the --set-feature-flags option should look something like "flag1=enabled,flag2=disabled"
+              # Here we translate that string into a hash, e.g.: { 'flag1' => 'enabled', 'flag2' => "disabled" }
+              if opt.name == :set_feature_flags
+                parser.on(opt.arg, opt.desc) do |flags|
+                  value = flags.split(',').each_with_object({}) do |pair, hash|
+                    flag_name, flag_value = pair.split('=')
+                    raise '--set-feature-flags requires flag name and flag state for each flag, e.g., flag1=enabled,flag2=disabled' unless flag_name && flag_value
+
+                    hash[flag_name] = flag_value
+                  end
+                  Runtime::Scenario.define(opt.name, value)
+                end
+
+                next
+              end
+
               parser.on(opt.arg, opt.desc) do |value|
                 Runtime::Scenario.define(opt.name, value)
               end

qa/qa/scenario/shared_attributes.rb

@@ -8,6 +8,9 @@ module QA
       attribute :gitlab_address, '--address URL', 'Address of the instance to test'
       attribute :enable_feature, '--enable-feature FEATURE_FLAG', 'Enable a feature before running tests'
       attribute :disable_feature, '--disable-feature FEATURE_FLAG', 'Disable a feature before running tests'
+      attribute :set_feature_flags, '--set-feature-flags FEATURE_FLAGS',
+        'Set one or more feature flags before running tests. ' \
+        'Specify FEATURE_FLAGS as comma-separated flag=state pairs, e.g., "flag1=enabled,flag2=disabled"'
       attribute :parallel, '--parallel', 'Execute tests in parallel'
       attribute :loop, '--loop', 'Execute test repeatedly'
     end

qa/qa/scenario/template.rb

@@ -38,8 +38,8 @@ module QA
         Runtime::Release.perform_before_hooks
 
         Runtime::Feature.enable(options[:enable_feature]) if options.key?(:enable_feature)
-
         Runtime::Feature.disable(options[:disable_feature]) if options.key?(:disable_feature) && (@feature_enabled = Runtime::Feature.enabled?(options[:disable_feature]))
+        Runtime::Feature.set(options[:set_feature_flags]) if options.key?(:set_feature_flags)
 
         Specs::Runner.perform do |specs|
           specs.tty = true

qa/spec/runtime/feature_spec.rb

@@ -175,6 +175,20 @@ RSpec.describe QA::Runtime::Feature do
         expect(described_class.enabled?(feature_flag)).to be_truthy
       end
 
+      it 'raises an error when the scope is unknown' do
+        expect(QA::Runtime::API::Request)
+          .to receive(:new)
+          .with(api_client, "/features")
+          .and_return(request)
+        expect(described_class)
+          .to receive(:get)
+          .and_return(
+            Struct.new(:code, :body)
+                  .new(200, %([{ "name": "a_flag", "state": "conditional", "gates": { "key": "groups", "value": ["foo"] } }])))
+
+        expect { described_class.enabled?(feature_flag, scope: 'foo') }.to raise_error(QA::Runtime::Feature::UnknownScopeError)
+      end
+
       context 'when a project scope is provided' do
         it_behaves_like 'checks a feature flag' do
           let(:scope) { :project }
@@ -212,4 +226,38 @@ RSpec.describe QA::Runtime::Feature do
       end
     end
   end
+
+  describe '.set' do
+    let(:scope) { { scope: 'actor' } }
+
+    it 'raises an error when the flag state is unknown' do
+      expect(described_class).not_to receive(:enable)
+      expect(described_class).not_to receive(:disable)
+
+      expect { described_class.set({ foo: 'bar' }, **scope) }.to raise_error(QA::Runtime::Feature::UnknownStateError, 'Unknown feature flag state: bar')
+    end
+
+    it 'enables feature flags' do
+      expect(described_class).to receive(:enable).with(:flag1, scope)
+      expect(described_class).to receive(:enable).with(:flag2, scope)
+      expect(described_class).not_to receive(:disable)
+
+      described_class.set({ flag1: 'enabled', flag2: 'enable' }, **scope)
+    end
+
+    it 'disables feature flags' do
+      expect(described_class).to receive(:disable).with(:flag1, scope)
+      expect(described_class).to receive(:disable).with(:flag2, scope)
+      expect(described_class).not_to receive(:enable)
+
+      described_class.set({ flag1: 'disable', flag2: 'disable' }, **scope)
+    end
+
+    it 'enables and disables feature flags' do
+      expect(described_class).to receive(:enable).with(:flag1, scope)
+      expect(described_class).to receive(:disable).with(:flag2, scope)
+
+      described_class.set({ flag1: 'enabled', flag2: 'disabled' }, **scope)
+    end
+  end
 end

scripts/changed-feature-flags

+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+require 'yaml'
+require 'optparse'
+require_relative 'api/default_options'
+
+# This script returns the desired feature flag state as a comma-separated string for the feature flags in the specified files.
+# Each desired feature flag state is specified as 'feature-flag=state'.
+#
+# For example, if the specified files included `config/feature_flags/development/ci_yaml_limit_size.yml` and the desired
+# state as specified by the second argument was enabled, the value returned would be `ci_yaml_limit_size=enabled`
+
+class GetFeatureFlagsFromFiles
+  def initialize(options)
+    @files = options.delete(:files)
+    @state = options.delete(:state)
+  end
+
+  def extracted_flags
+    files.each_with_object([]) do |file_path, all|
+      next unless file_path =~ %r{/feature_flags/development/.*\.yml}
+      next unless File.exist?(file_path)
+
+      ff_yaml = YAML.safe_load(File.read(file_path))
+      ff_to_add = "#{ff_yaml['name']}"
+      ff_to_add += "=#{state}" unless state.to_s.empty?
+
+      all << ff_to_add
+    end.join(',')
+  end
+
+  private
+
+  attr_reader :files, :state
+end
+
+if $0 == __FILE__
+  options = API::DEFAULT_OPTIONS.dup
+
+  OptionParser.new do |opts|
+    opts.on("-f", "--files FILES", Array, "Comma-separated list of feature flag config files") do |value|
+      options[:files] = value
+    end
+
+    opts.on("-s", "--state STATE", String,
+      "The desired state of the feature flags (enabled or disabled). If not specified the output will only list the feature flags."
+    ) do |value|
+      options[:state] = value
+    end
+
+    opts.on("-h", "--help", "Prints this help") do
+      puts opts
+      exit
+    end
+  end.parse!
+
+  puts GetFeatureFlagsFromFiles.new(options).extracted_flags
+end

scripts/trigger-build

@@ -154,7 +154,8 @@ module Trigger
         'SECURITY_SOURCES' => Trigger.security? ? 'true' : 'false',
         'ee' => Trigger.ee? ? 'true' : 'false',
         'QA_BRANCH' => ENV['QA_BRANCH'] || 'master',
-        'CACHE_UPDATE' => ENV['OMNIBUS_GITLAB_CACHE_UPDATE']
+        'CACHE_UPDATE' => ENV['OMNIBUS_GITLAB_CACHE_UPDATE'],
+        'GITLAB_QA_OPTIONS' => ENV['GITLAB_QA_OPTIONS']
       }
     end
   end

spec/scripts/changed-feature-flags_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+load File.expand_path('../../scripts/changed-feature-flags', __dir__)
+
+RSpec.describe 'scripts/changed-feature-flags' do
+  describe GetFeatureFlagsFromFiles do
+    let(:ff_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, 'feature_flags', 'development')) }
+
+    let(:feature_flag_definition1) do
+      file = Tempfile.new('foo.yml', ff_dir)
+      file.write(<<~YAML)
+        ---
+        name: foo_flag
+        default_enabled: true
+      YAML
+      file.rewind
+      file
+    end
+
+    let(:feature_flag_definition2) do
+      file = Tempfile.new('bar.yml', ff_dir)
+      file.write(<<~YAML)
+        ---
+        name: bar_flag
+        default_enabled: false
+      YAML
+      file.rewind
+      file
+    end
+
+    let(:feature_flag_definition_invalid_path) do
+      file = Tempfile.new('foobar.yml')
+      file.write(<<~YAML)
+        ---
+        name: not a feature flag
+      YAML
+      file.rewind
+      file
+    end
+
+    after do
+      FileUtils.remove_entry(ff_dir, true)
+    end
+
+    describe '.extracted_flags' do
+      it 'returns feature flags' do
+        subject = described_class.new({ files: [feature_flag_definition1.path, feature_flag_definition2.path] })
+
+        expect(subject.extracted_flags).to eq('foo_flag,bar_flag')
+      end
+
+      it 'returns feature flags and their state as enabled' do
+        subject = described_class.new({ files: [feature_flag_definition1.path, feature_flag_definition2.path], state: 'enabled' })
+
+        expect(subject.extracted_flags).to eq('foo_flag=enabled,bar_flag=enabled')
+      end
+
+      it 'returns feature flags and their state as disabled' do
+        subject = described_class.new({ files: [feature_flag_definition1.path, feature_flag_definition2.path], state: 'disabled' })
+
+        expect(subject.extracted_flags).to eq('foo_flag=disabled,bar_flag=disabled')
+      end
+
+      it 'ignores files that are not in the feature_flags/development directory' do
+        subject = described_class.new({ files: [feature_flag_definition_invalid_path.path] })
+
+        expect(subject.extracted_flags).to eq('')
+      end
+    end
+  end
+end

spec/tooling/quality/test_level_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe Quality::TestLevel do
     context 'when level is unit' do
       it 'returns a pattern' do
         expect(subject.pattern(:unit))
-          .to eq("spec/{bin,channels,config,db,dependencies,elastic,elastic_integration,experiments,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,models,policies,presenters,rack_servers,replicators,routing,rubocop,serializers,services,sidekiq,spam,support_specs,tasks,uploaders,validators,views,workers,tooling}{,/**/}*_spec.rb")
+          .to eq("spec/{bin,channels,config,db,dependencies,elastic,elastic_integration,experiments,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,models,policies,presenters,rack_servers,replicators,routing,rubocop,scripts,serializers,services,sidekiq,spam,support_specs,tasks,uploaders,validators,views,workers,tooling}{,/**/}*_spec.rb")
       end
     end
 
@@ -110,7 +110,7 @@ RSpec.describe Quality::TestLevel do
     context 'when level is unit' do
       it 'returns a regexp' do
         expect(subject.regexp(:unit))
-          .to eq(%r{spec/(bin|channels|config|db|dependencies|elastic|elastic_integration|experiments|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|models|policies|presenters|rack_servers|replicators|routing|rubocop|serializers|services|sidekiq|spam|support_specs|tasks|uploaders|validators|views|workers|tooling)})
+          .to eq(%r{spec/(bin|channels|config|db|dependencies|elastic|elastic_integration|experiments|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|models|policies|presenters|rack_servers|replicators|routing|rubocop|scripts|serializers|services|sidekiq|spam|support_specs|tasks|uploaders|validators|views|workers|tooling)})
       end
     end
 

tooling/quality/test_level.rb

@@ -40,6 +40,7 @@ module Quality
         replicators
         routing
         rubocop
+        scripts
         serializers
         services
         sidekiq