Merge branch 'improve-feature-flag-tool' into 'master'

Add `template` and `MR` question to `feature-flag`

See merge request gitlab-org/gitlab!39580

bin/feature-flag

@@ -8,7 +8,7 @@
 require 'optparse'
 require 'yaml'
 require 'fileutils'
-require 'cgi'
+require 'uri'
 
 require_relative '../lib/feature/shared' unless defined?(Feature::Shared)
 
@@ -105,10 +105,11 @@ class FeatureFlagOptionParser
     end
 
     def read_group
+      $stdout.puts
       $stdout.puts ">> Please specify the group introducing feature flag, like `group::apm`:"
 
       loop do
-        $stdout.print "\n?> "
+        $stdout.print "?> "
         group = $stdin.gets.strip
         group = nil if group.empty?
         return group if group.nil? || group.start_with?('group::')
@@ -121,6 +122,7 @@ class FeatureFlagOptionParser
       # if there's only one type, do not ask, return
       return TYPES.first.first if TYPES.one?
 
+      $stdout.puts
       $stdout.puts ">> Please specify the type of your feature flag:"
       $stdout.puts
       TYPES.each do |type, data|
@@ -128,7 +130,7 @@ class FeatureFlagOptionParser
       end
 
       loop do
-        $stdout.print "\n?> "
+        $stdout.print "?> "
 
         type = $stdin.gets.strip.to_sym
         return type if TYPES[type]
@@ -137,27 +139,41 @@ class FeatureFlagOptionParser
       end
     end
 
-    def read_issue_url(options)
+    def read_introduced_by_url
+      $stdout.puts
+      $stdout.puts ">> If you have MR open, can you paste the URL here? (or enter to skip)"
+
+      loop do
+        $stdout.print "?> "
+        introduced_by_url = $stdin.gets.strip
+        introduced_by_url = nil if introduced_by_url.empty?
+        return introduced_by_url if introduced_by_url.nil? || introduced_by_url.start_with?('https://')
+
+        $stderr.puts "URL needs to start with https://"
+      end
+    end
+
+    def read_rollout_issue_url(options)
       return unless TYPES.dig(options.type, :rollout_issue)
 
       url = "https://gitlab.com/gitlab-org/gitlab/-/issues/new"
       title = "[Feature flag] Rollout of `#{options.name}`"
-      description = File.read('.gitlab/issue_templates/Feature Flag Roll Out.md')
-      description.sub!(':feature_name', options.name)
 
-      issue_new_url = url + "?" +
-        "issue[title]=" + CGI.escape(title) + "&"
-        # TODO: We should be able to pick `issueable_template`
-        # + "issue[description]=" + CGI.escape(description)
+      params = {
+        'issue[title]' => "[Feature flag] Rollout of `#{options.name}`",
+        'issuable_template' => 'Feature Flag Roll Out',
+      }
+      issue_new_url = url + "?" + URI.encode_www_form(params)
 
+      $stdout.puts
       $stdout.puts ">> Open this URL and fill the rest of details:"
       $stdout.puts issue_new_url
       $stdout.puts
 
-      $stdout.puts ">> Paste URL here, or enter to skip:"
+      $stdout.puts ">> Paste URL of `rollout issue` here, or enter to skip:"
 
       loop do
-        $stdout.print "\n?> "
+        $stdout.print "?> "
         created_url = $stdin.gets.strip
         created_url = nil if created_url.empty?
         return created_url if created_url.nil? || created_url.start_with?('https://')
@@ -185,7 +201,8 @@ class FeatureFlagCreator
     # Read type from $stdin unless is already set
     options.type ||= FeatureFlagOptionParser.read_type
     options.group ||= FeatureFlagOptionParser.read_group
-    options.rollout_issue_url ||= FeatureFlagOptionParser.read_issue_url(options)
+    options.introduced_by_url ||= FeatureFlagOptionParser.read_introduced_by_url
+    options.rollout_issue_url ||= FeatureFlagOptionParser.read_rollout_issue_url(options)
 
     $stdout.puts "\e[32mcreate\e[0m #{file_path}"
     $stdout.puts contents

doc/development/feature_flags/development.md

@@ -78,19 +78,21 @@ Only feature flags that have a YAML definition file can be used when running the
 ```shell
 $ bin/feature-flag my-feature-flag
 >> Please specify the group introducing feature flag, like `group::apm`:
-
 ?> group::memory
->> Open this URL and fill the rest of details:
-https://gitlab.com/gitlab-org/gitlab/-/issues/new?issue[title]=%5BFeature+flag%5D+Rollout+of+%60my-feature-flag%60&
 
->> Paste URL here, or enter to skip:
+>> If you have MR open, can you paste the URL here? (or enter to skip)
+?> https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38602
+
+>> Open this URL and fill the rest of details:
+https://gitlab.com/gitlab-org/gitlab/-/issues/new?issue%5Btitle%5D=%5BFeature+flag%5D+Rollout+of+%60test-flag%60&issuable_template=Feature+Flag+Roll+Out
 
-?>
-create config/feature_flags/development/my_feature_flag.yml
+>> Paste URL of `rollout issue` here, or enter to skip:
+?> https://gitlab.com/gitlab-org/gitlab/-/issues/232533
+create config/feature_flags/development/test-flag.yml
 ---
-name: my_feature_flag
-introduced_by_url:
-rollout_issue_url:
+name: test-flag
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38602
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/232533
 group: group::memory
 type: development
 default_enabled: false

spec/bin/feature_flag_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe 'bin/feature-flag' do
   using RSpec::Parameterized::TableSyntax
 
   describe FeatureFlagCreator do
-    let(:argv) { %w[feature-flag-name -t development -g group::memory -i https://url] }
+    let(:argv) { %w[feature-flag-name -t development -g group::memory -i https://url -m http://url] }
     let(:options) { FeatureFlagOptionParser.parse(argv) }
     let(:creator) { described_class.new(options) }
     let(:existing_flag) { File.join('config', 'feature_flags', 'development', 'existing-feature-flag.yml') }
@@ -183,15 +183,51 @@ RSpec.describe 'bin/feature-flag' do
       end
     end
 
-    describe '.rollout_issue_url' do
+    describe '.read_introduced_by_url' do
+      let(:url) { 'https://merge-request' }
+
+      it 'reads type from $stdin' do
+        expect($stdin).to receive(:gets).and_return(url)
+        expect do
+          expect(described_class.read_introduced_by_url).to eq('https://merge-request')
+        end.to output(/can you paste the URL here/).to_stdout
+      end
+
+      context 'empty URL given' do
+        let(:url) { '' }
+
+        it 'skips entry' do
+          expect($stdin).to receive(:gets).and_return(url)
+          expect do
+            expect(described_class.read_introduced_by_url).to be_nil
+          end.to output(/can you paste the URL here/).to_stdout
+        end
+      end
+
+      context 'invalid URL given' do
+        let(:url) { 'invalid' }
+
+        it 'shows error message and retries' do
+          expect($stdin).to receive(:gets).and_return(url)
+          expect($stdin).to receive(:gets).and_raise('EOF')
+
+          expect do
+            expect { described_class.read_introduced_by_url }.to raise_error(/EOF/)
+          end.to output(/can you paste the URL here/).to_stdout
+            .and output(/URL needs to start with/).to_stderr
+        end
+      end
+    end
+
+    describe '.read_rollout_issue_url' do
       let(:options) { OpenStruct.new(name: 'foo', type: :development) }
       let(:url) { 'https://issue' }
 
       it 'reads type from $stdin' do
         expect($stdin).to receive(:gets).and_return(url)
         expect do
-          expect(described_class.read_issue_url(options)).to eq('https://issue')
-        end.to output(/Paste URL here/).to_stdout
+          expect(described_class.read_rollout_issue_url(options)).to eq('https://issue')
+        end.to output(/Paste URL of `rollout issue` here/).to_stdout
       end
 
       context 'invalid URL given' do
@@ -202,8 +238,8 @@ RSpec.describe 'bin/feature-flag' do
           expect($stdin).to receive(:gets).and_raise('EOF')
 
           expect do
-            expect { described_class.read_issue_url(options) }.to raise_error(/EOF/)
-          end.to output(/Paste URL here/).to_stdout
+            expect { described_class.read_rollout_issue_url(options) }.to raise_error(/EOF/)
+          end.to output(/Paste URL of `rollout issue` here/).to_stdout
             .and output(/URL needs to start/).to_stderr
         end
       end