Locals #802

yorinasub17 · 2019-07-27T04:28:34Z

This implements the concept of locals in terragrunt config. This can be used to bind expressions to names for reusing within the config, so that you can DRY complex expressions within the config.

TODO:

Add more testing

jfunnell · 2019-07-28T22:04:50Z

Wow awesome, this is an excellent improvement if it works (might test it today..).

Like you said, I don't really see how this could have helped with issue #752 and that is fine.. I think the overall best solution to clean up some of the shortcomings of terragrunt is to implement the entire feature set in #759, which would probably require a hefty chunk of code/functionality to change.
In short, I think this is an acceptable stopgap for now. Just my 2c

brikis98

Nice addition!

README.md

brikis98 · 2019-07-29T13:55:39Z

README.md

+}
+```
+
+You can use any valid terragrunt expression in the `locals` configuration.


Prepare yourself for the "how do I reuse locals from another file?" question in 3... 2... 1... :-D

Perhaps tackle that head on and add a section on reusing/merging yaml files for now? And when we add get_input and get_output, we can replace it.

Ok I can add that in.

FWIW, I was thinking about proposing a similar construct, but called globals which can be merged with the parent in an include call. I think the explicit separating of locals from globals is important, especially if and when include becomes more complex to allow inclusion of multiple levels of terragrunt config.

Do you have any immediate plans to implement globals? We've just developed a need for something like that (having variables in our parent terragrunt.hcl that we can override from child terragrunt.hcls). While including a yaml file if it exists would work, it doesn't seem like the cleanest solution and we'd have to walk it back once there's a better way.

If you haven't started and you think it would be something that would be accepted, I don't mind creating a PR for it.

If you haven't started and you think it would be something that would be accepted, I don't mind creating a PR for it.

I don't think we have immediate plans for globals. I would suggest first creating an issue (with Feature Request in the title) describing:

Your use case. Why do you need something like globals?

Ideal solution. Is globals what you want, or do you want something like get_local, which can take another terragrunt.hcl config file as input, a key and return the value assigned to that local in the target file?

Indicate whether you want to open a PR, either for the ideal solution or if that is challenging, a temporary workaround solution that works, but is better than what is available now.

brikis98 · 2019-07-29T13:56:44Z

config/config.go

+// the config. For consistency, `locals` in the call to `decodeHcl` is always assumed to be nil.
+func decodeAsTerragruntLocals(file *hcl.File, filename string, terragruntOptions *options.TerragruntOptions) (*terragruntLocals, error) {
+	terragruntLocals := terragruntLocals{}
+	err := decodeHcl(file, filename, &terragruntLocals, terragruntOptions, nil, nil)


What happens if a local references another local?

Ah I didn't think about this possibility. I think currently it will panic or raise a syntax error, but it would be nice if it can be recursive. Let me do a quick spike to see how hard it is to implement this.

Co-Authored-By: Yevgeniy Brikman <[email protected]>

yorinasub17 · 2019-07-30T19:15:14Z

UPDATE:

I made a huge refactor of the implementation to convert the locals attribute to a block. This has the advantage of being able to reference other locals in the locals block, but at the expense of being unable to merge directly from yamldecode. I think this is an ok tradeoff considering that you can still import yamls as locals, only that it can't be merged at the top level (see the new docs I wrote).

Note that I am still optimizing the logs and error handling, so not quite ready to merge yet but I reached the point where it would be good to get some feedback on the new approach.

…util pkg

yorinasub17 · 2019-07-30T19:52:24Z

Ok @brikis98 this is ready for re-review given the new implementation.

brikis98

Nice!

brikis98 · 2019-07-30T23:20:13Z

README.md

+}
+```
+
+##### Including globally defined locals


Thx for adding 👍

brikis98 · 2019-07-30T23:20:35Z

config/config.go

@@ -45,6 +47,15 @@ type terragruntConfigFile struct {
 	PreventDestroy *bool                  `hcl:"prevent_destroy,attr"`
 	Skip           *bool                  `hcl:"skip,attr"`
 	IamRole        *string                `hcl:"iam_role,attr"`
+
+	// This should be ignored


Why ignored?

This needs to be skipped here because hcl can't decode it correctly when it self references locals. It is not necessary to decode it into this struct here because it is evaluated in a separate loop.

However, we don't want to completely omit the existence of it because we want a syntax error if we have blocks that terragrunt doesn't expect (e.g typos). Hence it is added here as an attribute but the struct won't attempt to parse anything in it.

Perhaps expand the comment a bit to explain this?

Already did: https://github.com/gruntwork-io/terragrunt/pull/802/files#diff-3baf47c64847a8fb8aaa8cc2e088513bR51

brikis98 · 2019-07-30T23:21:18Z

config/config.go

@@ -299,24 +318,37 @@ func ParseConfigString(configString string, terragruntOptions *options.Terragrun
 // an include block)
 func decodeAsTerragruntInclude(file *hcl.File, filename string, terragruntOptions *options.TerragruntOptions) (*terragruntInclude, error) {
 	terragruntInclude := terragruntInclude{}
-	err := decodeHcl(file, filename, &terragruntInclude, terragruntOptions, nil)
+	err := decodeHcl(file, filename, &terragruntInclude, terragruntOptions, nil, nil)


An include can't have its own locals?

Ah good point! Updated the evaluation order so it can be there.

brikis98 · 2019-07-30T23:22:40Z

config/locals.go

+	}
+	if localsBlock == nil {
+		// No locals block referenced in the file
+		terragruntOptions.Logger.Printf("Did not find any locals block: skipping evaluation.")


Do we need to log this?

Implemented a crude version of log levels. This is actually quite useful to debug issues with locals and I expect we will need it once people start using it.

brikis98 · 2019-07-30T23:22:57Z

config/locals.go

+		return nil, nil
+	}
+
+	terragruntOptions.Logger.Printf("Found locals block: evaluating the expressions.")


Same here... Terragrunt doesn't currently support log levels, so the log statements here could be quite noisy...

See above re:log levels.

brikis98 · 2019-07-30T23:24:23Z

config/locals.go

+	// further.
+	evaluatedLocals := map[string]cty.Value{}
+	evaluated := true
+	for len(locals) > 0 && evaluated {


Should we add a simple counter and ensure it stays under, say, 1000? Could be a nice safety valve in case of a bug in the code that leads to an infinite loop.

Actually, on that note, what if the code has:

locals { a = local.b b = local.a }

This will fail because neither can be evaluated in the first loop, so evaluated will be false.

Should we add a simple counter and ensure it stays under, say, 1000? Could be a nice safety valve in case of a bug in the code that leads to an infinite loop.

Implemented.

brikis98 · 2019-07-30T23:26:45Z

config/locals.go

+// - the updated map of evaluated locals after this attempt
+// - whether or not any locals were evaluated in this attempt
+// - any errors from the evaluation
+func attemptEvaluateLocals(


Yikes, this is quite complicated. I have some vague memory that the HCL parser has native support for partial evaluation specifically for this use case, but can't find it in the code base quickly...

I searched but the only partial evaluation I could find was at the top file level, and not at the individual block level where we have expressions.

brikis98 · 2019-07-30T23:28:06Z

config/locals_test.go

+  // Multiple references
+  x = 1
+  y = 2
+  z = local.x + local.y


Consider testing a few corner cases:

Reference to a local that isn't defined in a locals { ... } block

Reference to a local that isn't defined elsewhere in the code

Cyclical references

Several layers of references (a = local.b, b = local.c, c = local.d, etc)

Done in a107c89

…blocks - Expand comment on why locals evaluation is ignored at the file level - Implement a crude version of Debugf that can be used for debug logging - Add infinite loop protection

brikis98 · 2019-07-31T10:55:46Z

util/logger.go

+	if os.Getenv("TG_LOG") == "DEBUG" {
+		logger.Printf(fmtString, fmtArgs...)
+	}
+}


Ah, clever 👍

yorinasub17 · 2019-07-31T18:32:37Z

UPDATE a107c89 :

Added a bunch more test cases
Fixed a bug where we parsed multiple locals block, but silently ignored all but the first one defined. Now we throw an error if we detect multiple locals blocks.
Fixed a case where we weren't properly writing out diagnostics information.

With that, if the build passes, I think we are ready to merge + release!

brikis98 · 2019-07-31T22:32:24Z

LGTM!

yorinasub17 · 2019-08-01T07:01:46Z

Ok merging and releasing. Thanks for the review!

yorinasub17 requested review from autero1, brikis98 and eak12913 as code owners July 27, 2019 04:28

yorinasub17 mentioned this pull request Jul 27, 2019

Enhancement: Support local variables in terragrunt.hcl #776

Closed

brikis98 approved these changes Jul 29, 2019

View reviewed changes

yorinasub17 and others added 4 commits July 30, 2019 12:11

RDD

73e3c3e

Implement concept of locals

cb445c4

Update README.md

521c6ee

Co-Authored-By: Yevgeniy Brikman <[email protected]>

Improve locals evaluation to support references to other locals

12610aa

yorinasub17 force-pushed the yori-locals branch from 91cb700 to 12610aa Compare July 30, 2019 19:11

yorinasub17 added 3 commits July 30, 2019 12:26

Write out diagnostics and fix testS

d763e80

Fix another test failure

4661f90

Fix unused imports in hclfmt.go after refactor of moving function to …

9eceb15

…util pkg

yorinasub17 requested a review from brikis98 July 30, 2019 19:52

Actually return an error if not all locals could be evaluated

9a46cd6

brikis98 approved these changes Jul 30, 2019

View reviewed changes

- Reorder evaluation of locals so that they can be used with include …

3601cfe

…blocks - Expand comment on why locals evaluation is ignored at the file level - Implement a crude version of Debugf that can be used for debug logging - Add infinite loop protection

brikis98 approved these changes Jul 31, 2019

View reviewed changes

util/logger.go

if os.Getenv("TG_LOG") == "DEBUG" {

logger.Printf(fmtString, fmtArgs...)

}

}

Copy link

Member

brikis98 Jul 31, 2019

Choose a reason for hiding this comment

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

Ah, clever 👍

Add more test cases

a107c89

yorinasub17 merged commit e6c8cb3 into master Aug 1, 2019

yorinasub17 deleted the yori-locals branch August 1, 2019 07:01

apottere mentioned this pull request Aug 2, 2019

Feature Request: Shared and overridable variables (globals?) #814

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Locals #802

Locals #802

yorinasub17 commented Jul 27, 2019 •

edited

Loading

jfunnell commented Jul 28, 2019

brikis98 left a comment

brikis98 Jul 29, 2019

yorinasub17 Jul 29, 2019

apottere Aug 2, 2019

yorinasub17 Aug 2, 2019

brikis98 Jul 29, 2019

yorinasub17 Jul 29, 2019

yorinasub17 commented Jul 30, 2019

yorinasub17 commented Jul 30, 2019

brikis98 left a comment

brikis98 Jul 30, 2019

brikis98 Jul 30, 2019

yorinasub17 Jul 31, 2019

brikis98 Jul 31, 2019

yorinasub17 Jul 31, 2019

brikis98 Jul 30, 2019

yorinasub17 Jul 31, 2019

brikis98 Jul 30, 2019

yorinasub17 Jul 31, 2019

brikis98 Jul 30, 2019

yorinasub17 Jul 31, 2019

brikis98 Jul 30, 2019

yorinasub17 Jul 30, 2019

yorinasub17 Jul 31, 2019

brikis98 Jul 30, 2019

yorinasub17 Jul 30, 2019

brikis98 Jul 30, 2019

yorinasub17 Jul 31, 2019

brikis98 Jul 31, 2019

yorinasub17 commented Jul 31, 2019

brikis98 commented Jul 31, 2019

yorinasub17 commented Aug 1, 2019

Locals #802

Locals #802

Conversation

yorinasub17 commented Jul 27, 2019 • edited Loading

jfunnell commented Jul 28, 2019

brikis98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorinasub17 commented Jul 30, 2019

yorinasub17 commented Jul 30, 2019

brikis98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorinasub17 commented Jul 31, 2019

brikis98 commented Jul 31, 2019

yorinasub17 commented Aug 1, 2019

yorinasub17 commented Jul 27, 2019 •

edited

Loading