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
adding env var for --terragrunt-include-external-dependencies #1548
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
cli/args.go
Outdated
@@ -103,7 +103,7 @@ func parseTerragruntOptionsFromArgs(terragruntVersion string, args []string, wri | |||
|
|||
ignoreExternalDependencies := parseBooleanArg(args, OPT_TERRAGRUNT_IGNORE_EXTERNAL_DEPENDENCIES, false) | |||
|
|||
includeExternalDependencies := parseBooleanArg(args, OPT_TERRAGRUNT_INCLUDE_EXTERNAL_DEPENDENCIES, false) | |||
includeExternalDependencies := parseBooleanArg(args, OPT_TERRAGRUNT_INCLUDE_EXTERNAL_DEPENDENCIES, func() bool { _, b := os.LookupEnv("TERRAGRUNT_INCLUDE_EXTERNAL_DEPENDENCIES"); return b }()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a self-calling function here?
configstack/stack.go
Outdated
@@ -81,12 +81,6 @@ func (stack *Stack) Run(terragruntOptions *options.TerragruntOptions) error { | |||
func (stack *Stack) summarizePlanAllErrors(terragruntOptions *options.TerragruntOptions, errorStreams []bytes.Buffer) { | |||
for i, errorStream := range errorStreams { | |||
output := errorStream.String() | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be part of this PR?
- [terragrunt-check](#terragrunt-check) | ||
- [terragrunt-hclfmt-file](#terragrunt-hclfmt-file) | ||
- [terragrunt-override-attr](#terragrunt-override-attr) | ||
- [CLI commands](#cli-commands) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be part of this PR?
@@ -512,7 +526,8 @@ directories. | |||
|
|||
**CLI Arg**: `--terragrunt-ignore-dependency-order` | |||
|
|||
When passed in, ignore the depedencies between modules when running `*-all` commands. | |||
**CLI Arg**: `--terragrunt-include-external-dependencies`</br> | |||
**Environment Variable**: `TERRAGRUNT_INCLUDE_EXTERNAL_DEPENDENCIES` (set to `false`)</br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this flag apply here?
@brikis98 hey, thank you for the review! I'm going to catch my fork up to current HEAD and address your issues. |
Revert "Revert "Fix empty outputs (gruntwork-io#1568)"" This reverts commit f286021.
@brikis98 sorry about the hassle on the strange commit for I think all your comments are addressed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, great, thanks! Code changes LGTM. I'll kick off tests now.
Tests passed! Merging now. |
Hey there!
We've been running into the same issue as #1418 and gruntwork-io/terratest/issues/699. This small patch provides the ability to use an environment var with the expected behavior.
I looked through the tests around this code and didn't see where the env var bits for cli flags were tested. I'm happy to add coverage for this or discuss any required changes.
Let me know what thoughts are around the change.
Edit: forgot to include test run output, gist link here - not sure if the full integration tests need to pass as well, in which case I'll need to read through what is required in AWS and plan accordingly.