(PDK-641) Make pdk bundle fully interactive
#712
Conversation
|
Still need to add some unit tests for the new class but wanted to get this up for some conceptual review. Tested the approach on macOS, Linux, and Windows and it seems to work. (Tested by adding a |
| @process = ChildProcess.build(*@argv) | ||
| @process.leader = true | ||
|
|
||
| @stdout = Tempfile.new('stdout').tap { |io| io.sync = true } |
There was a problem hiding this comment.
Warning: Tempfile doesn't set binary mode, so CRLF will be used on Windows which may not be what you want.
There was a problem hiding this comment.
The stuff in lib/pdk/cli/exec/command.rb was extracted out into it's own file but pretty much unchanged from the current implementation. So this is a good thing to know but shouldn't be a regression from current behavior.
| PDK::Util::RubyVersion.gem_paths_raw.map { |gem_path| File.join(gem_path, 'bin') }, | ||
| package_binpath, | ||
| PDK::Util.package_install? ? PDK::Util::Git.git_paths : nil, | ||
| ENV['PATH'], |
There was a problem hiding this comment.
Warning: ENV has known issues with non-ASCII characters on Windows environment variables. https://github.com/puppetlabs/puppet/blob/eb19bedb221959bd202e598401a3d29ea77950c9/spec/unit/util_spec.rb#L99-L218
Typically this comes from non-English Usernames e.g. C:\Users\jmüller
There was a problem hiding this comment.
Similar to above, good thing to know but shouldn't be a regression since this is the existing implementation.
|
Manually tested with cmd.exe, powershell.exe, conemu and new windows terminal. Confirmed I can use pry interactively within a |
Previously, the subprocess invoked by `pdk bundle` had its output streams buffered as a side effect of the subprocess infrastructure used for other commands. Additionally, the input stream was not connected to the subprocess at all. This commit adds a new subclass of `PDK::CLI::Exec::Command` called `PDK::CLI::Exec::InteractiveCommand` that bypasses all of the output buffering, spinners, etc. that the parent class implements and instead uses Ruby's built-in `Kernel.system` method to run the subprocess fully interactively. The `pdk bundle` subcommand is also updated to use this new class for invoking Bundler. If this approach proves successful, it will likely be conditionally extended to certain invocations of `pdk test unit` in a future PR.
scotje
force-pushed
the
641_pdk_bundle_passthru
branch
from
3f4e346 to
de03313
Compare
|
Updated with unit tests, I'm wondering how necessary acceptance/package tests are since it would mostly be testing that |
|
We decided to separately ticket automated acceptance tests for this, so I think this is ready to merge. |
Previously, the subprocess invoked by
pdk bundlehad its outputstreams buffered as a side effect of the subprocess infrastructure used
for other commands. Additionally, the input stream was not connected to
the subprocess at all.
This commit adds a new subclass of
PDK::CLI::Exec::CommandcalledPDK::CLI::Exec::InteractiveCommandthat bypasses all of the outputbuffering, spinners, etc. that the parent class implements and instead
uses Ruby's built-in
Kernel.systemmethod to run the subprocess fullyinteractively. The
pdk bundlesubcommand is also updated to use thisnew class for invoking Bundler.
If this approach proves successful, it will likely be conditionally
extended to certain invocations of
pdk test unitin a future PR.