From 503a6006ed4188c59146b1eb85b8e50415b129a2 Mon Sep 17 00:00:00 2001
From: Peter Bittner <peter.bittner@vshn.ch>
Date: Thu, 6 Aug 2020 19:04:47 +0200
Subject: [PATCH 1/5] Add feature tests for --pr option (GitHub + GitLab)

The newly added tests prove the bug reported with issue #187.
This change also adds --noop support for the --pr option.
---
 features/update.feature     | 22 ++++++++++++++++++++++
 lib/modulesync.rb           |  1 +
 lib/modulesync/pr/github.rb | 34 +++++++++++++++++++++-------------
 lib/modulesync/pr/gitlab.rb | 37 +++++++++++++++++++++----------------
 4 files changed, 65 insertions(+), 29 deletions(-)

diff --git a/features/update.feature b/features/update.feature
index 51411d78..6e9b0ff4 100644
--- a/features/update.feature
+++ b/features/update.feature
@@ -905,6 +905,28 @@ Feature: update
     Then the output should not contain "error"
     Then the output should not contain "rejected"
 
+  Scenario: Creating a GitHub PR with an update
+    Given a mocked git configuration
+    And a remote module repository
+    And a directory named "moduleroot"
+    And I set the environment variables to:
+      | variable     | value  |
+      | GITHUB_TOKEN | foobar |
+    When I run `msync update --noop --branch managed_update --pr`
+    Then the output should contain "Submitted PR "
+    And the exit status should be 0
+
+  Scenario: Creating a GitLab MR with an update
+    Given a mocked git configuration
+    And a remote module repository
+    And a directory named "moduleroot"
+    And I set the environment variables to:
+      | variable     | value  |
+      | GITLAB_TOKEN | foobar |
+    When I run `msync update --noop --branch managed_update --pr`
+    Then the output should contain "Submitted MR "
+    And the exit status should be 0
+
   Scenario: Repository with a default branch other than master
     Given a mocked git configuration
     And a remote module repository with "develop" as the default branch
diff --git a/lib/modulesync.rb b/lib/modulesync.rb
index aa627ac5..ec050557 100644
--- a/lib/modulesync.rb
+++ b/lib/modulesync.rb
@@ -132,6 +132,7 @@ def self.manage_module(puppet_module, module_files, module_options, defaults, op
 
     if options[:noop]
       Git.update_noop(git_repo, options)
+      options[:pr] && pr(module_options).manage(namespace, module_name, options)
     elsif !options[:offline]
       pushed = Git.update(git_repo, files_to_manage, options)
       pushed && options[:pr] && pr(module_options).manage(namespace, module_name, options)
diff --git a/lib/modulesync/pr/github.rb b/lib/modulesync/pr/github.rb
index 808144e6..55c2d3fc 100644
--- a/lib/modulesync/pr/github.rb
+++ b/lib/modulesync/pr/github.rb
@@ -18,18 +18,22 @@ def manage(namespace, module_name, options)
         head = "#{namespace}:#{options[:branch]}"
         target_branch = options[:pr_target_branch] || 'master'
 
-        pull_requests = @api.pull_requests(repo_path, :state => 'open', :base => target_branch, :head => head)
-        if pull_requests.empty?
-          pr = @api.create_pull_request(repo_path,
-                                        target_branch,
-                                        options[:branch],
-                                        options[:pr_title],
-                                        options[:message])
-          $stdout.puts \
-            "Submitted PR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
+        if options[:noop]
+          puts "Using no-op. Submitted PR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
         else
-          # Skip creating the PR if it exists already.
-          $stdout.puts "Skipped! #{pull_requests.length} PRs found for branch #{options[:branch]}"
+          pull_requests = @api.pull_requests(repo_path, :state => 'open', :base => target_branch, :head => head)
+          if pull_requests.empty?
+            pr = @api.create_pull_request(repo_path,
+                                          target_branch,
+                                          options[:branch],
+                                          options[:pr_title],
+                                          options[:message])
+            $stdout.puts \
+              "Submitted PR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
+          else
+            # Skip creating the PR if it exists already.
+            $stdout.puts "Skipped! #{pull_requests.length} PRs found for branch #{options[:branch]}"
+          end
         end
 
         # PR labels can either be a list in the YAML file or they can pass in a comma
@@ -39,8 +43,12 @@ def manage(namespace, module_name, options)
         # 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)
+        if options[:noop]
+          puts "Using no-op. Attaching the following labels to the PR: #{pr_labels.join(', ')}"
+        else
+          $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
diff --git a/lib/modulesync/pr/gitlab.rb b/lib/modulesync/pr/gitlab.rb
index c5ad57d8..8ed14fcd 100644
--- a/lib/modulesync/pr/gitlab.rb
+++ b/lib/modulesync/pr/gitlab.rb
@@ -18,23 +18,28 @@ def manage(namespace, module_name, options)
 
         head = "#{namespace}:#{options[:branch]}"
         target_branch = options[:pr_target_branch] || 'master'
-        merge_requests = @api.merge_requests(repo_path,
-                                             :state => 'opened',
-                                             :source_branch => head,
-                                             :target_branch => target_branch)
-        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 => target_branch,
-                                         :labels => mr_labels)
-          $stdout.puts \
-            "Submitted MR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
-          return if mr_labels.empty?
-          $stdout.puts "Attached the following labels to MR #{mr.iid}: #{mr_labels.join(', ')}"
+
+        if options[:noop]
+          puts "Using no-op. Submitted MR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
         else
-          # Skip creating the MR if it exists already.
-          $stdout.puts "Skipped! #{merge_requests.length} MRs found for branch #{options[:branch]}"
+          merge_requests = @api.merge_requests(repo_path,
+                                               :state => 'opened',
+                                               :source_branch => head,
+                                               :target_branch => target_branch)
+          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 => target_branch,
+                                           :labels => mr_labels)
+            $stdout.puts \
+              "Submitted MR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
+            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

From 41d5def8e07cce58b711dfc0297fff9cd0c8991b Mon Sep 17 00:00:00 2001
From: Peter Bittner <peter.bittner@vshn.ch>
Date: Mon, 10 Aug 2020 17:17:05 +0200
Subject: [PATCH 2/5] Use more obvious wording for no-op runs

---
 features/update.feature     | 4 ++--
 lib/modulesync/pr/github.rb | 4 ++--
 lib/modulesync/pr/gitlab.rb | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/features/update.feature b/features/update.feature
index 6e9b0ff4..5dedf977 100644
--- a/features/update.feature
+++ b/features/update.feature
@@ -913,7 +913,7 @@ Feature: update
       | variable     | value  |
       | GITHUB_TOKEN | foobar |
     When I run `msync update --noop --branch managed_update --pr`
-    Then the output should contain "Submitted PR "
+    Then the output should contain "Would submit PR "
     And the exit status should be 0
 
   Scenario: Creating a GitLab MR with an update
@@ -924,7 +924,7 @@ Feature: update
       | variable     | value  |
       | GITLAB_TOKEN | foobar |
     When I run `msync update --noop --branch managed_update --pr`
-    Then the output should contain "Submitted MR "
+    Then the output should contain "Would submit MR "
     And the exit status should be 0
 
   Scenario: Repository with a default branch other than master
diff --git a/lib/modulesync/pr/github.rb b/lib/modulesync/pr/github.rb
index 55c2d3fc..13606888 100644
--- a/lib/modulesync/pr/github.rb
+++ b/lib/modulesync/pr/github.rb
@@ -19,7 +19,7 @@ def manage(namespace, module_name, options)
         target_branch = options[:pr_target_branch] || 'master'
 
         if options[:noop]
-          puts "Using no-op. Submitted PR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
+          puts "Using no-op. Would submit PR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
         else
           pull_requests = @api.pull_requests(repo_path, :state => 'open', :base => target_branch, :head => head)
           if pull_requests.empty?
@@ -44,7 +44,7 @@ def manage(namespace, module_name, options)
         # already exist. We DO NOT create missing labels.
         return if pr_labels.empty?
         if options[:noop]
-          puts "Using no-op. Attaching the following labels to the PR: #{pr_labels.join(', ')}"
+          puts "Using no-op. Would attach the following labels to the PR: #{pr_labels.join(', ')}"
         else
           $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)
diff --git a/lib/modulesync/pr/gitlab.rb b/lib/modulesync/pr/gitlab.rb
index 8ed14fcd..0659bb2f 100644
--- a/lib/modulesync/pr/gitlab.rb
+++ b/lib/modulesync/pr/gitlab.rb
@@ -20,7 +20,7 @@ def manage(namespace, module_name, options)
         target_branch = options[:pr_target_branch] || 'master'
 
         if options[:noop]
-          puts "Using no-op. Submitted MR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
+          puts "Using no-op. Would submit MR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
         else
           merge_requests = @api.merge_requests(repo_path,
                                                :state => 'opened',

From d75e9713aaddc1de55fc40d57259dc6858069289 Mon Sep 17 00:00:00 2001
From: Peter Bittner <peter.bittner@vshn.ch>
Date: Mon, 10 Aug 2020 17:51:53 +0200
Subject: [PATCH 3/5] Avoid element access on nil value

Fixes #187
---
 lib/modulesync.rb           | 11 ++++++++---
 lib/modulesync/pr/github.rb |  7 +++++--
 lib/modulesync/pr/gitlab.rb |  7 +++++--
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/lib/modulesync.rb b/lib/modulesync.rb
index ec050557..d742a121 100644
--- a/lib/modulesync.rb
+++ b/lib/modulesync.rb
@@ -184,9 +184,14 @@ def self.update(options)
     exit 1 if errors && options[:fail_on_warnings]
   end
 
-  def self.pr(module_options = {})
-    github_conf = module_options[:github]
-    gitlab_conf = module_options[:gitlab]
+  def self.pr(module_options)
+    if !module_options.nil?
+      github_conf = module_options[:github]
+      gitlab_conf = module_options[:gitlab]
+    else
+      github_conf = nil
+      gitlab_conf = nil
+    end
 
     if !github_conf.nil?
       base_url = github_conf[:base_url] || ENV.fetch('GITHUB_BASE_URL', 'https://api.github.com')
diff --git a/lib/modulesync/pr/github.rb b/lib/modulesync/pr/github.rb
index 13606888..2791c060 100644
--- a/lib/modulesync/pr/github.rb
+++ b/lib/modulesync/pr/github.rb
@@ -19,7 +19,9 @@ def manage(namespace, module_name, options)
         target_branch = options[:pr_target_branch] || 'master'
 
         if options[:noop]
-          puts "Using no-op. Would submit PR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
+          $stdout.puts \
+            "Using no-op. Would submit PR '#{options[:pr_title]}' to #{repo_path} " \
+            "- merges #{options[:branch]} into #{target_branch}"
         else
           pull_requests = @api.pull_requests(repo_path, :state => 'open', :base => target_branch, :head => head)
           if pull_requests.empty?
@@ -29,7 +31,8 @@ def manage(namespace, module_name, options)
                                           options[:pr_title],
                                           options[:message])
             $stdout.puts \
-              "Submitted PR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
+              "Submitted PR '#{options[:pr_title]}' to #{repo_path} "\
+              "- merges #{options[:branch]} into #{target_branch}"
           else
             # Skip creating the PR if it exists already.
             $stdout.puts "Skipped! #{pull_requests.length} PRs found for branch #{options[:branch]}"
diff --git a/lib/modulesync/pr/gitlab.rb b/lib/modulesync/pr/gitlab.rb
index 0659bb2f..4a7b067c 100644
--- a/lib/modulesync/pr/gitlab.rb
+++ b/lib/modulesync/pr/gitlab.rb
@@ -20,7 +20,9 @@ def manage(namespace, module_name, options)
         target_branch = options[:pr_target_branch] || 'master'
 
         if options[:noop]
-          puts "Using no-op. Would submit MR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
+          $stdout.puts \
+            "Using no-op. Would submit MR '#{options[:pr_title]}' to #{repo_path} " \
+            "- merges #{options[:branch]} into #{target_branch}"
         else
           merge_requests = @api.merge_requests(repo_path,
                                                :state => 'opened',
@@ -33,7 +35,8 @@ def manage(namespace, module_name, options)
                                            :target_branch => target_branch,
                                            :labels => mr_labels)
             $stdout.puts \
-              "Submitted MR '#{options[:pr_title]}' to #{repo_path} - merges #{options[:branch]} into #{target_branch}"
+              "Submitted MR '#{options[:pr_title]}' to #{repo_path} " \
+              "- merges #{options[:branch]} into #{target_branch}"
             return if mr_labels.empty?
             $stdout.puts "Attached the following labels to MR #{mr.iid}: #{mr_labels.join(', ')}"
           else

From d441ca46ee3b8eb0ffe1c5ac9c1227a405a6d0ce Mon Sep 17 00:00:00 2001
From: Peter Bittner <peter.bittner@vshn.ch>
Date: Tue, 11 Aug 2020 09:30:09 +0200
Subject: [PATCH 4/5] Make code more concise

---
 lib/modulesync.rb | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/lib/modulesync.rb b/lib/modulesync.rb
index d742a121..101d4197 100644
--- a/lib/modulesync.rb
+++ b/lib/modulesync.rb
@@ -185,13 +185,9 @@ def self.update(options)
   end
 
   def self.pr(module_options)
-    if !module_options.nil?
-      github_conf = module_options[:github]
-      gitlab_conf = module_options[:gitlab]
-    else
-      github_conf = nil
-      gitlab_conf = nil
-    end
+    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')

From ebc888fbb6d52df1dc494ba5ca19dfc0f9e95849 Mon Sep 17 00:00:00 2001
From: Peter Bittner <peter.bittner@vshn.ch>
Date: Tue, 11 Aug 2020 09:36:26 +0200
Subject: [PATCH 5/5] Reduce visual complexity by early exit instead of
 indentation

Align implementation for GitHub PRs and GitLab MRs
---
 lib/modulesync/pr/github.rb | 44 +++++++++++++++++-------------------
 lib/modulesync/pr/gitlab.rb | 45 ++++++++++++++++++++-----------------
 2 files changed, 45 insertions(+), 44 deletions(-)

diff --git a/lib/modulesync/pr/github.rb b/lib/modulesync/pr/github.rb
index 2791c060..31254dce 100644
--- a/lib/modulesync/pr/github.rb
+++ b/lib/modulesync/pr/github.rb
@@ -22,36 +22,34 @@ def manage(namespace, module_name, options)
           $stdout.puts \
             "Using no-op. Would submit PR '#{options[:pr_title]}' to #{repo_path} " \
             "- merges #{options[:branch]} into #{target_branch}"
-        else
-          pull_requests = @api.pull_requests(repo_path, :state => 'open', :base => target_branch, :head => head)
-          if pull_requests.empty?
-            pr = @api.create_pull_request(repo_path,
-                                          target_branch,
-                                          options[:branch],
-                                          options[:pr_title],
-                                          options[:message])
-            $stdout.puts \
-              "Submitted PR '#{options[:pr_title]}' to #{repo_path} "\
-              "- merges #{options[:branch]} into #{target_branch}"
-          else
-            # Skip creating the PR if it exists already.
-            $stdout.puts "Skipped! #{pull_requests.length} PRs found for branch #{options[:branch]}"
-          end
+          return
+        end
+
+        pull_requests = @api.pull_requests(repo_path,
+                                           :state => 'open',
+                                           :base => target_branch,
+                                           :head => head)
+        unless pull_requests.empty?
+          # Skip creating the PR if it exists already.
+          $stdout.puts "Skipped! #{pull_requests.length} PRs found for branch #{options[:branch]}"
+          return
         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 = @api.create_pull_request(repo_path,
+                                      target_branch,
+                                      options[:branch],
+                                      options[:pr_title],
+                                      options[:message])
+        $stdout.puts \
+          "Submitted PR '#{options[:pr_title]}' to #{repo_path} " \
+          "- merges #{options[:branch]} into #{target_branch}"
 
         # 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?
-        if options[:noop]
-          puts "Using no-op. Would attach the following labels to the PR: #{pr_labels.join(', ')}"
-        else
-          $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
+        $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
diff --git a/lib/modulesync/pr/gitlab.rb b/lib/modulesync/pr/gitlab.rb
index 4a7b067c..514e4255 100644
--- a/lib/modulesync/pr/gitlab.rb
+++ b/lib/modulesync/pr/gitlab.rb
@@ -15,7 +15,6 @@ def initialize(token, endpoint)
 
       def manage(namespace, module_name, options)
         repo_path = File.join(namespace, module_name)
-
         head = "#{namespace}:#{options[:branch]}"
         target_branch = options[:pr_target_branch] || 'master'
 
@@ -23,27 +22,31 @@ def manage(namespace, module_name, options)
           $stdout.puts \
             "Using no-op. Would submit MR '#{options[:pr_title]}' to #{repo_path} " \
             "- merges #{options[:branch]} into #{target_branch}"
-        else
-          merge_requests = @api.merge_requests(repo_path,
-                                               :state => 'opened',
-                                               :source_branch => head,
-                                               :target_branch => target_branch)
-          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 => target_branch,
-                                           :labels => mr_labels)
-            $stdout.puts \
-              "Submitted MR '#{options[:pr_title]}' to #{repo_path} " \
-              "- merges #{options[:branch]} into #{target_branch}"
-            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
+          return
+        end
+
+        merge_requests = @api.merge_requests(repo_path,
+                                             :state => 'opened',
+                                             :source_branch => head,
+                                             :target_branch => target_branch)
+        unless merge_requests.empty?
+          # Skip creating the MR if it exists already.
+          $stdout.puts "Skipped! #{merge_requests.length} MRs found for branch #{options[:branch]}"
+          return
         end
+
+        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 => target_branch,
+                                       :labels => mr_labels)
+        $stdout.puts \
+          "Submitted MR '#{options[:pr_title]}' to #{repo_path} " \
+          "- merges #{options[:branch]} into #{target_branch}"
+
+        return if mr_labels.empty?
+        $stdout.puts "Attached the following labels to MR #{mr.iid}: #{mr_labels.join(', ')}"
       end
     end
   end