Refactor PR/MR related code (1/2)

This commit starts the refactoring of PR/MR feature code.

It provides a more readable code by:
- renaming method #manage for GitHub and GitLab class to open_pull_request
- extracting the credentials and endpoints to use in only one method: GitService#instantiate

It also provides a more understandable messages:
- `msync` does not warn anymore if no environmment variables are sets, but correctly configured in module options
- when no tokens are provided, the message now only contains the service that requires to specify token

Note: This message could be improved to help the user to setup its
modules configuration or ask him to set the environment variable.

features/update/pull_request.feature

@@ -1,7 +1,13 @@
 Feature: Create a pull-request/merge-request after update
 
-  Scenario: Creating a GitHub PR with an update
+  Scenario: Run update in no-op mode and ask for GitHub PR
     Given a basic setup with a puppet module "puppet-test" from "fakenamespace"
+    And a file named "managed_modules.yml" with:
+      """
+      ---
+      puppet-test:
+        github: {}
+      """
     And a directory named "moduleroot"
     And I set the environment variables to:
       | variable     | value  |
@@ -11,9 +17,15 @@ Feature: Create a pull-request/merge-request after update
     And the exit status should be 0
     And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"
 
-  Scenario: Creating a GitLab MR with an update
+  Scenario: Run update in no-op mode and ask for GitLab MR
     Given a basic setup with a puppet module "puppet-test" from "fakenamespace"
     And a directory named "moduleroot"
+    And a file named "managed_modules.yml" with:
+      """
+      ---
+      puppet-test:
+        gitlab: {}
+      """
     And I set the environment variables to:
       | variable     | value  |
       | GITLAB_TOKEN | foobar |

lib/modulesync.rb

@@ -130,17 +130,15 @@ def self.manage_module(puppet_module, module_files, defaults)
     if options[:noop]
       puts "Using no-op. Files in '#{puppet_module.given_name}' may be changed but will not be committed."
       puppet_module.repository.show_changes(options)
-      options[:pr] && \
-        pr(puppet_module).manage(puppet_module.repository_namespace, puppet_module.repository_name, options)
+      options[:pr] && puppet_module.open_pull_request
     elsif !options[:offline]
       pushed = puppet_module.repository.submit_changes(files_to_manage, options)
       # Only bump/tag if pushing didn't fail (i.e. there were changes)
       if pushed && options[:bump]
         new = puppet_module.bump(options[:message], options[:changelog])
         puppet_module.repository.tag(new, options[:tag_pattern]) if options[:tag]
       end
-      pushed && options[:pr] && \
-        pr(puppet_module).manage(puppet_module.repository_namespace, puppet_module.repository_name, options)
+      pushed && options[:pr] && puppet_module.open_pull_request
     end
   end
 
@@ -157,15 +155,6 @@ def self.update(cli_options)
     @options = config_defaults.merge(cli_options)
     defaults = Util.parse_config(config_path(CONF_FILE, options))
 
-    if options[:pr]
-      unless options[:branch]
-        $stderr.puts 'A branch must be specified with --branch to use --pr!'
-        raise
-      end
-
-      @pr = create_pr_manager if options[:pr]
-    end
-
     local_template_dir = config_path(MODULE_FILES_DIR, options)
     local_files = find_template_files(local_template_dir)
     module_files = relative_names(local_files, local_template_dir)
@@ -189,40 +178,4 @@ def self.update(cli_options)
     end
     exit 1 if errors && options[:fail_on_warnings]
   end
-
-  def self.pr(puppet_module)
-    module_options = puppet_module.options
-    github_conf = module_options[:github]
-    gitlab_conf = module_options[:gitlab]
-
-    if !github_conf.nil?
-      base_url = github_conf[:base_url] || ENV.fetch('GITHUB_BASE_URL', 'https://api.github.com')
-      require 'modulesync/pr/github'
-      ModuleSync::PR::GitHub.new(github_conf[:token], base_url)
-    elsif !gitlab_conf.nil?
-      base_url = gitlab_conf[:base_url] || ENV.fetch('GITLAB_BASE_URL', 'https://gitlab.com/api/v4')
-      require 'modulesync/pr/gitlab'
-      ModuleSync::PR::GitLab.new(gitlab_conf[:token], base_url)
-    elsif @pr.nil?
-      $stderr.puts 'No GitHub or GitLab token specified for --pr!'
-      raise
-    else
-      @pr
-    end
-  end
-
-  def self.create_pr_manager
-    github_token = ENV.fetch('GITHUB_TOKEN', '')
-    gitlab_token = ENV.fetch('GITLAB_TOKEN', '')
-
-    if !github_token.empty?
-      require 'modulesync/pr/github'
-      ModuleSync::PR::GitHub.new(github_token, ENV.fetch('GITHUB_BASE_URL', 'https://api.github.com'))
-    elsif !gitlab_token.empty?
-      require 'modulesync/pr/gitlab'
-      ModuleSync::PR::GitLab.new(gitlab_token, ENV.fetch('GITLAB_BASE_URL', 'https://gitlab.com/api/v4'))
-    else
-      warn '--pr specified without environment variables GITHUB_TOKEN or GITLAB_TOKEN'
-    end
-  end
 end

lib/modulesync/git_service.rb

@@ -0,0 +1,24 @@
+module ModuleSync
+  # Namespace for Git service classes (ie. GitHub, GitLab)
+  module GitService
+    def self.instantiate(type:, options:)
+      options ||= {}
+      case type
+      when :github
+        endpoint = options[:base_url] || ENV.fetch('GITHUB_BASE_URL', 'https://api.github.com')
+        token = options[:token] || ENV['GITHUB_TOKEN']
+        raise ModuleSync::Error, 'No GitHub token specified to create a pull request' if token.nil?
+        require 'modulesync/pr/github'
+        return ModuleSync::PR::GitHub.new(token, endpoint)
+      when :gitlab
+        endpoint = options[:base_url] || ENV.fetch('GITLAB_BASE_URL', 'https://gitlab.com/api/v4')
+        token = options[:token] || ENV['GITLAB_TOKEN']
+        raise ModuleSync::Error, 'No GitLab token specified to create a merge request' if token.nil?
+        require 'modulesync/pr/gitlab'
+        return ModuleSync::PR::GitLab.new(token, endpoint)
+      else
+        raise ModuleSync::Error, "Unable to manage a PR/MR for Git service: '#{type}'"
+      end
+    end
+  end
+end

lib/modulesync/pr/github.rb

@@ -1,3 +1,5 @@
+require 'modulesync/git_service'
+
 require 'octokit'
 require 'modulesync/util'
 
@@ -13,7 +15,7 @@ def initialize(token, endpoint)
         @api = Octokit::Client.new(:access_token => token)
       end
 
-      def manage(namespace, module_name, options)
+      def open_pull_request(namespace, module_name, options)
         repo_path = File.join(namespace, module_name)
         branch = options[:remote_branch] || options[:branch]
         head = "#{namespace}:#{branch}"

lib/modulesync/pr/gitlab.rb

@@ -1,3 +1,5 @@
+require 'modulesync/git_service'
+
 require 'gitlab'
 require 'modulesync/util'
 
@@ -13,7 +15,7 @@ def initialize(token, endpoint)
         )
       end
 
-      def manage(namespace, module_name, options)
+      def open_pull_request(namespace, module_name, options)
         repo_path = File.join(namespace, module_name)
         branch = options[:remote_branch] || options[:branch]
         head = "#{namespace}:#{branch}"

lib/modulesync/source_code.rb

@@ -1,4 +1,5 @@
 require 'modulesync'
+require 'modulesync/git_service'
 require 'modulesync/repository'
 require 'modulesync/util'
 
@@ -47,6 +48,11 @@ def path(*parts)
       File.join(working_directory, *parts)
     end
 
+    def open_pull_request
+      git_service = GitService.instantiate type: git_service_type, options: @options[git_service_type]
+      git_service.open_pull_request(repository_namespace, repository_name, ModuleSync.options)
+    end
+
     private
 
     def _repository_remote

spec/unit/modulesync/git_service_spec.rb

@@ -0,0 +1,15 @@
+require 'modulesync/git_service'
+
+describe ModuleSync::GitService do
+  context 'when instantiate a GitHub service without credentials' do
+    it 'raises an error' do
+      expect { ModuleSync::GitService.instantiate(type: :github, options: nil) }.to raise_error(ModuleSync::Error, 'No GitHub token specified to create a pull request')
+    end
+  end
+
+  context 'when instantiate a GitLab service without credentials' do
+    it 'raises an error' do
+      expect { ModuleSync::GitService.instantiate(type: :gitlab, options: nil) }.to raise_error(ModuleSync::Error, 'No GitLab token specified to create a merge request')
+    end
+  end
+end

spec/unit/modulesync/pr/github_spec.rb

@@ -2,7 +2,7 @@
 require 'modulesync/pr/github'
 
 describe ModuleSync::PR::GitHub do
-  context '::manage' do
+  context '::open_pull_request' do
     before(:each) do
       @git_repo = 'test/modulesync'
       @namespace, @repo_name = @git_repo.split('/')
@@ -22,7 +22,7 @@
     it 'submits PR when --pr is set' do
       allow(@client).to receive(:pull_requests).with(@git_repo, :state => 'open', :base => 'master', :head => "#{@namespace}:#{@options[:branch]}").and_return([])
       expect(@client).to receive(:create_pull_request).with(@git_repo, 'master', @options[:branch], @options[:pr_title], @options[:message]).and_return({"html_url" => "http://example.com/pulls/22"})
-      expect { @it.manage(@namespace, @repo_name, @options) }.to output(/Submitted PR/).to_stdout
+      expect { @it.open_pull_request(@namespace, @repo_name, @options) }.to output(/Submitted PR/).to_stdout
     end
 
     it 'skips submitting PR if one has already been issued' do
@@ -33,7 +33,7 @@
       }
 
       expect(@client).to receive(:pull_requests).with(@git_repo, :state => 'open', :base => 'master', :head => "#{@namespace}:#{@options[:branch]}").and_return([pr])
-      expect { @it.manage(@namespace, @repo_name, @options) }.to output(/Skipped! 1 PRs found for branch test/).to_stdout
+      expect { @it.open_pull_request(@namespace, @repo_name, @options) }.to output(/Skipped! 1 PRs found for branch test/).to_stdout
     end
 
     it 'adds labels to PR when --pr-labels is set' do
@@ -43,7 +43,7 @@
       allow(@client).to receive(:pull_requests).with(@git_repo, :state => 'open', :base => 'master', :head => "#{@namespace}:#{@options[:branch]}").and_return([])
 
       expect(@client).to receive(:add_labels_to_an_issue).with(@git_repo, "44", ["HELLO", "WORLD"])
-      expect { @it.manage(@namespace, @repo_name, @options) }.to output(/Attaching the following labels to PR 44: HELLO, WORLD/).to_stdout
+      expect { @it.open_pull_request(@namespace, @repo_name, @options) }.to output(/Attaching the following labels to PR 44: HELLO, WORLD/).to_stdout
     end
   end
 end

spec/unit/modulesync/pr/gitlab_spec.rb

@@ -2,7 +2,7 @@
 require 'modulesync/pr/gitlab'
 
 describe ModuleSync::PR::GitLab do
-  context '::manage' do
+  context '::open_pull_request' do
     before(:each) do
       @git_repo = 'test/modulesync'
       @namespace, @repo_name = @git_repo.split('/')
@@ -35,7 +35,7 @@
               :target_branch => 'master',
              ).and_return({"html_url" => "http://example.com/pulls/22"})
 
-      expect { @it.manage(@namespace, @repo_name, @options) }.to output(/Submitted MR/).to_stdout
+      expect { @it.open_pull_request(@namespace, @repo_name, @options) }.to output(/Submitted MR/).to_stdout
     end
 
     it 'skips submitting MR if one has already been issued' do
@@ -52,7 +52,7 @@
               :target_branch => 'master',
              ).and_return([mr])
 
-      expect { @it.manage(@namespace, @repo_name, @options) }.to output(/Skipped! 1 MRs found for branch test/).to_stdout
+      expect { @it.open_pull_request(@namespace, @repo_name, @options) }.to output(/Skipped! 1 MRs found for branch test/).to_stdout
     end
 
     it 'adds labels to MR when --pr-labels is set' do
@@ -75,7 +75,7 @@
               :target_branch => 'master',
              ).and_return([])
 
-      expect { @it.manage(@namespace, @repo_name, @options) }.to output(/Attached the following labels to MR 42: HELLO, WORLD/).to_stdout
+      expect { @it.open_pull_request(@namespace, @repo_name, @options) }.to output(/Attached the following labels to MR 42: HELLO, WORLD/).to_stdout
     end
   end
 end

spec/unit/modulesync_spec.rb

@@ -11,16 +11,4 @@
       ModuleSync.update(options)
     end
   end
-
-  context '::pr' do
-    describe "Raise Error" do
-      let(:puppet_module) do
-        ModuleSync::PuppetModule.new 'puppet-test', remote: 'dummy'
-      end
-
-      it 'raises an error when neither GITHUB_TOKEN nor GITLAB_TOKEN are set for PRs' do
-        expect { ModuleSync.pr(puppet_module) }.to raise_error(RuntimeError).and output(/No GitHub or GitLab token specified for --pr/).to_stderr
-      end
-    end
-  end
 end