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 output malformed when wrapper enabled #367

Merged
merged 5 commits into from
Oct 27, 2023

Conversation

OJFord
Copy link
Contributor

@OJFord OJFord commented Oct 25, 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 (#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, #42, #167

@OJFord OJFord requested a review from a team as a code owner October 25, 2023 16:41
@OJFord OJFord force-pushed the fix-wrapper-output branch from bb87bf5 to 0d847da Compare October 25, 2023 17:58
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

Hey there @OJFord 👋🏻 , thanks for the PR.

Trying to digest these issues and your PR. After taking a glance into the underlying @actions/exec package, it looks like currently, the package outputs to STDOUT by default... however, that output seems to also include a printing of the command it's running (odd default behavior!).

Current:
Screenshot 2023-10-25 at 2 13 45 PM

As a result, running your PR'd changes just outputs the STDOUT twice. I ran your PR's changes against my own repo action as a test if you'd like to see the output:

image

Looking at @actions/exec options, we may be able to suppress the default STDOUT writing with the silent option, then use your changes here to output the correct STDOUT/STDERR.

https://github.com/hashicorp/setup-terraform/blob/599d3831962aa3431ae84f19b03fea842780ad4a/wrapper/terraform.js#L34C1-L37

@OJFord
Copy link
Contributor Author

OJFord commented Oct 26, 2023

Thanks! Will have a look.

@OJFord OJFord force-pushed the fix-wrapper-output branch from 0d847da to f28aa14 Compare October 26, 2023 16:49
@OJFord OJFord changed the title Fix output swallowed when wrapper enabled Fix output mangled when wrapper enabled Oct 26, 2023
@OJFord OJFord changed the title Fix output mangled when wrapper enabled Fix output malformed when wrapper enabled Oct 26, 2023
@OJFord OJFord force-pushed the fix-wrapper-output branch from f28aa14 to adb3460 Compare October 26, 2023 16:54
@OJFord OJFord requested a review from austinvalle October 26, 2023 16:55
@OJFord
Copy link
Contributor Author

OJFord commented Oct 26, 2023

Thanks for that - I've worked around it as you suggested and proposed a fix (write it to stderr, not stdout) upstream. I've updated the commit message/PR description to reflect that too, sorry that slightly breaks the context of your comment.

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
Copy link
Member

Alright, more weirdness coming from the wrapper (some of this is mentioned in the GH issues linked in the description).

So I added a simple test with JQ to this PR so we can prove the STDOUT is being printed without any unwanted characters.

You can see with the wrapper, we still get an error (although JQ still processes the command properly), however the actual output looks correct 🤔 (vs. the non-wrapper equivalent that is successful).

After some debugging and looking through the linked issues, I found that there actually is hidden output because of the core.debug lines in the wrapper (which GH action's logs hides by default 😄 )
https://github.com/austinvalle/terraform-provider-sandbox/actions/runs/6660856553/job/18102722958

$ terraform output -json

{
  "pet_name": {
    "sensitive": false,
    "type": "string",
    "value": "balanced-marmot"
  }
}
::debug::Terraform exited with code 0.
::debug::stdout: {%0A  "pet_name": {%0A    "sensitive": false,%0A    "type": "string",%0A    "value": "balanced-marmot"%0A  }%0A}%0A
::debug::stderr: 
::debug::exitcode: 0

These core.debug lines aren't really needed imo, so I just removed them in fd5f36a 🙂. And now we get all green 🥳

@austinvalle austinvalle added this to the v3.0.0 milestone Oct 27, 2023
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this @OJFord! Will look to release this with v3.0.0 next week!

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2024
@OJFord OJFord deleted the fix-wrapper-output branch May 23, 2024 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raw output when using wrapper with $() in bash
2 participants