Fix error output in plan-all.. #789
Conversation
| @@ -95,6 +95,7 @@ func downloadTerraformSourceIfNecessary(terraformSource *TerraformSource, terrag | |||
| // before and after hooks (if any). | |||
| terragruntOptionsForDownload := terragruntOptions.Clone(terragruntOptions.TerragruntConfigPath) | |||
| terragruntOptionsForDownload.TerraformCommand = CMD_INIT_FROM_MODULE | |||
| terragruntOptionsForDownload.Logger = terragruntOptions.Logger | |||
There was a problem hiding this comment.
Does Clone not do this automatically?
There was a problem hiding this comment.
the clone function sets up a new logger logging to the current ErrWriter.. which is not where the current logger is logging to..
I know this might not be the cleanest solution to just set it this way, but since it fixed it for me, i went for it.
maybe an actual go developer can come up with a cleaner implementation ;)
There was a problem hiding this comment.
btw it seems that ErrWriter and Logger are not really connected in a logical consequent way. since setting ErrWriter does not change the Logger in any way and also not sets up a new Logger.
There was a problem hiding this comment.
Hm, it seems like the right fix would be to make the Clone method do the right thing. This small workaround means (a) that Clone may still be doing the wrong thing in other places and (b) if we make updates to Clone in the future, that may break in this one-off fix here.
| } | ||
|
|
||
| if terragruntOptions.ErrWriter != os.Stderr { | ||
| errWriter = io.MultiWriter(terragruntOptions.ErrWriter, os.Stderr) |
There was a problem hiding this comment.
Can you explain the thinking behind these changes a bit more?
There was a problem hiding this comment.
this is basically taking care of giving a summary of all errors at the end of a plan-all run and printing the errors while running... the check is needed to not print logs twice while running if used in a normal plan (run without being wrapped in a plan-all)..
There was a problem hiding this comment.
This is the RunShellCommandWithOutput method, which is used in many places in the code base, and not just during plan-all commands. I'm a little concerned about piling up workarounds within RunShellCommandWithOutput that are intended for other methods, as it's not at all obvious why this would be here from just reading this part of the code. Is there some other way to set terragruntOptions params correctly so this method does the right thing without one-off hacks?
There was a problem hiding this comment.
agreed.. ;) i can try to fix it in another way..
There was a problem hiding this comment.
as far as i have seen ErrWriter is not used in a consequent manner and fixing it in a nicer way would require a lot of refactoring as there are already a lot of exceptions throughout the code..
this is basically a fix to the issue described in #397: where at the end of a plan-all run all errors are printed by basically just capturing everything sent to stderr and postponing the output to the end of the plan-all phase.
what also got wrongly captured was stderr from hooks: e.g. from running a |
|
How does your fix compare to #722? |
good question... basically this code adds fixes for already intended functionality without adding more complexity.. but feel free to close this one if the other PR also improves it.. i guess the outcome is very similar.. |
|
closed as this is outdated. |
tries to fix issues mentioned in #397:
when runnign
plan-allevery run states "Errors with plan".. which is due to capturing wrong output.This patch series fixes several issues and add minor improvements in output:
it's my first go code ;) so feel free to just take it as a hint how this was fixed for me.