Allow specifying stdin #360

sethvargo · 2020-02-20T19:23:21Z

sethvargo · 2020-02-20T19:24:21Z

packages/exec/src/interfaces.ts

@@ -30,6 +30,9 @@ export interface ExecOptions {
  /** optional. How long in ms to wait for STDIO streams to close after the exit event of the process before terminating. defaults to 10000 */
  delay?: number

+  /** optional. input to write to the process on STDIN. */
+  stdin?: Buffer


I kept going back-and-forth about whether to accept a Buffer or Stream here. I think Buffer is a better API for most users, but it's not quite as flexible as giving users the ability to specify a stream.

I agree that buffer is the better api for most users. Its simple and should work for most use cases here. If we end up implementing more complicated scenarios with streams in the future, we can always support passing in a stream or a buffer mutually exclusive. We some use cases for that scenario

@bryanmacfarlane , FYI.

Are we sure this is a buffer? subprocess.stdin advertises it as a writable stream. Another reason I wanted to see a pure js test instead of the script. https://nodejs.org/api/child_process.html#child_process_subprocess_stdin

The input we are allowing a user to pass in here is a buffer.

We write the contents of that buffer to the process's StdIn Stream.

Very similar to how node does this with the input option you outlined below, which also takes a string and various other inputs.
https://nodejs.org/api/child_process.html#child_process_child_process_execfilesync_file_args_options

Sorry I just realized node also has input option. Should we just pass the value through and let node handle writing to the stream?

...that said, seems like would be an implementation detail and wouldn't change behavior exposed by this PR. So i think this PR is OK the way it is (incremental)

input doesn't appear to be available for any of the async process creation function. In particular we use spawn. https://nodejs.org/api/child_process.html#child_process_child_process_spawn_command_args_options

oh weird, didnt realize it was only on the sync functions. makes sense now why we dont pass thru :)

sethvargo · 2020-02-20T19:37:03Z

Test failure seems unrelated?

joshmgross · 2020-02-24T18:26:48Z

👋 Thanks for this! I've added @bryanmacfarlane and @ericsciple to help review this change.

ericsciple · 2020-02-28T18:34:10Z

adding to @thboop's radar

ericsciple · 2020-02-28T18:41:22Z

Would be great to solve in a way that enables #346 (chain output of one tool runner to input of second)

Might be good to start with generic bare minimum to enable the scenario, before adding specific helpers like "execWithPiping".

Workaround today, piping can be achieved today by running command line, but then lose arg escaping goodness built into tool runner.

Extremely leary of adding piping into toolrunner. Was in a lib before and weird race never solved. I think the second process shouldnt be started until output is actually ready from the first process? Worried about adding solution into toolrunner but not finishing.

sethvargo · 2020-02-28T19:28:47Z

Hey @ericsciple thanks for the feedback. I think this does solve it in a way that supports #346. This is a lower-level API that enables Action authors to accept an input stream in their action. The framework itself could be extended to leverage this API to chain action output, but I don't think that belongs in this PR.

The proposed solution in #346 wouldn't solve the use case this PR is solving. I have actions that invoke a command. The command accepts either a filepath on disk or input on stdin. The current action performs additional work to write the contents to disk before executing the command, but the extra steps could be removed if the toolrunner accepted a stream instead.

It's not feasible to change the command's behavior
Accepting stdin on a pipe is quite common among many commands
Altering the command to run echo 'thing' | command isn't cross-platform compatible and relies on shell-specifics (like pipes), in addition to your mention of arg escaping; spawning that additional subcommand is an unnecessary fork if the process just accepts stdin
Writing files to disk is sometimes seen as a security hole, and the Actions framework makes it trivally easy to create a world-readable file

ericsciple · 2020-03-02T14:04:55Z

@sethvargo sorry i intended much of the feedback for @thboop to consider when reviewing.

@thboop two scenarios i think we should cover: stdin, and piping.

craigdbarber · 2020-04-03T21:32:17Z

@imjohnbo could you PTAL.

thboop

Handles stdin is failing on windows. Do you have some time to fix that? If not, I can take over the PR to get this merged in.

Thanks for the contribution!

packages/exec/__tests__/exec.test.ts

thboop · 2020-04-08T22:32:10Z

packages/exec/src/interfaces.ts

@@ -30,6 +30,9 @@ export interface ExecOptions {
  /** optional. How long in ms to wait for STDIO streams to close after the exit event of the process before terminating. defaults to 10000 */
  delay?: number

+  /** optional. input to write to the process on STDIN. */
+  stdin?: Buffer


I agree that buffer is the better api for most users. Its simple and should work for most use cases here. If we end up implementing more complicated scenarios with streams in the future, we can always support passing in a stream or a buffer mutually exclusive. We some use cases for that scenario

@bryanmacfarlane , FYI.

packages/exec/src/toolrunner.ts

sethvargo · 2020-04-08T23:20:21Z

@thboop updated. seems happy now.

packages/exec/src/toolrunner.ts

bryanmacfarlane · 2020-04-09T17:28:28Z

packages/exec/__tests__/exec.test.ts

+    if (IS_WINDOWS) {
+      command = 'wait-for-input.cmd'
+    } else {
+      command = 'wait-for-input.sh'


I would like to see a test that shows what an action author would do. e.g. all js instead of the sh and cmd scripts.

I added a js-specific test, although I admit I'm not sure exactly what you're asking. All of these tests spawn a childprocess using exec - there's no "all js". The test spawns a node subprocess with a node file that prints what it gets on stdin.

packages/exec/src/interfaces.ts

bryanmacfarlane · 2020-04-09T17:31:32Z

packages/exec/src/toolrunner.ts

@@ -524,6 +524,14 @@ export class ToolRunner extends events.EventEmitter {
          resolve(exitCode)
        }
      })
+
+      if (this.options.stdin) {
+        if (!cp.stdin) {


is this a buffer or a stream? https://nodejs.org/api/child_process.html#child_process_subprocess_stdin

Responded to this above: #360 (comment)

thboop

LGTM

ericsciple

I think this change is good.

It seems like future goal is likely to align more with node's options. That is, allow more types for input (whatever types node allows) and passthru to node.

As long as we're convinced this PR doesn't prevent ^, then i think this incremental change sounds fine.

bryanmacfarlane · 2020-04-10T18:31:56Z

added to todo list for one last look

sethvargo · 2020-04-12T18:29:09Z

Yay! Looks like we're all green 🟢

actions/toolkit#360 (comment)

Allow specifying stdin

fd4bf8a

Fixes GH-164

sethvargo commented Feb 20, 2020

View reviewed changes

Fix linter error

d9229d3

sethvargo requested a review from joshmgross February 24, 2020 18:18

joshmgross requested review from bryanmacfarlane and ericsciple February 24, 2020 18:26

This comment has been minimized.

Sign in to view

ghost approved these changes Feb 28, 2020

View reviewed changes

ericsciple requested a review from thboop February 28, 2020 18:33

ghost approved these changes Feb 29, 2020

View reviewed changes

sittie82 approved these changes Feb 29, 2020

View reviewed changes

thboop self-assigned this Mar 2, 2020

thboop requested changes Apr 8, 2020

View reviewed changes

sethvargo added 2 commits April 8, 2020 19:15

Handle Windows in tests

80762b5

Just end directly

c1aa1b4

sethvargo requested a review from thboop April 8, 2020 23:16

ericsciple reviewed Apr 9, 2020

View reviewed changes

packages/exec/src/toolrunner.ts Outdated Show resolved Hide resolved

sethvargo requested a review from ericsciple April 9, 2020 13:22

Do not set a default encoding

cb07748

bryanmacfarlane reviewed Apr 9, 2020

View reviewed changes

packages/exec/src/interfaces.ts Outdated Show resolved Hide resolved

bryanmacfarlane reviewed Apr 9, 2020

View reviewed changes

sethvargo added 4 commits April 9, 2020 13:53

Rename to input

13d5ff2

Add node-specific test

0fb84a5

Fix EOL on windows

76f6c9a

Use process.write instead

ee69942

sethvargo requested a review from bryanmacfarlane April 9, 2020 18:15

thboop approved these changes Apr 9, 2020

View reviewed changes

ericsciple approved these changes Apr 10, 2020

View reviewed changes

joshmgross removed their request for review April 10, 2020 19:04

bryanmacfarlane approved these changes Apr 12, 2020

View reviewed changes

thboop merged commit c4b6011 into actions:master Apr 13, 2020

sethvargo deleted the sethvargo/stdin branch April 13, 2020 18:31

smorimoto referenced this pull request Aug 5, 2020

bump exec version

4e734dc

domdfcoding mentioned this pull request May 1, 2021

Update @actions/exec to avoid passing the docker password on the command line satackey/push-prebuilt-action#134

Open

akiojin added a commit to akiojin/store-git-credential-github-action that referenced this pull request Feb 21, 2022

input の戻り値は Buffer

c3698c1

actions/toolkit#360 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying stdin #360

Allow specifying stdin #360

sethvargo commented Feb 20, 2020

sethvargo Feb 20, 2020

thboop Apr 8, 2020

bryanmacfarlane Apr 9, 2020

thboop Apr 9, 2020

ericsciple Apr 9, 2020

ericsciple Apr 10, 2020

thboop Apr 10, 2020

ericsciple Apr 10, 2020

sethvargo commented Feb 20, 2020

joshmgross commented Feb 24, 2020

This comment has been minimized.

This comment has been minimized.

ericsciple commented Feb 28, 2020

ericsciple commented Feb 28, 2020

sethvargo commented Feb 28, 2020

ericsciple commented Mar 2, 2020

craigdbarber commented Apr 3, 2020

thboop left a comment

thboop Apr 8, 2020

sethvargo commented Apr 8, 2020

bryanmacfarlane Apr 9, 2020

sethvargo Apr 9, 2020

bryanmacfarlane Apr 9, 2020

thboop Apr 9, 2020

thboop left a comment

ericsciple left a comment

bryanmacfarlane commented Apr 10, 2020

sethvargo commented Apr 12, 2020

Allow specifying stdin #360

Allow specifying stdin #360

Conversation

sethvargo commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethvargo commented Feb 20, 2020

joshmgross commented Feb 24, 2020

This comment has been minimized.

This comment has been minimized.

ericsciple commented Feb 28, 2020

ericsciple commented Feb 28, 2020

sethvargo commented Feb 28, 2020

ericsciple commented Mar 2, 2020

craigdbarber commented Apr 3, 2020

thboop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethvargo commented Apr 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thboop left a comment

Choose a reason for hiding this comment

ericsciple left a comment

Choose a reason for hiding this comment

bryanmacfarlane commented Apr 10, 2020

sethvargo commented Apr 12, 2020