diff --git a/.gitlab/ci/qa.gitlab-ci.yml b/.gitlab/ci/qa.gitlab-ci.yml index 93cd43fbdee61ce0b7004d64875bbeb065a8c531..b7dc937f0c6a846ba8d012f114b6fd6d221c9ef5 100644 --- a/.gitlab/ci/qa.gitlab-ci.yml +++ b/.gitlab/ci/qa.gitlab-ci.yml @@ -115,13 +115,13 @@ update-qa-cache: .package-and-qa-ff-base: script: - | - feature_flags=$(scripts/changed-feature-flags --files $(cat $CHANGES_FILE | tr ' ' ',') --state $QA_FF_STATE) + feature_flags=$(scripts/changed-feature-flags --files $CHANGES_DIFFS_DIR --state $QA_FF_STATE) if [[ $feature_flags ]]; then export GITLAB_QA_OPTIONS="--set-feature-flags $feature_flags" echo $GITLAB_QA_OPTIONS ./scripts/trigger-build.rb omnibus else - echo "No changed feature flag found to test. The tests are skipped if the flag was removed." + echo "No changed feature flag found to test as $QA_FF_STATE." fi package-and-qa: @@ -135,7 +135,7 @@ package-and-qa-ff-enabled: - .package-and-qa-ff-base - .qa:rules:package-and-qa:feature-flags variables: - QA_FF_STATE: "enable" + QA_FF_STATE: "enabled" package-and-qa-ff-disabled: extends: @@ -143,4 +143,12 @@ package-and-qa-ff-disabled: - .package-and-qa-ff-base - .qa:rules:package-and-qa:feature-flags variables: - QA_FF_STATE: "disable" + QA_FF_STATE: "disabled" + +package-and-qa-ff-deleted: + extends: + - .package-and-qa-base + - .package-and-qa-ff-base + - .qa:rules:package-and-qa:feature-flags + variables: + QA_FF_STATE: "deleted" diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index d7deaa90b13fd2e42dcd37160857f5a2ad0a9769..7fd7a02873d9b7b62424bb5d2ae66a7b1351f9b8 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -900,9 +900,6 @@ rules: - <<: *if-not-ee when: never - - <<: *if-dot-com-gitlab-org-and-security-merge-request - changes: *feature-flag-development-config-patterns - when: never - <<: *if-merge-request-targeting-stable-branch allow_failure: true - <<: *if-dot-com-gitlab-org-and-security-merge-request diff --git a/qa/qa/runtime/feature.rb b/qa/qa/runtime/feature.rb index ec28813c1f6c8abfb61c2e6469ce9e409ce23510..93e215e96358cd3ef37d85b890eaeb9b13115786 100644 --- a/qa/qa/runtime/feature.rb +++ b/qa/qa/runtime/feature.rb @@ -42,6 +42,8 @@ def set(flags, **scopes) enable(flag, **scopes) when 'disabled', 'disable', 'false', 0, false disable(flag, **scopes) + when 'deleted' + QA::Runtime::Logger.info("Feature flag definition for '#{flag}' was deleted. The state of the feature flag has not been changed.") else raise UnknownStateError, "Unknown feature flag state: #{state}" end diff --git a/scripts/changed-feature-flags b/scripts/changed-feature-flags index 3a4f18bd78f838b39a9e149bb207235ea59f4b8c..86214e867880646903d630ae0a520f462ad0f2ea 100755 --- a/scripts/changed-feature-flags +++ b/scripts/changed-feature-flags @@ -3,10 +3,12 @@ require 'yaml' require 'optparse' +require 'pathname' 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'. +# Each desired feature flag state is specified as 'feature-flag=state'. This allows us to run package-and-qa with the +# feature flag set to the desired 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` @@ -15,31 +17,84 @@ class GetFeatureFlagsFromFiles def initialize(options) @files = options.delete(:files) @state = options.delete(:state) + + abort("ERROR: Please specify the directory containing MR diffs.") if @files.to_s.empty? end + # Gets feature flags from definition files or diffs of deleted defition files + # + # @return [String] a comma-separated list of feature flags and their desired state def extracted_flags - files.each_with_object([]) do |file_path, all| - next unless file_path =~ %r{/feature_flags/(development|ops)/.*\.yml} - next unless File.exist?(file_path) + flags_list = diffs_dir.glob('**/*').each_with_object([]) do |file_path, flags| + ff_yaml = ff_yaml_for_file(file_path) + next if ff_yaml.nil? + break [] if ff_yaml.empty? + + flags << ff_yaml['name'] + end + flags_list = flags_list.map { |flag| "#{flag}=#{state}" } unless state.to_s.empty? + flags_list.join(',') + end - ff_yaml = YAML.safe_load(File.read(file_path)) - ff_to_add = "#{ff_yaml['name']}" - ff_to_add += "=#{state}" unless state.to_s.empty? + # Loads the YAML feature flag definition based on a diff of the definition file. The definition is loaded from the + # definition file itself, or from a diff of the deleted definition file. + # + # @param [Pathname] path the path to the diff + # @return [Hash] a hash containing the YAML data for the feature flag definition + def ff_yaml_for_file(path) + return unless File.expand_path(path).to_s =~ %r{/feature_flags/(development|ops)/.*\.yml} - all << ff_to_add - end.join(',') + if path.to_s.end_with?('yml.deleted.diff') + # Ignore deleted feature flag definitions if we want to enable/disable existing flags. + return if state != 'deleted' + + yaml_from_deleted_diff(path) + else + # If we want deleted definition files but find one that wasn't deleted, we return immediately to + # because non-deleted flags are tested in separate jobs from deleted flags, so we don't need to run + # a job with just deleted flags. + return [] if state == 'deleted' + + yaml_from_file(path, diffs_dir) + end end private attr_reader :files, :state + + # The absolute path to the directory of diffs + # + # @return [String] + def diffs_dir + @diffs_dir ||= Pathname.new(files).expand_path + end + + # Loads the YAML feature flag definition from a file corresponding to a diff of the definition file. + # + # @param [Pathname] file_path the path to the diff + # @param [Pathname] diffs_dir the path to the diffs directory + # @return [Hash] a hash containing the YAML data from the feature flag definition file corresponding to the diff + def yaml_from_file(file_path, diffs_dir) + real_file_path = File.join(Dir.pwd, file_path.to_s.delete_prefix(diffs_dir.to_s)).delete_suffix('.diff') + YAML.safe_load(File.read(real_file_path)) + end + + # Loads the YAML feature flag definition from a diff of the deleted feature flag definition file. + # + # @param [Pathname] file_path the path of the diff + # @return [Hash] a hash containing the YAML data for the feature flag definition from the diff + def yaml_from_deleted_diff(file_path) + cleaned_diff = File.read(file_path).gsub(/^[^a-z]+/, '') + YAML.safe_load(cleaned_diff) + end 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| + opts.on("-f", "--files FILES", String, "A directory containing diffs including feature flag definition change diffs") do |value| options[:files] = value end diff --git a/spec/scripts/changed-feature-flags_spec.rb b/spec/scripts/changed-feature-flags_spec.rb index 5c858588c0cb8890a5f7b84bf26dffbbafb59608..bbae49a90e4b88bdde0a3dd61c9b2dfbfb82a846 100644 --- a/spec/scripts/changed-feature-flags_spec.rb +++ b/spec/scripts/changed-feature-flags_spec.rb @@ -6,8 +6,8 @@ RSpec.describe 'scripts/changed-feature-flags' do describe GetFeatureFlagsFromFiles do - let(:feature_flag_definition1) do - file = Tempfile.new('foo.yml', ff_dir) + let!(:feature_flag_definition1) do + file = File.open(File.join(ff_dir, "#{file_name1}.yml"), 'w+') file.write(<<~YAML) --- name: foo_flag @@ -17,8 +17,8 @@ file end - let(:feature_flag_definition2) do - file = Tempfile.new('bar.yml', ff_dir) + let!(:feature_flag_definition2) do + file = File.open(File.join(ff_dir, "#{file_name2}.yml"), 'w+') file.write(<<~YAML) --- name: bar_flag @@ -28,48 +28,136 @@ file end + let!(:feature_flag_diff1) do + FileUtils.mkdir_p(File.join(diffs_dir, ff_sub_dir)) + file = File.open(File.join(diffs_dir, ff_sub_dir, "#{file_name1}.yml.diff"), 'w+') + file.write(<<~YAML) + @@ -5,4 +5,4 @@ + name: foo_flag + -default_enabled: false + +default_enabled: true + YAML + file.rewind + file + end + + let!(:feature_flag_diff2) do + FileUtils.mkdir_p(File.join(diffs_dir, ff_sub_dir)) + file = File.open(File.join(diffs_dir, ff_sub_dir, "#{file_name2}.yml.diff"), 'w+') + file.write(<<~YAML) + @@ -0,0 +0,0 @@ + name: bar_flag + -default_enabled: true + +default_enabled: false + YAML + file.rewind + file + end + + let!(:deleted_feature_flag_diff) do + FileUtils.mkdir_p(File.join(diffs_dir, ff_sub_dir)) + file = File.open(File.join(diffs_dir, ff_sub_dir, "foobar_ff_#{SecureRandom.hex(8)}.yml.deleted.diff"), 'w+') + file.write(<<~YAML) + @@ -0,0 +0,0 @@ + -name: foobar_flag + -default_enabled: true + YAML + file.rewind + file + end + + before do + allow(Dir).to receive(:pwd).and_return(Dir.tmpdir) + end + after do - FileUtils.remove_entry(ff_dir, true) + feature_flag_definition1.close + feature_flag_definition2.close + feature_flag_diff1.close + feature_flag_diff2.close + deleted_feature_flag_diff.close + FileUtils.rm_r(ff_dir) + FileUtils.rm_r(diffs_dir) end describe '.extracted_flags' do + let(:file_name1) { "foo_ff_#{SecureRandom.hex(8)}"} + let(:file_name2) { "bar_ff_#{SecureRandom.hex(8)}"} + let(:ff_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, ff_sub_dir)) } + let(:diffs_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, 'diffs')).first } + shared_examples 'extract feature flags' do it 'returns feature flags on their own' do - subject = described_class.new({ files: [feature_flag_definition1.path, feature_flag_definition2.path] }) + subject = described_class.new({ files: diffs_dir }) - expect(subject.extracted_flags).to eq('foo_flag,bar_flag') + expect(subject.extracted_flags.split(',')).to include('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' }) + subject = described_class.new({ files: diffs_dir, state: 'enabled' }) - expect(subject.extracted_flags).to eq('foo_flag=enabled,bar_flag=enabled') + expect(subject.extracted_flags.split(',')).to include('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' }) + subject = described_class.new({ files: diffs_dir, state: 'disabled' }) + + expect(subject.extracted_flags.split(',')).to include('foo_flag=disabled', 'bar_flag=disabled') + end + + it 'does not return feature flags when there are mixed deleted and non-deleted definition files' do + subject = described_class.new({ files: diffs_dir, state: 'deleted' }) - expect(subject.extracted_flags).to eq('foo_flag=disabled,bar_flag=disabled') + expect(subject.extracted_flags).to eq('') end end context 'with definition files in the development directory' do - let(:ff_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, 'feature_flags', 'development')) } + let(:ff_sub_dir) { %w[feature_flags development] } it_behaves_like 'extract feature flags' end context 'with definition files in the ops directory' do - let(:ff_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, 'feature_flags', 'ops')) } + let(:ff_sub_dir) { %w[feature_flags ops] } it_behaves_like 'extract feature flags' end context 'with definition files in the experiment directory' do - let(:ff_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, 'feature_flags', 'experiment')) } + let(:ff_sub_dir) { %w[feature_flags experiment] } it 'ignores the files' do - subject = described_class.new({ files: [feature_flag_definition1.path, feature_flag_definition2.path] }) + subject = described_class.new({ files: diffs_dir }) + + expect(subject.extracted_flags).to eq('') + end + end + + context 'with only deleted definition files' do + let(:ff_sub_dir) { %w[feature_flags development] } + + before do + feature_flag_diff1.close + feature_flag_diff2.close + FileUtils.rm_r(feature_flag_diff1) + FileUtils.rm_r(feature_flag_diff2) + end + + it 'returns feature flags and their state as deleted' do + subject = described_class.new({ files: diffs_dir, state: 'deleted' }) + + expect(subject.extracted_flags).to eq('foobar_flag=deleted') + end + + it 'does not return feature flags when the desired state is enabled' do + subject = described_class.new({ files: diffs_dir, state: 'enabled' }) + + expect(subject.extracted_flags).to eq('') + end + + it 'does not return feature flags when the desired state is disabled' do + subject = described_class.new({ files: diffs_dir, state: 'disabled' }) expect(subject.extracted_flags).to eq('') end diff --git a/tooling/bin/find_change_diffs b/tooling/bin/find_change_diffs index 7857945ea7453b6ab13609d9dbd1c26aefb993de..b28b20df0f41c10c6816a010a58016f7933615bb 100755 --- a/tooling/bin/find_change_diffs +++ b/tooling/bin/find_change_diffs @@ -33,6 +33,8 @@ end Gitlab.merge_request_changes(mr_project_path, mr_iid).changes.each do |change| next if change['diff'].empty? - output_diffs_dir.join(File.dirname(change['new_path'])).mkpath - output_diffs_dir.join("#{change['new_path']}.diff").write(change['diff']) + ext = change['deleted_file'] ? ".deleted.diff" : ".diff" + new_path = output_diffs_dir.join("#{change['new_path']}#{ext}") + new_path.dirname.mkpath + new_path.write(change['diff']) end