gruntwork-io / boilerplate

A tool for generating files and folders ("boilerplate") from a set of templates
https://www.gruntwork.io
Mozilla Public License 2.0
160 stars 12 forks source link

Add two new template helpers to support relative paths between dependencies. #30

Closed josh-padnick closed 7 years ago

josh-padnick commented 7 years ago

While working with a client who had an unusual folder structure, I could set the output-property for any boilerplate dependencies to output my boilerplate file in the desired location, but computing relative paths to teraform_remote_state and for terragrunt dependencies wasn't possible. So this PR implements two new functions:

brikis98 commented 7 years ago

Before diving into the code, a thought on the helpers.

I suspect boilerplate will be far more powerful if we focus on generic helpers that can be combined in a myriad of ways. The helpers you propose sound like they are purpose-built for a couple specific use cases, which is handy, but if we follow this philosophy, we quickly go down a rabbit hole of implementing a general-purpose programming language in helper syntax.

For example, a more general purpose combo would be:

You could then combine these in a myriad of ways. Your original dependencyOutputFolderName helper would be something like:

{{ stripPrefix (boilerplate-config "dependencies" "vpc-mgmt" "output-folder") "some-prefix" }}

The dependencyOutputFolderRelPath would be equivalent to something like:

{{ relPath (boilerplate-config "dependencies" "foo" "output-folder") (dependencies "bar" "output-folder") }}
josh-padnick commented 7 years ago

Your suggestion is an excellent one! Let's see if I can pump this one out real quick...

josh-padnick commented 7 years ago

Ok, added the suggested functions. The template helper boilerplateConfig came out a little messy, but it seemed like a pure reflection strategy wasn't the right approach. Also, any thoughts on a simple way to implement stripSuffix? Or better yet, it's really just "removeThisString", so any good ideas for a function name for removeFromString STRING TO_REMOVE (maybe that's better).

I have to run now, but code is all done less some comments and docs. Thx for your input!

brikis98 commented 7 years ago

Also, any thoughts on a simple way to implement stripSuffix?

https://golang.org/pkg/strings/#TrimSuffix

Or better yet, it's really just "removeThisString", so any good ideas for a function name for removeFromString STRING TO_REMOVE (maybe that's better).

https://golang.org/pkg/strings/#Replace

brikis98 commented 7 years ago

The template helper boilerplateConfig came out a little messy, but it seemed like a pure reflection strategy wasn't the right approach.

Agreed that it's messy and that reflection isn't a better alternative. Here's an idea for how to make this better:

In theory, Go templating allows you to call functions and access struct fields. So if you just add the root config, as an object, to the variables map right at the start (probably somewhere in the GetVariables method) under some special key (e.g. BoilerplateConfig), then we can navigate that like any other variable:

{{ range $index, $variable := .BoilerplateConfig.Variables }}
  {{ if eq $variable.Name "foo" }}
    <h1>$variable.Default</h1>
  {{ end }}
{{ end }}

If Go templating supported some sort of lambda, it would be possible to add a select helper that makes the above simpler:

{{ select .BoilerplateConfig.Variables ($var => eq $var.Name "foo") }}<h1>.Default</h1>{{ end }}

But I don't think it lets you create that sort of lambda on the fly. Therefore, instead of duping the BoilerplateConfig object as-is into the variable map, you could turn the Variables and Dependencies lists into maps, where the keys are the names and the values are the original structs:

vars := map[string]Variable{}
for _, var := range config.Variables {
  vars[var.Name()] = var
}

deps := map[string]Dependency{}
for _, dep := range config.Dependencies {
  deps[dep.Name] = dep
}

Now if you put the vars and deps maps into the variable map under special keys, such as BoilerplateConfigVars and BoilerplateConfigDeps, you can do:

<h1>{{ .BoilerplateConfigVars.foo.Default }}</h1>
josh-padnick commented 7 years ago

Thanks for the great suggestion on BoilerplateConfigVars and BoilerplateConfigDeps. Implemented in 5502446!

josh-padnick commented 7 years ago

All comments responded to. I think this one's ready to ship. Since I can use it locally without issuing a release, I'll wait for a final review before I merge.

josh-padnick commented 7 years ago

This refactor did introduce one snag. Consider the following template code:

{{- range $depName, $stringToDelete := .AlbNamesToInclude }}

data "terraform_remote_state" "{{ $depName }}" {
  backend = "s3"
  config {
    region = "{{ $.TerraformStateS3BucketRegion }}"
    bucket = "{{ $.TerraformStateS3BucketName }}"
    key = "${var.aws_region}/{{ trimPrefix ($.BoilerplateConfigDeps.$depName.OutputFolder) ($stringToDelete) }}/terraform.tfstate"
  }
}
{{- end }}

The problem is that $.BoilerplateConfigDeps.$depName.OutputFolder causes a panic. I tried placing the value in quotes, and various parentheses values. I didn't see any way to handle this from some Googling.

So I implemented new helper functions, boilerplateConfigDeps and boilerplateConfigVars. The example now becomes:

{{- range $depName, $stringToDelete := .AlbNamesToInclude }}

data "terraform_remote_state" "{{ $depName }}" {
  backend = "s3"
  config {
    region = "{{ $.TerraformStateS3BucketRegion }}"
    bucket = "{{ $.TerraformStateS3BucketName }}"
    key = "${var.aws_region}/{{ trimPrefix (boilerplateConfigDeps $depName "OutputFolder") ($stringToDelete) }}/terraform.tfstate"
  }
}
{{- end }}

Everything works exactly as I need, but the boilerplate template syntax is admittedly a little clumsy. Let me know if you have any further suggestions here.

brikis98 commented 7 years ago

The problem is that $.BoilerplateConfigDeps.$depName.OutputFolder causes a panic.

Why? Is that because $depName is a variable? If so, does something like $.BoilerplateConfigDeps | $depName work?

josh-padnick commented 7 years ago

Why? Is that because $depName is a variable?

Exactly. Golang templates don't seem to support a way to dynamically give the field name inline. I think I could use the index builtin function, but it starts to get ugly.

If so, does something like $.BoilerplateConfigDeps | $depName work?

When I try $.BoilerplateConfigDeps | $depName I get the error ... at <$depName>: can't give argument to non-function $depName.

So unless you have any strong objections, I think I'm good with the current solution. Thoughts?

brikis98 commented 7 years ago

Oh yea, the index function is exactly what you need here, isn't it?

index $.BoilerplateConfigDeps $depName | .OutputFolder

I'd say that doesn't look too bad, and certainly prettier and more maintainable than reflection...

josh-padnick commented 7 years ago

When I try index $.BoilerplateConfigDeps $depName | .OutputFolder, I get this error:

... at <.OutputFolder>: can't evaluate field OutputFolder in type string

I tried a few other variations and couldn't get those to work either. For what it's worth, the boilerplateConfigDeps function does use reflection but the code is pretty manageable. It takes advantage of the global vars I added! Here's the current definition:

func boilerplateConfigDeps(options *config.BoilerplateOptions) func(string, string) string {
    return func(name string, property string) string {
        deps := options.Vars["BoilerplateConfigDeps"].(map[string]variables.Dependency)
        dep := deps[name]

        r := reflect.ValueOf(dep)
        f := reflect.Indirect(r).FieldByName(property)
        return f.String()
    }
}

Any concerns about just running with this? Seems like a pretty clean solution to me, and it nicely combines the benefits of the original function I had while still making everything pretty generic.

josh-padnick commented 7 years ago

@brikis98 I just encountered the need to know "assuming I am a Dependency, what is my output-folder value?". It came up in the context of a Terragrunt dependency. I needed something like this:

dependencies = {
  paths = ["{{ relPath .This.OutputFolder (boilerplateConfigDeps .VpcDependencyName "OutputFolder") }}"]
}

I implemented support for .This in 05968b6. Thoughts?

brikis98 commented 7 years ago

I implemented support for .This in 05968b6. Thoughts?

Seems handy! A couple thoughts:

  1. .This is a bit ambiguous. Knowing the boilerplate code, I would actually expect it to be aBoilerplateConfig struct, since that's what's actually getting processed, not a Dependency. Alternatively, it could also be a BoilerplateOptions, since that's what gets passed to ProcessTemplate.
  2. Since there are a number of possible things that could go into .This, it may be worth contemplating what really belongs there. You could have .This.Config to return the current BoilerplateConfig, .This.Options to return the current BoilerplateOptions, and .This.Dependency to return the current Dependency (if any). Each of these would update on each iteration through the ProcessTemplate calls.

That said, taking a step back, we seem to be going down a bit of a rabbit hole.

dependencies = {
  paths = ["{{ relPath .This.OutputFolder (boilerplateConfigDeps .VpcDependencyName "OutputFolder") }}"]
}

This is a pretty damn complicated use case. You're building a relative path between the current module's output folder and the output folder of another module that you happen to know is a dependency with some dynamically-provided name. Oof, that's a mouthful.

What's the goal here? Is there a simpler way to express this?

brikis98 commented 7 years ago

When I try index $.BoilerplateConfigDeps $depName | .OutputFolder, I get this error:

Try (index $.BoilerplateConfigDeps $depName).OutputFolder instead: https://play.golang.org/p/R2aVeKwsQc

josh-padnick commented 7 years ago

What's the goal here? Is there a simpler way to express this?

The goal is to support the dependencies section of the Terragrunt file for an arbitrary file structure (inspired by Twiage). Sounds simple enough, but required me to add the relPath helper, the ability to look up another module's outputFolder and then to know the current module's outputFolder.

... various comments on .This

From the perspective of a non-boilerplate author (i.e. me!) I'm constantly adding boilerplate.yml dependencies and so that's what I think of as "this". If I want to get the "rootConfig" type stuff, I believe there are already helpers for that (e.g. outputFolder) and of course now the new global vars I added.

Also, I've added a lot of code already that already uses .This (admittedly before receiving feedback from someone other than myself), so I'm a little weary of making changes at this point...

brikis98 commented 7 years ago

The goal is to support the dependencies section of the Terragrunt file for an arbitrary file structure (inspired by Twiage).

Maybe a dumb question, but couldn't you just hard-code the dependency path as a boilerplate variable? It might be simpler than all this path manipulation...

brikis98 commented 7 years ago

To make that last comment clearer, in .terragrunt, you'd have:

dependencies = ["{{ .PathToVpcModule }}"]

And in boilerplate.yml, for that customer, you'd have:

variables:
  name: PathToVpcModule
  default: "../../../modules/vpc"
josh-padnick commented 7 years ago

Maybe a dumb question, but couldn't you just hard-code the dependency path as a boilerplate variable?

Having worked with this on a custom file layout, I just don't consider that a tenable option. Consider these actual .terragrunt files that were auto-generated:

ECS Service

dependencies = {
  paths = [
    "../../../vpc",
    "../../../ecs-cluster",
    "../../alb/nodejs-sample",
    "../../../../_global/route53",
  "../../../../_global/sns-topics",
    "../../../kms-master-key",
  ]
}

ECS Cluster (Twiage wanted 1 ALB per ECS Service and 3 environments in a single VPC)

dependencies = {
  paths = [
    "../vpc",
    "../../mgmt/bastion-host",
    "../_demo/alb/api",
    "../_demo/alb/dashboard",
    "../_stage/alb/api",
    "../_stage/alb/dashboard",
    "../_uat/alb/api",
    "../_uat/alb/dashboard",
    "../../_global/sns-topics",
  ]
}

Managing those by hand seems really painful, whereas more a tiny amount of verbosity, not even that bad when it's one per line, it's all auto-generated.

brikis98 commented 7 years ago

Well, in the examples you're listing, you're still specifying each of those modules manually. The only thing you're not doing is specifying the relative path to them. That could be reduced to a small handful of variables (e.g. "{{ .EnvRoot }}/ecs-cluster"). That said, I agree generating automatically is preferable, so long as it doesn't take us crazy rabbit holes with complicated code to read, write, and maintain.

I'd still argue that .This must be the boilerplate template currently being processed. The root boilerplate.yml file, the one that mostly specifies dependencies, is definitely one such template, but not a Dependency. You even had to set a special case for it, setting it to an empty Dependency. Therefore, either go with a different name (e.g. .CurrentDependency) or make .This a struct with fields like .Config, .Options, .CurrentDependency. Doing a search & replace on your code to convert .This to one of these other options should be fairly easy in any IDE.

josh-padnick commented 7 years ago

That could be reduced to a small handful of variables (e.g. "{{ .EnvRoot }}/ecs-cluster").

The use case I was working on was to support arbitrary folder structures with customers, so the .EnvRoot var in your example, would be different for each customer we setup. For this reason, I think relPath (myOuputPath) (someOtherModuleOutputPath) is the right way to solve that problem.

I'd still argue that .This must be the boilerplate template currently being processed.

That does make sense. Ok, I'll change it one or the other of your suggestions.

josh-padnick commented 7 years ago

Ok, see what you think of 8cf2580. If that looks good, I think this ship is set to sail.

josh-padnick commented 7 years ago

Ok, I think this is finally ready to go. If any present can show just cause why this PR should not be properly merged, let them speak now or forever hold their peace.

josh-padnick commented 7 years ago

Merging.