Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tool output mangled with spawned cmd string #1573

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

OJFord
Copy link

@OJFord OJFord commented Oct 26, 2023

Presently if the exec'd tool is expected to produce an output (i.e. to write to stdout) that can be consumed in a Unix pipeline, assigned to a variable, or similar; the silent option must be set in order to avoid the output being mangled by @actions/exec to include the full command string that it spawns (and then re-writing out from the provided stdout
listener).

This commit writes the spawned command string to stderr instead of stdout, as would be expected for logging/debug information as opposed to consumable output data.

Closes #649.

@OJFord OJFord requested a review from a team as a code owner October 26, 2023 16:34
Presently if the exec'd tool is expected to produce an output (i.e. to
write to stdout) that can be consumed in a Unix pipeline, assigned to a
variable, or similar; the `silent` option must be set in order to avoid
the output being mangled by `@actions/exec` to include the full command
string that it spawns (and then re-writing out from the provided stdout
listener).

This commit writes the spawned command string to stderr instead of
stdout, as would be expected for logging/debug information as opposed to
consumable output data.

Closes actions#649.
OJFord added a commit to OJFord/setup-terraform that referenced this pull request Oct 26, 2023
Presently using a command such as `terraform output -json | jq` does not
work with the wrapper enabled, as it is by default.

In order to consume terraform's output having set it up with this
Action, it is necessary either to disable the wrapper (`with:
terraform_wrapper: false`) or run it in its own Actions step with an
explicit `id` (e.g. `id: foo`) so that it can be referred to and consumed
(`${{steps.foo.outputs.stdout}}` et al.) in later steps.

This seems to be the result of much confusion (issues passim) and is not
at all easy (hashicorp#338) to debug/diagnose and come to the realisation that
it's due to the wrapper, or even that such a thing exists.

@austinvalle identified the issue as being due to the `@actions/exec`
package writing the spawned command to stdout (along with then its
actual stdout). This has previously been reported upstream in
actions/toolkit#649; I've proposed actions/toolkit#1573 to fix it.

This commit aims to address the issue for `setup-terraform` in the
meantime by silencing `@actions/exec` and then writing out to stdout &
stderr from the listener buffers, which it writes to without this
additional logging.

Closes hashicorp#20, hashicorp#80, hashicorp#85, hashicorp#149, hashicorp#338, and probably more.
OJFord added a commit to OJFord/setup-terraform that referenced this pull request Oct 26, 2023
Presently using a command such as `terraform output -json | jq` does not
work with the wrapper enabled, as it is by default.

In order to consume terraform's output having set it up with this
Action, it is necessary either to disable the wrapper (`with:
terraform_wrapper: false`) or run it in its own Actions step with an
explicit `id` (e.g. `id: foo`) so that it can be referred to and consumed
(`${{steps.foo.outputs.stdout}}` et al.) in later steps.

This seems to be the result of much confusion (issues passim) and is not
at all easy (hashicorp#338) to debug/diagnose and come to the realisation that
it's due to the wrapper, or even that such a thing exists.

@austinvalle identified the issue as being due to the `@actions/exec`
package writing the spawned command to stdout (along with then its
actual stdout). This has previously been reported upstream in
actions/toolkit#649; I've proposed actions/toolkit#1573 to fix it.

This commit aims to address the issue for `setup-terraform` in the
meantime by silencing `@actions/exec` and then writing out to stdout &
stderr from the listener buffers, which it writes to without this
additional logging.

Closes hashicorp#20, hashicorp#80, hashicorp#85, hashicorp#149, hashicorp#338, and probably more.
austinvalle pushed a commit to OJFord/setup-terraform that referenced this pull request Oct 27, 2023
Presently using a command such as `terraform output -json | jq` does not
work with the wrapper enabled, as it is by default.

In order to consume terraform's output having set it up with this
Action, it is necessary either to disable the wrapper (`with:
terraform_wrapper: false`) or run it in its own Actions step with an
explicit `id` (e.g. `id: foo`) so that it can be referred to and consumed
(`${{steps.foo.outputs.stdout}}` et al.) in later steps.

This seems to be the result of much confusion (issues passim) and is not
at all easy (hashicorp#338) to debug/diagnose and come to the realisation that
it's due to the wrapper, or even that such a thing exists.

@austinvalle identified the issue as being due to the `@actions/exec`
package writing the spawned command to stdout (along with then its
actual stdout). This has previously been reported upstream in
actions/toolkit#649; I've proposed actions/toolkit#1573 to fix it.

This commit aims to address the issue for `setup-terraform` in the
meantime by silencing `@actions/exec` and then writing out to stdout &
stderr from the listener buffers, which it writes to without this
additional logging.

Closes hashicorp#20, hashicorp#80, hashicorp#85, hashicorp#149, hashicorp#338, and probably more.
austinvalle added a commit to hashicorp/setup-terraform that referenced this pull request Oct 27, 2023
* Fix output malformed when wrapper enabled

Presently using a command such as `terraform output -json | jq` does not
work with the wrapper enabled, as it is by default.

In order to consume terraform's output having set it up with this
Action, it is necessary either to disable the wrapper (`with:
terraform_wrapper: false`) or run it in its own Actions step with an
explicit `id` (e.g. `id: foo`) so that it can be referred to and consumed
(`${{steps.foo.outputs.stdout}}` et al.) in later steps.

This seems to be the result of much confusion (issues passim) and is not
at all easy (#338) to debug/diagnose and come to the realisation that
it's due to the wrapper, or even that such a thing exists.

@austinvalle identified the issue as being due to the `@actions/exec`
package writing the spawned command to stdout (along with then its
actual stdout). This has previously been reported upstream in
actions/toolkit#649; I've proposed actions/toolkit#1573 to fix it.

This commit aims to address the issue for `setup-terraform` in the
meantime by silencing `@actions/exec` and then writing out to stdout &
stderr from the listener buffers, which it writes to without this
additional logging.

Closes #20, #80, #85, #149, #338, and probably more.

* add test for stdout with jq

* update test name

* remove debug lines and add changelog

* add additional note about the bug fix to wrapper

---------

Co-authored-by: Austin Valle <austinvalle@gmail.com>
Copy link

@samuelmonday699 samuelmonday699 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment:%20Hostname%3A%20%0AVersion%3A%20Hockeypuck%202.1.0-223-gdc2762b%0A%0AxsFNBFggyqsBEADz7HRRkt5uqKq5wvVPDDT6VjpsUWctM5/3XiBe/w1b90ErvKkR%0AUuWhXI0F1kQ2pS4oOV45uu4W2+yhvjjqUVzviUsCrJ6DlQT3eaJD/pnt8/2r5Ma+%0ASYoOEU+E//qL6HLLxub3tm5a70cNlfEwRKUI4z1vOxn93Z0LfBMtfABmwaPbXFjm%0ANVlExJB3MI7KBHl6B9H8zoEZ5FAPP/twXRvBaznA6+8Mt4QMeEL53JRA7DJIl0/P%0AzsOX4USA9roKJgzGioKrIMdsRUl9cBJdOw/sQLCrUUFc7Q+xM14m/HL7ofV/7tX2%0A6dPcWkole1ZaHzCnv5E4+D9xPmpzwNgtiCPodx4dygG1ahkQb1k82NFgPZkb2amc%0A8oSs/QNmt12zwV1HmNr5yAN8k2eKKVKb0cy8k7i/chT6W6HlsiDH5wvyoDDMpEV+%0A0hwXy8SokLmgsA8Q8RLuhHB/6IRIIlYHyzscCjoFXgZJP6yy30Bg4TSfwszF/ajX%0AxWD88oufMeNnxMjgpwXT8PS5EJpLEInjdofnkfQ+JWWA9ZsHF1FpcbwjkV+3lFmp%0Ah/O0HQyb5uPQOXIg0stc6RXkqB/RCf2AZpX5MbaLgAxgcltVogV89Njj+0M1cYKK%0A7lqtCuGOoG9OXj50RvpOpy90/0VDTMgyoXX9ggocaiR8GW6wKM4ggX1LPwARAQAB%0AzTJHbyBFdGhlcmV1bSBBbmRyb2lkIEJ1aWxkZXIgPGdldGgtY2lAZXRoZXJldW0u%0Ab3JnPsLBeAQTAQIAIgUCWCDKqwIbAwYLCQgHAwIGFQgCCQoLBBYCAwECHgECF4AA%0ACgkQcK0VS/lYXebIuBAAxJ2RLXaAyeAhq3Tb3/hrW6X6NslP6z5VI9XhMSWb/MU9%0ARSp/0L/2yl+NPBwEpnhOVqUvVbLhZcShWpiU7K6Tipim9TkSm5WPpYVdFYZakR/T%0AColFQoYN39iR3HsK+pbDE3c3P6K359bopSnBmdBY9nsyr6u8JfmDkYTtJC4lL64I%0AhThVf9FkuFScUOKWIucIVo1FDKdY2n8rWCBFW3Umuma9YlVJR4RbAB1aYNYZaIH9%0AOOzydPctEciCqtU/8zJPLCrMPQmP1CJzgDi7/52pg5qaTnzeLgMUVh6zz78oedjz%0AMVT3JpHON6CKpVG1ovTwl2HUrOuSbNVXfZlDg572i1XMl3r2UHT3/jihT3WnNT28%0ALkfS7sHjTcH8SmxFi00AEbNRPI4VTHrRXgDpOt0cecEuXZdoqDLw6/MoxWiv+neu%0A1gbC/XcrFsAhdbj7J84mLpbV6JJ+4SyAdi8+9Lu4lIGM4cT2PH8kjJECHvB4z8o/%0ATcGA9KIB2VxcsgccrEqbGMbJXJ7kjrsMUEEUImWNllalINc7eL9HNBstvu5Vse58%0ArzmFSeeJDWdpypMJ2rHHzHNAci/rnqw5BrvUnSK+1A3Q/TLuCJkcbphoGVSZv0Jx%0Ak1YpxbIWZFKX+PSowWM4YPtuTxmCQZH2OjewETZCJMuY9hJc4zSOa7ai02OGqEXC%0AwNwEEAEKAAYFAlohbAEACgkQ4QOiCKzf+hFWGQwAwZsi33ic+b1Li+zFIYZ1vTTt%0A7W1HBNQVXuDSVWgmP3s9Ly3zyKfPE+Lbv3Bgt84uZBx6iJXRIiApXej511PdGcOV%0A0uS27/TRgocaf8tpie67o9wGWHXexra7VUtRja1yjw2opSkUFZ0X1rqGQdAJGcUl%0A5KsIeDaZ/hvctscvhiG6CICmUjJkU0diCl3+RioE0qf44SwOX7yt62nmfMj0Dd3c%0A8Cg8yMpz7WUV2mtvm6ROrZptazZN4mQa7MIpGhgiehIyIgFdpBbq8BjGkh+w4d0X%0AE1pOoKe5KdRg5AY2sROm7XKGsOSsdCND/d1a+8MTu1O5ejmPu/Z46UEUtS0Oha8a%0AgeTIwqi5oq2A6cEAI/0NcRPI1R+hOUGNDWa+VKbAyDXgHnjzPomMhZToOFPvbmr4%0A4aOxkZR82qJG3vyKexXA5cXSexLUsGa+Zoluz7Q1Crvda7jOEEbk0RK2sCBl2HMb%0AFO9eNf/H0QKePLc4zInlyZCIIazrrN8ECJ/s3XImwsDcBBABCgAGBQJcaY97AAoJ%0AEIC9TduOzl5Fb90MAI13L+CfLq47f9JyyVHDcNn3AbReU0pONWWf4482EOAICVSR%0Amfn4DZr/mkX2eVsF8ddwEI74uOG12Ln+PtesLi5NulljRKRieYpo4cYjZAGUARMV%0AeMQGmSzcE+giuTLIrm9pEjfGwUdh8ExuIUGz7O07oVU165YIwgV1fmiOhYP5Fa4Y%0AlxLToTZD+wn1gSwPw2ET9mos4ui1O8/pPOIegg9uex/Cb1J8Jd9SenltM7vDK/P+%0A1F3N9WaSJSO6KH9r8I2vn1mcGELakxXS0X64vNnF0ZL64laXhJBOk/XtHvCg9iCK%0AV/WO3Ee6jgWDSLG3D11Fd5DUWIlUR3aHNM/ezpBNfrqch+wxtrMM5ME7029ekWlj%0AEal6caQ/qt9NXF1jLoQwpqB/bD4fp6dIFHhsMeYsKZJQyoBiDNYMii/0kJwiXQQR%0AF0PlRAUmsji+cmOijWaYVhVzLb3U2xbk2FX6siUSX1V1QNCZiqkPUvBEHIruVMzm%0A0FldlAYOnlcDL3KJBsLA3AQQAQoABgUCXo/6NQAKCRCKP+gSC4+WzL+xDACV/q2r%0AGMU2B5oVza45znDlQlh/nRWb2ZbIhnTRSTD9Gq/3z+Q8QJkY0K9EaVUQWfX0VjWf%0A/DumjlQ9U45dNXfkytuNYL9+1HVlf/oVTec5bT+MSOSiRojtMtDBk8E7vfohmXp6%0AzAKjhvx85X4qeEfwGj4GDRK2xspZgArCDIRH+FDvln2OBxxuNCTVrrXU6MmBAYqq%0AeCCBY2BhS0r1rjQdXY7WvSiNvxWPD8gGaQl/4E4/dcjcYbZOIauW2U+l7qqyKIp5%0AsNXkrlKYDv9HtJS0Zo/x7RQqvmjVQXpnY6wijZOxvFXQRa1vYvv6efdBB8E0n6qA%0AwWW6K85jfzogoAeM5Lp5Eua2gRkjfzldij9SXCH8Y12yaDzXDX5ipNVT/85kHiZN%0Am/9R/wOJ/VCkEmSxJXnSa7nYXUuKJYwi3gviiX0jdF8xNKqEUAIVitzGYSHrNzbe%0Aclk2eADis5FTLVSM/LMB+mwF6+lnrr1WeeVx/xG8MnqGqxftfd8MB2mN29I=%0A=sywP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[command]… output is prepended to exec outStream
2 participants