Output run-all plan errors #1604
Conversation
By using a [`io.MultiWriter`](https://golang.org/pkg/io/#MultiWriter) terraform errors and hook output is printed/output to stdErr. This makes debugging of a hook and terraform easier because errors are no longer swallowed. It also makes `terragrunt run-all plan` act the same in regards to output as `terragrunt plan`.
| @@ -61,7 +62,7 @@ func (stack *Stack) Run(terragruntOptions *options.TerragruntOptions) error { | |||
| // We capture the out stream for each module | |||
| errorStreams := make([]bytes.Buffer, len(stack.Modules)) | |||
| for n, module := range stack.Modules { | |||
| module.TerragruntOptions.ErrWriter = &errorStreams[n] | |||
| module.TerragruntOptions.ErrWriter = io.MultiWriter(module.TerragruntOptions.ErrWriter, &errorStreams[n]) | |||
There was a problem hiding this comment.
This change looks reasonable to me, but a quick sanity check: @yorinasub17 were we suppressing run-all plan output intentionally by any chance to keep the logs cleaner? Or was that unintentional?
There was a problem hiding this comment.
My guess is that it was done to keep the output cleaner because now there are duplicate error in the stdErr because the logger is also writing to it once the run-all plan is complete.
I'm not sure here so what the expectations are regards to logging, terraform & hook output (stdOut & stdErr) is because right now it seems a little bit of a omelet 🙂
Would adding an extra option to terraform to specify where terraform & hook output should be written be a better option?
There was a problem hiding this comment.
This was unintentional. I believe we merged the run-all changes before the logger updates, and something must have gone wrong in the merge of the two features.
There was a problem hiding this comment.
My guess is that it was done to keep the output cleaner because now there are duplicate error in the stdErr because the logger is also writing to it once the run-all plan is complete.
Wait, so the errors are in the logs?
Would adding an extra option to terraform to specify where terraform & hook output should be written be a better option?
I'd like to avoid extra flags/options if we can. Too many of those already.
There was a problem hiding this comment.
This was unintentional. I believe we merged the
run-allchanges before the logger updates, and something must have gone wrong in the merge of the two features.
Roger
There was a problem hiding this comment.
My guess is that it was done to keep the output cleaner because now there are duplicate error in the stdErr because the logger is also writing to it once the run-all plan is complete.
Wait, so the errors are in the logs?
Yes (see
terragrunt/configstack/stack.go
Line 91 in 37f7ae3
Would adding an extra option to terraform to specify where terraform & hook output should be written be a better option?
I'd like to avoid extra flags/options if we can. Too many of those already.
Makes sense.
So what is the plan forward? can we merge this or should I try some other fix? or would y'all prefer to do this?
There was a problem hiding this comment.
but this only happens at the end of the plan which can be very confusing for people checking logs in a CI.
So what does the log output look like? And what do you expect it to look like?
There was a problem hiding this comment.
Well I expected the output to be the same as running multiple terragrunt plan outputs (which this PR makes happen).
The summary of all errors was a bit of a surprise to me but it makes sense from a UX point of view then again because of this, this PR will now cause duplicate errors in the stdErr because the logger is using the same ErrWriter.
The more I'm thinking about this the more I see 2/3 flows of information.
- TerraGrunt User information (UI/UX) which is something like
Stack at:and a list of modules and maybe something like Executing Module etc. - Debug/Log information examples are https://github.com/gruntwork-io/terragrunt/search?q=debugf
- Third party output (aka Terraform & Hook)
Now for a single terragrunt plan everything looks great but the run-all plan is streaming output from terraform directly to stdOut and a buffer for stdErr which causes some confusion for people using run-al plan because some log entries are not directly written and terraform errors are not printed with the terraform output.
I think a first option would be to start using a single Logger for all run-all calls this would at least avoid the issue of logs like Executing hook: being shown as part of the plan summary (proposed in #1612).
The second thing would be to decide how to show third party (terraform/hook) output. Should it maybe be buffered so that it's written once Terraform is done running? Should the output only be written to stdOut?
The final question is what to do with the plan summary? Make it optional?
I'm honestly not sure what options are the best also because I don't have the backstory of why things where done they way they where.
By using a
io.MultiWriterterraform errors and hook output is printed/output to stdErr.This makes debugging of a hook and terraform easier because errors are no longer swallowed.
It also makes
terragrunt run-all planact the same in regards to output asterragrunt plan.In my case having the terraform output while running terragrunt is very helpful since we have more then 900+ terraform things being called in some cases.
Possibly related Issue: #198