-
-
Notifications
You must be signed in to change notification settings - Fork 971
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
Restrict Locals Evaluation with Include Dirs #1644
base: main
Are you sure you want to change the base?
Restrict Locals Evaluation with Include Dirs #1644
Conversation
Resync with Upstream
@yorinasub17 I think you may be best to review this one. |
Believe I have found a small issue with the code in this PR, will take a look a bit later and see if I can address it. Reverting this PR to draft/WIP status for now. |
This implementation makes sense to me, and would also provide a workaround for #1033 @celestialorb please |
@yorinasub17 This is ready for a formal review again. I was right about the small issue I mentioned previously in that this original implementation did not account for globs in the A small oddity I encountered is that the ordering of the file paths returned by |
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.
Implementation LGTM! Only had one nit about a stale comment. Once that is addressed, and if the build passes, we can merge this in!
// TODO: since the terragrunt include directories can be specified as globs, we need to expand | ||
// those globs before walking over them. |
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 TODO is now addressed in the for loop above right? If so, we should remove this.
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.
Ah yes, forgot to take that out. Good catch.
Ah looks like the build failed for
|
Seems this is stemming from this line:
which is running a validate all with strict include and no given modules, and is thus expecting no modules to be returned...however that test is also checking to ensure that Terragrunt does not throw any errors; which it will in this case now since it's not detecting any Terragrunt configurations in I think the best course of action here might be to change that error to warning since it should be expected to not find any Terragrunt configuration files with strict include on and no include directories specified. |
Hmm I think the error message is actually correct. I would want terragrunt to give me a non-zero exit code when it finds no terragrunt.hcl config that it should run on. Can you actually just remove that test condition? I think using strict-include without include-dir should be considered an error given the intention of strict-include, so we shouldn't be testing that condition. |
What about cases where a script or CI system might generate a Terragrunt command with strict include based off of a set of changed modules? That set might be empty and could generate and run a Terragrunt command with |
Personally, I think the CI system or script should handle the case where there are no directories and not let terragrunt figure it out. That's a really simple array count check in bash, or whatever language it's in. In fact, I would argue that terragrunt should really be erroring much earlier with a clear error message if As a user, I would want terragrunt to complain to me that |
You might be interested in taking a look at #1600 then. |
Ah you got me. I missed that, and if @brikis98 made that decision, then I'm fine going forward with this. I would still argue that this should be handled early in the pipeline though, as a special check in terragrunt/configstack/stack.go Line 114 in f99b3fc
terragrunt in a dir without any terragrunt.hcl file.
|
I am also interested to see this merged, is there a specific reason why it is stuck for over a year? |
Shortly after this my team ditched Terragrunt in favor of plain Terraform, and thus I lost motivation to follow through on this. |
@yorinasub17 - If one wanted to get this merged, at this point is it just a matter of resolving conflicts and making sure the existing tests pass? |
Please make sure that you don't tag anyone that isn't currently a CODEOWNER. I want to make sure we aren't bugging anyone that isn't currently responsible for maintaining the project. @mfulgo Please do resolve conflicts and make sure tests pass. After that, please evaluate whether this pull request is necessary, given that it was opened quite a while ago. I've moved the pull request to in-draft, if you can convince the author to do these things, please encourage them to mark the PR as ready for review afterwards too (or do all this in a new pull request you start, etc). |
This PR is an attempt to address #1640.
We have a Terragrunt project that has a rather large number of modules (close to the order of 100) and we've noticed that Terragrunt is very slow to start when using the
run-all
command. This was discovered to be due to the fact that Terragrunt is running over all modules in the working directory and evaluating their locals, which was causing a few modules to be initialized during this phase as some locals were/are referencing dependencies.It was discovered that this behavior is the same even when specifying
--terragrunt-strict-include
. It is my belief that Terragrunt shouldn't need to evaluate locals of any modules outside of those specified in the--terragrunt-include-dir
flags (and subsequent module dependencies).This PR attempts to resolve this by changing the implementation of
FindConfigFilesInPath
under theconfig
package, which originally walks over the entire working directory looking for Terragrunt modules. The implementation has been changed to only walk over the directories specified by--terragrunt-include-dir
flags. Two tests have been added to the test suite to test these changes.I have verified that all tests pass under the
config
package with these changes, and I have also tested the build with these changes on our large Terragrunt project and can see significant startup time improvement.