This repository has been archived by the owner on Apr 14, 2021. It is now read-only.
Fix GemHelper to allow reading Input/Output #7108
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm not particularly sure how to go about testing this, any ideas welcome. |
|
I made one suggestion in the other PR. In particular, have a look at the specs that call the |
ghost
pushed a commit
that referenced
this pull request
Apr 9, 2019
7109: Backport `GemHelper` changes for #7108 r=colby-swandale a=colby-swandale I'm working on getting #7108 into the v2.0.2 release but that code has not been kept up to date over time (i'm not sure why i haven't been doing this). Trying to cherry-pick the changes to `GemHelper` is proving to be not super fun to deal with. Instead, i'm just going to copy/paste the current file & specs into `2-0-stable` There doesn't seem to be any breaking changes, so this should be ok to perform. Co-authored-by: Colby Swandale <[email protected]>
ghost
pushed a commit
that referenced
this pull request
Apr 9, 2019
7109: Backport `GemHelper` changes for #7108 r=colby-swandale a=colby-swandale I'm working on getting #7108 into the v2.0.2 release but that code has not been kept up to date over time (i'm not sure why i haven't been doing this). Trying to cherry-pick the changes to `GemHelper` is proving to be not super fun to deal with. Instead, i'm just going to copy/paste the current file & specs into `2-0-stable` There doesn't seem to be any breaking changes, so this should be ok to perform. Co-authored-by: Colby Swandale <[email protected]>
ghost
pushed a commit
that referenced
this pull request
Apr 9, 2019
7109: Backport `GemHelper` changes for #7108 r=colby-swandale a=colby-swandale I'm working on getting #7108 into the v2.0.2 release but that code has not been kept up to date over time (i'm not sure why i haven't been doing this). Trying to cherry-pick the changes to `GemHelper` is proving to be not super fun to deal with. Instead, i'm just going to copy/paste the current file & specs into `2-0-stable` There doesn't seem to be any breaking changes, so this should be ok to perform. Co-authored-by: Colby Swandale <[email protected]>
colby-swandale
added a commit
that referenced
this pull request
Apr 9, 2019
Backport `GemHelper` changes for #7108
This was referenced Jun 4, 2019
ghost
pushed a commit
that referenced
this pull request
Jun 12, 2019
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 #6854 (comment). `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 <[email protected]>
colby-swandale
pushed a commit
that referenced
this pull request
Jun 12, 2019
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 #6854 (comment). `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 <[email protected]> (cherry picked from commit cd05f13)
|
Superseded by #7199. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was the end-user problem that led to this PR?
Note: This PR is a super seed of #7005.
Currently, users that have enabled 2FA in RubyGems.org with both Web+API are not able to upload their gems via
rake release, due to a bug inGemHelper#sh_with_statusnot handling input/output correctly.What was your diagnosis of the problem?
See #6854
What is your fix for the problem, implemented in this PR?
See #6854 as this PR is mostly the same.
The major difference here is that i've merged the change into
#sh_with_statusand that i'm usingIO#getcinstead ofIO#gets. This is because commands could potentially be asking for input on the same line as output, resulting in output that still hasn't been printed.Example using
passwdCloses #6854 #7005