Cleanup: move config validation out of processing

This commit moves configuration validation (--pr cannot be used without
--branch) into initialization. This catches the error early on, before
any actual work is done.

Signed-off-by: Manuel Hutter <manuel@vshn.ch>

lib/modulesync.rb

@@ -132,32 +132,22 @@ def self.manage_module(puppet_module, module_files, module_options, defaults, op
     if options[:noop]
       Git.update_noop(git_repo, options)
     elsif !options[:offline]
-      if options[:pr]
-        manage_pr_or_mr(git_repo, files_to_manage, namespace, module_name, options)
-      else
-        Git.update(git_repo, files_to_manage, options)
-      end
+      pushed = Git.update(git_repo, files_to_manage, options)
+      pushed && options[:pr] && @pr.manage(namespace, module_name, options)
     end
   end
 
-  def self.manage_pr_or_mr(git_repo, files_to_manage, namespace, module_name, options)
-    unless options[:branch]
-      $stderr.puts 'A branch must be specified with --branch to use --pr!'
-      return nil
-    end
-    # Git.update() returns a boolean: true if files were pushed, false if not.
-    pushed = Git.update(git_repo, files_to_manage, options)
-
-    # If there's nothing pushed, we're done
-    return nil unless pushed
-
-    @pr.manage(namespace, module_name, options)
-  end
-
   def self.update(options)
     options = config_defaults.merge(options)
     defaults = Util.parse_config(File.join(options[:configs], CONF_FILE))
-    @pr = get_pr_manager if options[:pr]
+    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 = File.join(options[:configs], MODULE_FILES_DIR)
     local_files = find_template_files(local_template_dir)
@@ -182,14 +172,13 @@ def self.update(options)
     exit 1 if errors && options[:fail_on_warnings]
   end
 
-  def self.get_pr_manager
-    case
-    when !GITHUB_TOKEN.empty?
+  def self.create_pr_manager
+    if !GITHUB_TOKEN.empty?
       require 'modulesync/pr/github'
-      return ModuleSync::PR::GitHub.new(GITHUB_TOKEN, ENV.fetch('GITHUB_BASE_URL', 'https://api.github.com'))
-    when !GITLAB_TOKEN.empty?
+      ModuleSync::PR::GitHub.new(GITHUB_TOKEN, ENV.fetch('GITHUB_BASE_URL', 'https://api.github.com'))
+    elsif !GITLAB_TOKEN.empty?
       require 'modulesync/pr/github'
-      return ModuleSync::PR::GitLab.new(GITLAB_TOKEN, ENV.fetch('GITLAB_BASE_URL', 'https://gitlab.com/api/v4'))
+      ModuleSync::PR::GitLab.new(GITLAB_TOKEN, ENV.fetch('GITLAB_BASE_URL', 'https://gitlab.com/api/v4'))
     else
       $stderr.puts 'Environment variables GITHUB_TOKEN or GITLAB_TOKEN must be set to use --pr!'
       raise

lib/modulesync/pr/github.rb

@@ -1,37 +1,41 @@
 require 'octokit'
 require 'modulesync/util'
 
-module ModuleSync::PR
-  class GitHub
-    def initialize(token, endpoint)
-      Octokit.configure do |c|
-        c.api_endpoint = endpoint
+module ModuleSync
+  module PR
+    # GitHub creates and manages pull requests on github.com or GitHub
+    # Enterprise installations.
+    class GitHub
+      def initialize(token, endpoint)
+        Octokit.configure do |c|
+          c.api_endpoint = endpoint
+        end
+        @api = Octokit::Client.new(:access_token => token)
       end
-      @api = Octokit::Client.new(:access_token => token)
-    end
 
-    def manage(namespace, module_name, options)
-      repo_path = File.join(namespace, module_name)
-      head = "#{namespace}:#{options[:branch]}"
+      def manage(namespace, module_name, options)
+        repo_path = File.join(namespace, module_name)
+        head = "#{namespace}:#{options[:branch]}"
 
-      pull_requests = @api.pull_requests(repo_path, :state => 'open', :base => 'master', :head => head)
-      if pull_requests.empty?
-        pr = @api.create_pull_request(repo_path, 'master', options[:branch], options[:pr_title], options[:message])
-        $stdout.puts "Submitted PR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into master"
-      else
-        # Skip creating the PR if it exists already.
-        $stdout.puts "Skipped! #{pull_requests.length} PRs found for branch #{options[:branch]}"
-      end
+        pull_requests = @api.pull_requests(repo_path, :state => 'open', :base => 'master', :head => head)
+        if pull_requests.empty?
+          pr = @api.create_pull_request(repo_path, 'master', options[:branch], options[:pr_title], options[:message])
+          $stdout.puts "Submitted PR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into master"
+        else
+          # Skip creating the PR if it exists already.
+          $stdout.puts "Skipped! #{pull_requests.length} PRs found for branch #{options[:branch]}"
+        end
 
-      # PR labels can either be a list in the YAML file or they can pass in a comma
-      # separated list via the command line argument.
-      pr_labels = ModuleSync::Util.parse_list(options[:pr_labels])
+        # PR labels can either be a list in the YAML file or they can pass in a comma
+        # separated list via the command line argument.
+        pr_labels = ModuleSync::Util.parse_list(options[:pr_labels])
 
-      # We only assign labels to the PR if we've discovered a list > 1. The labels MUST
-      # already exist. We DO NOT create missing labels.
-      return if pr_labels.empty?
-      $stdout.puts "Attaching the following labels to PR #{pr['number']}: #{pr_labels.join(', ')}"
-      @api.add_labels_to_an_issue(repo_path, pr['number'], pr_labels)
+        # We only assign labels to the PR if we've discovered a list > 1. The labels MUST
+        # already exist. We DO NOT create missing labels.
+        return if pr_labels.empty?
+        $stdout.puts "Attaching the following labels to PR #{pr['number']}: #{pr_labels.join(', ')}"
+        @api.add_labels_to_an_issue(repo_path, pr['number'], pr_labels)
+      end
     end
   end
 end

lib/modulesync/pr/gitlab.rb

@@ -1,35 +1,39 @@
 require 'gitlab'
 require 'modulesync/util'
 
-module ModuleSync::PR
-  class GitLab
-    def initialize(token, endpoint)
-      @api = Gitlab::Client.new(
-        :endpoint => endpoint,
-        :private_token => token,
-      )
-    end
+module ModuleSync
+  module PR
+    # GitLab creates and manages merge requests on gitlab.com or private GitLab
+    # installations.
+    class GitLab
+      def initialize(token, endpoint)
+        @api = Gitlab::Client.new(
+          :endpoint => endpoint,
+          :private_token => token
+        )
+      end
 
-    def manage(namespace, module_name, options)
-      repo_path = File.join(namespace, module_name)
+      def manage(namespace, module_name, options)
+        repo_path = File.join(namespace, module_name)
 
-      head = "#{namespace}:#{options[:branch]}"
-      merge_requests = @api.merge_requests(repo_path,
-                                           :state => 'opened',
-                                           :source_branch => head,
-                                           :target_branch => 'master')
-      if merge_requests.empty?
-        mr_labels = ModuleSync::Util.parse_list(options[:pr_labels])
-        mr = @api.create_merge_request(repo_path, options[:pr_title],
-                                       :source_branch => options[:branch],
-                                       :target_branch => 'master',
-                                       :labels => mr_labels)
-        $stdout.puts "Submitted MR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into master"
-        return if mr_labels.empty?
-        $stdout.puts "Attached the following labels to MR #{mr.iid}: #{mr_labels.join(', ')}"
-      else
-        # Skip creating the MR if it exists already.
-        $stdout.puts "Skipped! #{merge_requests.length} MRs found for branch #{options[:branch]}"
+        head = "#{namespace}:#{options[:branch]}"
+        merge_requests = @api.merge_requests(repo_path,
+                                             :state => 'opened',
+                                             :source_branch => head,
+                                             :target_branch => 'master')
+        if merge_requests.empty?
+          mr_labels = ModuleSync::Util.parse_list(options[:pr_labels])
+          mr = @api.create_merge_request(repo_path, options[:pr_title],
+                                         :source_branch => options[:branch],
+                                         :target_branch => 'master',
+                                         :labels => mr_labels)
+          $stdout.puts "Submitted MR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into master"
+          return if mr_labels.empty?
+          $stdout.puts "Attached the following labels to MR #{mr.iid}: #{mr_labels.join(', ')}"
+        else
+          # Skip creating the MR if it exists already.
+          $stdout.puts "Skipped! #{merge_requests.length} MRs found for branch #{options[:branch]}"
+        end
       end
     end
   end