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

Changing the wrapper to show real-time output #395

Closed
dannystaple opened this issue Feb 15, 2024 · 11 comments · Fixed by #410
Closed

Changing the wrapper to show real-time output #395

dannystaple opened this issue Feb 15, 2024 · 11 comments · Fixed by #410
Labels
bug Something isn't working
Milestone

Comments

@dannystaple
Copy link

Context

For lengthy apply's, the wrapper will mean that no output is shown until the end.
This is due to the code https://github.com/hashicorp/setup-terraform/blob/main/wrapper/terraform.js using a listener, which looks like it buffers up everything, and then prints it to the console at https://github.com/hashicorp/setup-terraform/blob/main/wrapper/terraform.js#L42.

Potential fix

What would work here, would be to update the listener https://github.com/hashicorp/setup-terraform/blob/main/wrapper/lib/output-listener.js#L28 so the constructor takes an output stream as an argument, like process.stdout, and when making the call this._buff.push(data);, also perform something like this.stream.write(data);, potentially with a this.stream.flush();.

Ie:

  get listener () {
    const listen = function listen (data) {
      this._buff.push(data);
      if (this.stream) {
        this.stream.write(data);
        this.stream.flush();
      }
    };
    return listen.bind(this);
  }

The effect

This will then ensure that every chunk of data gets sent on to the appropriate stream, while also buffering, similar to the tee command.

Testing

Unit tests could be sorted by perhaps using a mock, looking for the stream calls when applied. Something like expect(listener.contents).toHaveCalls(['foo', 'bar', 'baz']);.

@ChristianSauer
Copy link

This is super annoying. Especially if you want to diagnose why terraform is hung.

@ben-clarke
Copy link

This is super annoying. Especially if you want to diagnose why terraform is hung.

Agreed. I have downgraded to v2 now as that outputs in real-time. Was the only way I could diagnose what the issue was for our run.

@vmaerten
Copy link

vmaerten commented Mar 6, 2024

👍 It's very annoying

@ChristianSauer
Copy link

@ben-clarke I could actually set
with:
terraform_wrapper: false

and it outputs everything at once but that depends on your use case

@SunsetYe66
Copy link

+1
The new way of dealing STDOUT/STDERR does not synchronous bump things. It leaves Actions page just like hang and we would be thinking if the runner died

@justinmchase
Copy link

justinmchase commented Mar 11, 2024

This is such a major bug, I cannot tell you how many hours I've lost because of this. I had no idea there even was a wrapper being applied so its incredibly difficult to figure out where this issue was coming from.

Here is a little snippet of node code I am using that maybe you could borrow from. I don't think you need to use the action runner exec and then you can just inherit the stdout like normal and not have to worry about it printing the command.

const { spawn } = require('node:child_process')

const vars = process.env[`INPUT_VARS`] || ''
const workingDirectory = process.env[`INPUT_WORKING-DIRECTORY`] || ''
const args = [
  'apply',
  '-input=false',
  '-auto-approve',
  ...vars.split('\n').filter(v => v).map(v => ['--var', v]).flat()
]

console.log('terraform', ...args)
console.log(process.env)

const ctp = spawn('terraform', args, {
  stdio: 'inherit',
  env: process.env,
  cwd: workingDirectory,
})
ctp.ref()
ctp.on('exit', (code) => {
  process.exit(code)
})

const cancel = (signal) => {
  ctp.kill(signal)
}
process.on('SIGINT', cancel)
process.on('SIGTERM', cancel)

@justinmchase
Copy link

justinmchase commented Mar 11, 2024

By the way if you really want to process the output stream I would strongly recommend using a class that extends the Transform class.

Its been a while since I've done this in node but something like this:

export class Wrapper extends Transform {
  _transform(data, encoding, callback) {
    // do whatever you want with data then push it
    // push will pass it through to the next stream
    this.push(data);
    callback();
  }
}

@aleuziNC
Copy link

aleuziNC commented Apr 3, 2024

+1
Am I the only one encountering an issue with the output sequence of the terraform apply command, particularly when errors occur? Specifically, I've noticed that detailed error information appears right at the beginning of the output, rather than following the error message itself.

@justinmchase
Copy link

Am I the only one encountering an issue with the output sequence of the terraform apply command...

No check out #405

@ViacheslavKudinov
Copy link

Same situation. Will be great to get it fixed.

Copy link

github-actions bot commented Jun 8, 2024

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, 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 Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants