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.

Sign up for GitHub

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

Jump to bottom

Output run-all plan errors #1604

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

Output run-all plan errors #1604

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion configstack/stack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package configstack
import (
"bytes"
"fmt"
"io"
"sort"
"strings"

Expand Down Expand Up @@ -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])
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Roger

Copy link
Author

Choose a reason for hiding this comment

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

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

terragruntOptions.Logger.Infoln(output)
) but this only happens at the end of the plan which can be very confusing for people checking logs in a CI.

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@boekkooi-fresh boekkooi-fresh Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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.

}
defer stack.summarizePlanAllErrors(terragruntOptions, errorStreams)
}
Expand Down