Merge #7199

7199: Allow `rake release` to ask for input (3rd take) r=colby-swandale a=deivid-rodriguez

This PR supersedes #7108 and #7005.

It fixes #6854.

### What was the end-user problem that led to this PR?

The problem was that if users has 2FA authentication on their rubygems account, `rake release` doesn't really work at the moment, since it hangs asking for the OTP code, but without letting the user know.

### What was your diagnosis of the problem?

My diagnosis was that we need to allow the `rake release` helper that shells out to `gem push` to read `gem push` output and show it to the user, so that she can introduce the requested information.

### What is your fix for the problem, implemented in this PR?

My fix is inspired by @segiddins's comment in https://github.com/bundler/bundler/issues/6854#issuecomment-450536844. `Kernel#system` works like we would expect in this situation.

### Why did you choose this fix out of the possible options?

I chose this fix because #7108 had a few problems:

* It would update the `sh` helper, which is used in many different places. This was unnecessary since most of the times we shell out to the `gem` CLI we don't need to ask for input, and it also produced a very verbose output in those cases, since everything the `gem` CLI prints to the screen would be printed by the bundler helpers too. This PR does not change the current output, other than for `rake release`.

* It would print _duplicate_ output. This is a `rake release` test using #7108:

    ```
    $ RUBYOPT="-I../bundler/lib" ../bundler/exe/bundle exec rake release
      Successfully built RubyGem
      Name: rake_release_tester
      Version: 0.1.0
      File: rake_release_tester-0.1.0.gem
    rake_release_tester 0.1.0 built to pkg/rake_release_tester-0.1.0.gem.
    v0.1.0
    Tag v0.1.0 has already been created.
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   asd
    Your OTP code is incorrect. Please check it and retry.
    rake aborted!
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   Your OTP code is incorrect. Please check it and retry.
    /home/deivid/Code/bundler/lib/bundler/gem_helper.rb:187:in `sh'
    /home/deivid/Code/bundler/lib/bundler/gem_helper.rb:109:in `rubygem_push'
    /home/deivid/Code/bundler/lib/bundler/gem_helper.rb:70:in `block in install'
    /home/deivid/Code/bundler/lib/bundler/cli/exec.rb:69:in `load'
    /home/deivid/Code/bundler/lib/bundler/cli/exec.rb:69:in `kernel_load'
    /home/deivid/Code/bundler/lib/bundler/cli/exec.rb:28:in `run'
    /home/deivid/Code/bundler/lib/bundler/cli.rb:468:in `exec'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
    /home/deivid/Code/bundler/lib/bundler/cli.rb:26:in `dispatch'
    /home/deivid/Code/bundler/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
    /home/deivid/Code/bundler/lib/bundler/cli.rb:17:in `start'
    ../bundler/exe/bundle:30:in `block in <main>'
    /home/deivid/Code/bundler/lib/bundler/friendly_errors.rb:123:in `with_friendly_errors'
    ../bundler/exe/bundle:22:in `<main>'
    Tasks: TOP => release => release:rubygem_push
    (See full trace by running task with --trace)
    ```

    This is the same test using this PR:

    ```
    $ RUBYOPT="-I../bundler/lib" ../bundler/exe/bundle exec rake release
    rake_release_tester 0.1.0 built to pkg/rake_release_tester-0.1.0.gem.
    Tag v0.1.0 has already been created.
    Pushing gem to https://rubygems.org...
    You have enabled multi-factor authentication. Please enter OTP code.
    Code:   asdfasdf
    Your OTP code is incorrect. Please check it and retry.
    ```

* Previous approach was hard to test. The test included here might not be great but it's something...

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
(cherry picked from commit cd05f13a74ef2556823be48c23feaf509c3b1de7)

4 files changed, 45 insertions, 22 deletions

diff --git a/lib/bundler/gem_helper.rb b/lib/bundler/gem_helper.rb
index e9ee03b8a2..c923a0ad37 100644
--- a/lib/bundler/gem_helper.rb
+++ b/lib/bundler/gem_helper.rb
@@ -106,7 +106,7 @@ module Bundler
       unless allowed_push_host || Bundler.user_home.join(".gem/credentials").file?
         raise "Your rubygems.org credentials aren't set. Run `gem push` to set them."
       end
-      sh(gem_command)
+      sh_with_input(gem_command)
       Bundler.ui.confirm "Pushed #{name} #{version} to #{gem_push_host}"
     end
 
@@ -180,6 +180,13 @@ module Bundler
       gemspec.name
     end
 
+    def sh_with_input(cmd)
+      Bundler.ui.debug(cmd)
+      SharedHelpers.chdir(base) do
+        abort unless Kernel.system(*cmd)
+      end
+    end
+
     def sh(cmd, &block)
       out, status = sh_with_status(cmd, &block)
       unless status.success?
diff --git a/spec/bundler/gem_helper_spec.rb b/spec/bundler/gem_helper_spec.rb
index dc982c6ee7..4fa4831c59 100644
--- a/spec/bundler/gem_helper_spec.rb
+++ b/spec/bundler/gem_helper_spec.rb
@@ -11,6 +11,7 @@ RSpec.describe Bundler::GemHelper do
   before(:each) do
     global_config "BUNDLE_GEM__MIT" => "false", "BUNDLE_GEM__TEST" => "false", "BUNDLE_GEM__COC" => "false"
     bundle "gem #{app_name}"
+    prepare_gemspec(app_gemspec_path)
   end
 
   context "determining gemspec" do
@@ -218,15 +219,26 @@ RSpec.describe Bundler::GemHelper do
           end
         end
 
-        it "on releasing" do
-          mock_build_message app_name, app_version
-          mock_confirm_message "Tagged v#{app_version}."
-          mock_confirm_message "Pushed git commits and tags."
-          expect(subject).to receive(:rubygem_push).with(app_gem_path.to_s)
+        context "on releasing" do
+          before do
+            mock_build_message app_name, app_version
+            mock_confirm_message "Tagged v#{app_version}."
+            mock_confirm_message "Pushed git commits and tags."
 
-          Dir.chdir(app_path) { sys_exec("git push -u origin master") }
+            Dir.chdir(app_path) { sys_exec("git push -u origin master") }
+          end
 
-          Rake.application["release"].invoke
+          it "calls rubygem_push with proper arguments" do
+            expect(subject).to receive(:rubygem_push).with(app_gem_path.to_s)
+
+            Rake.application["release"].invoke
+          end
+
+          it "uses Kernel.system" do
+            expect(Kernel).to receive(:system).with("gem", "push", app_gem_path.to_s, "--host", "http://example.org").and_return(true)
+
+            Rake.application["release"].invoke
+          end
         end
 
         it "even if tag already exists" do
@@ -249,7 +261,7 @@ RSpec.describe Bundler::GemHelper do
       before(:each) do
         Rake.application = Rake::Application.new
         subject.install
-        allow(subject).to receive(:sh)
+        allow(subject).to receive(:sh_with_input)
       end
 
       after(:each) do
diff --git a/spec/commands/newgem_spec.rb b/spec/commands/newgem_spec.rb
index 2914ba66c5..fada9c8bfc 100644
--- a/spec/commands/newgem_spec.rb
+++ b/spec/commands/newgem_spec.rb
@@ -182,19 +182,7 @@ RSpec.describe "bundle gem" do
     in_app_root
     bundle! "gem newgem --bin"
 
-    process_file(bundled_app("newgem", "newgem.gemspec")) do |line|
-      # Simulate replacing TODOs with real values
-      case line
-      when /spec\.metadata\["(?:allowed_push_host|homepage_uri|source_code_uri|changelog_uri)"\]/, /spec\.homepage/
-        line.gsub(/\=.*$/, "= 'http://example.org'")
-      when /spec\.summary/
-        line.gsub(/\=.*$/, "= %q{A short summary of my new gem.}")
-      when /spec\.description/
-        line.gsub(/\=.*$/, "= %q{A longer description of my new gem.}")
-      else
-        line
-      end
-    end
+    prepare_gemspec(bundled_app("newgem", "newgem.gemspec"))
 
     Dir.chdir(bundled_app("newgem")) do
       gems = ["rake-10.0.2", :bundler]
diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb
index 8af2b1ff2d..1e6b70b471 100644
--- a/spec/support/helpers.rb
+++ b/spec/support/helpers.rb
@@ -543,6 +543,22 @@ module Spec
       Dir[pattern].each(&chmod[0o755, 0o644])
     end
 
+    # Simulate replacing TODOs with real values
+    def prepare_gemspec(pathname)
+      process_file(pathname) do |line|
+        case line
+        when /spec\.metadata\["(?:allowed_push_host|homepage_uri|source_code_uri|changelog_uri)"\]/, /spec\.homepage/
+          line.gsub(/\=.*$/, "= 'http://example.org'")
+        when /spec\.summary/
+          line.gsub(/\=.*$/, "= %q{A short summary of my new gem.}")
+        when /spec\.description/
+          line.gsub(/\=.*$/, "= %q{A longer description of my new gem.}")
+        else
+          line
+        end
+      end
+    end
+
     def process_file(pathname)
       changed_lines = pathname.readlines.map do |line|
         yield line