feat: add load of export_as_locals #123
Conversation
Codecov Report
@@ Coverage Diff @@
## main #123 +/- ##
==========================================
+ Coverage 73.29% 73.41% +0.11%
==========================================
Files 27 29 +2
Lines 2760 2998 +238
==========================================
+ Hits 2023 2201 +178
- Misses 556 597 +41
- Partials 181 200 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
thanks for the feedback/review 💯 |
There was a problem hiding this comment.
LGTM but left comments regarding the naming of choice.
The block is called export_as_locals to be clear the symbols will be generated as terraform locals, but it doesn't mean types and functions must have exportAsLocals as this is just the user-facing block name.
We're adding the ability to generate TF locals from terramate variables (globals and metadata at this moment), so from very high level, the generate has the task of "load locals variable mapping" and generate TF locals. Having this in mind, I would call the file locals_decl.go or locals_mapping.go and the functions to load the locals definitions could be LoadLocalsMap() or LoadLocalsDecl() or something like that.
Just sharing that the whole naming feels strange, it's like we are mixing syntax with the semantics.
| // export_as_locals blocks. | ||
| // | ||
| // The rootdir MUST be an absolute path. | ||
| func LoadStackExportAsLocals(rootdir string, sm StackMetadata, g *Globals) (ExportAsLocals, error) { |
There was a problem hiding this comment.
Why are they called LoadStack* if they load from any kind of configuration?
There was a problem hiding this comment.
By having ExportAsLocals in the function names and types seems like the logic is doing this, when in fact this is just the name of the block...
I would call this just LoadStackLocals(), as it loads the locals values that are going to be generated later.
There was a problem hiding this comment.
Why are they called
LoadStack*if they load from any kind of configuration?
Loading exported locals definitions depends on metadata + globals, and both are defined only on the context of a stack, there is no way to load locals that don't belong to a stack, unless we were loading un-evaluated information. It is pretty much the same as globals/metadata, they can be used/defined anywhere on a project but evaluation is always on the context of a stack.
There was a problem hiding this comment.
By having
ExportAsLocalsin the function names and types seems like the logic is doing this, when in fact this is just the name of the block...I would call this just
LoadStackLocals(), as it loads the locals values that are going to be generated later.
Maybe LoadStackExportedLocals() ?
| return ExportAsLocals{}, fmt.Errorf("%q must be an absolute path", rootdir) | ||
| } | ||
|
|
||
| unEvalExport, err := loadStackExportAsLocals(rootdir, sm.Path) |
There was a problem hiding this comment.
unEvalExport is funny 😄
First, you have symbols or variables and after evaluation you get values. So maybe:
localVars, err := loadStackLocals(rootdir, sm.Path)
localValues, err := localVars.eval(sm, g)
as globals and metadata are kinds of variables.
There was a problem hiding this comment.
man I hated this uneval naming thing..but at the time I was completely stuck on not being able to think on better names, thanks 💯
| return unEvalExportAsLocals{}, err | ||
| } | ||
|
|
||
| exportLocals := newUnEvalExportAsLocals() |
There was a problem hiding this comment.
newUnEvalExportAsLocals() again newUnEval is strange and AsLocal looks like converting something. Of course the context of the file is export_as_locals but it doesn't mean we need to embed this everywhere.
There was a problem hiding this comment.
Man adding the "As" everywhere was the dumbest idea I ever had in my life..honestly..WTF. +1 on improving.
I hated the naming, was just not able to think on something better yet =P. +1 on improving.
hmm maybe LoadStackLocals, Im torn because I also wanted to make it clear the whole concept that locals use exported data from Terramate (that is the whole point), just talking about "loading locals" doesn't make that very clear IMHO, and the concept exists independent of syntax, exported as locals are a thing, they are exported from Terramate and imported on Terraform code on the stack.
Hmm yeah maybe, I think I was too literal, but the semantic concept of exporting Terramate data as locals exist, it is not just because of the syntax, we are literally exporting data from Terramate configs into stack Terraform code. At this stage we are not generating the final code yet, but indeed just loading, but we are loading "exported locals". Maybe the naming should be around "exported locals"... hmm thinking. I will merge this and address this on a PR ASAP, just to make sure I can keep building generate. Thanks for the review/feedback ❤️ |
Includes the core logic but no code generation, which will be done in a different PR. It is very similar to globals, but evaluation is simpler, so there is no attempt to extract duplication here yet, maybe in a future PR.
The spec: https://github.com/mineiros-io/terramate/blob/main/docs/locals-generation.md