Rework PR/MR feature

.rubocop_todo.yml

@@ -1,37 +1,43 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2021-04-22 16:30:35 +0200 using RuboCop version 0.50.0.
+# on 2021-04-23 16:47:47 +0200 using RuboCop version 0.50.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
-# Offense count: 1
+# Offense count: 3
 Lint/UselessAssignment:
   Exclude:
     - 'lib/modulesync.rb'
 
 # Offense count: 10
 Metrics/AbcSize:
-  Max: 67
+  Max: 59
 
 # Offense count: 2
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 128
+  Max: 125
 
-# Offense count: 3
+# Offense count: 4
 Metrics/CyclomaticComplexity:
-  Max: 12
+  Max: 13
+
+# Offense count: 3
+# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
+# URISchemes: http, https
+Metrics/LineLength:
+  Max: 186
 
-# Offense count: 13
+# Offense count: 17
 # Configuration parameters: CountComments.
 Metrics/MethodLength:
-  Max: 36
+  Max: 32
 
 # Offense count: 3
 Metrics/PerceivedComplexity:
-  Max: 13
+  Max: 14
 
 # Offense count: 8
 Style/Documentation:
@@ -49,3 +55,12 @@ Style/Documentation:
 Style/EachWithObject:
   Exclude:
     - 'lib/modulesync/util.rb'
+
+# Offense count: 2
+# Cop supports --auto-correct.
+# Configuration parameters: EnforcedStyleForMultiline, SupportedStylesForMultiline.
+# SupportedStylesForMultiline: comma, consistent_comma, no_comma
+Style/TrailingCommaInArguments:
+  Exclude:
+    - 'lib/modulesync/git_service/base.rb'
+    - 'lib/modulesync/source_code.rb'

features/update.feature

@@ -713,28 +713,6 @@ Feature: update
     And the puppet module "puppet-test" from "fakenamespace" should have only 1 commit made by "Aruba"
     And the puppet module "puppet-test" from "fakenamespace" should have 1 commit made by "Aruba" in branch "test"
 
-  Scenario: Creating a GitHub PR with an update
-    Given a basic setup with a puppet module "puppet-test" from "fakenamespace"
-    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 "Would submit PR "
-    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
-    Given a basic setup with a puppet module "puppet-test" from "fakenamespace"
-    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 "Would submit MR "
-    And the exit status should be 0
-    And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"
-
   Scenario: Repository with a default branch other than master
     Given a basic setup with a puppet module "puppet-test" from "fakenamespace"
     And the puppet module "puppet-test" from "fakenamespace" has the default branch named "develop"

features/update/pull_request.feature

@@ -0,0 +1,184 @@
+Feature: Create a pull-request/merge-request after 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 I set the environment variables to:
+      | variable        | value                    |
+      | GITHUB_TOKEN    | foobar                   |
+      | GITHUB_BASE_URL | https://github.example.com |
+    And a file named "config_defaults.yml" with:
+      """
+      ---
+      test:
+        name: aruba
+      """
+    And a file named "moduleroot/test.erb" with:
+      """
+      <%= @configs['name'] %>
+      """
+    When I run `msync update --noop --branch managed_update --pr`
+    Then the output should contain "Would submit PR "
+    And the exit status should be 0
+    And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"
+
+  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 file named "managed_modules.yml" with:
+      """
+      ---
+      puppet-test:
+        gitlab: {
+          base_url: 'https://gitlab.example.com'
+      }
+      """
+    And I set the environment variables to:
+      | variable     | value  |
+      | GITLAB_TOKEN | foobar |
+    And a file named "config_defaults.yml" with:
+      """
+      ---
+      test:
+        name: aruba
+      """
+    And a file named "moduleroot/test.erb" with:
+      """
+      <%= @configs['name'] %>
+      """
+    When I run `msync update --noop --branch managed_update --pr`
+    Then the output should contain "Would submit MR "
+    And the exit status should be 0
+    And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"
+
+  Scenario: Run update without changes 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: {
+          base_url: 'https://gitlab.example.com'
+        }
+      """
+    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 not contain "Would submit MR "
+    And the exit status should be 0
+    And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"
+
+  Scenario: Ask for PR without credentials
+    Given a basic setup with a puppet module "puppet-test" from "fakenamespace"
+    And a file named "managed_modules.yml" with:
+      """
+      ---
+      puppet-test:
+        gitlab: {
+          base_url: https://gitlab.example.com
+        }
+      """
+    And a file named "config_defaults.yml" with:
+      """
+      ---
+      test:
+        name: aruba
+      """
+    And a file named "moduleroot/test.erb" with:
+      """
+      <%= @configs['name'] %>
+      """
+    When I run `msync update --noop --pr`
+    Then the stderr should contain "A token is required to use services from gitlab"
+    And the exit status should be 1
+    And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"
+
+  Scenario: Ask for PR/MR with modules from GitHub and from GitLab
+    Given a basic setup with a puppet module "puppet-github" from "fakenamespace"
+    And a basic setup with a puppet module "puppet-gitlab" from "fakenamespace"
+    And a file named "managed_modules.yml" with:
+      """
+      ---
+      puppet-github:
+        github:
+          base_url: https://github.example.com
+          token: 'secret'
+      puppet-gitlab:
+        gitlab:
+          base_url: https://gitlab.example.com
+          token: 'secret'
+      """
+    And a file named "config_defaults.yml" with:
+      """
+      ---
+      test:
+        name: aruba
+      """
+    And a file named "moduleroot/test.erb" with:
+      """
+      <%= @configs['name'] %>
+      """
+    When I run `msync update --noop --branch managed_update --pr`
+    Then the exit status should be 0
+    And the output should contain "Would submit PR "
+    And the output should contain "Would submit MR "
+    And the puppet module "puppet-github" from "fakenamespace" should have no commits made by "Aruba"
+    And the puppet module "puppet-gitlab" from "fakenamespace" should have no commits made by "Aruba"
+
+  Scenario: Ask for PR with same source and target branch
+    Given a basic setup with a puppet module "puppet-test" from "fakenamespace"
+    And a file named "managed_modules.yml" with:
+      """
+      ---
+      puppet-test:
+        gitlab:
+          token: 'secret'
+          base_url: 'https://gitlab.example.com'
+      """
+    And a file named "config_defaults.yml" with:
+      """
+      ---
+      test:
+        name: aruba
+      """
+    And a file named "moduleroot/test.erb" with:
+      """
+      <%= @configs['name'] %>
+      """
+    When I run `msync update --noop --branch managed_update --pr --pr-target-branch managed_update`
+    Then the stderr should contain "Unable to open a pull request with the same source and target branch: 'managed_update'"
+    And the exit status should be 1
+    And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"
+
+  Scenario: Ask for PR with the default branch as source and target
+    Given a basic setup with a puppet module "puppet-test" from "fakenamespace"
+    And the puppet module "puppet-test" from "fakenamespace" has the default branch named "custom_default_branch"
+    And a file named "managed_modules.yml" with:
+      """
+      ---
+      puppet-test:
+        github:
+          token: 'secret'
+          base_url: 'https://gitlab.example.com'
+      """
+    And a file named "config_defaults.yml" with:
+      """
+      ---
+      test:
+        name: aruba
+      """
+    And a file named "moduleroot/test.erb" with:
+      """
+      <%= @configs['name'] %>
+      """
+    And a directory named "moduleroot"
+    When I run `msync update --noop --pr`
+    Then the stderr should contain "Unable to open a pull request with the same source and target branch: 'custom_default_branch'"
+    And the exit status should be 1
+    And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"

lib/modulesync.rb

@@ -129,18 +129,16 @@ 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)
+      changed = puppet_module.repository.show_changes(options)
+      changed && 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/cli.rb

@@ -104,7 +104,7 @@ class Base < Thor
              :default => CLI.defaults[:pr_labels] || []
       option :pr_target_branch,
              :desc => 'Target branch for the pull/merge request',
-             :default => CLI.defaults[:pr_target_branch] || 'master'
+             :default => CLI.defaults[:pr_target_branch]
       option :offline,
              :type => :boolean,
              :desc => 'Do not run any Git commands. Allows the user to manage Git outside of ModuleSync.',