create default tf vars file

[bdegeeter]

new feature for optional creation of a terraform.tfvars.json file from the vars block. This enables persistent of non-default or required vars with no default via a file parameter and output.

Add createVarFile: true to the terraform install block to enable.

[vdice]

Hi @bdegeeter, thanks for the PR. Before diving into a code review, I was hoping to check on a few things.

In familiarizing myself with the tfvars file, I created a minimal sample bundle employing the approach of using a pre-made tfvars file (existing in the same terraform directory with other *.tf files for the app). Here it is: https://www.terraform.io/docs/language/values/variables.html#variable-definitions-tfvars-files

I wonder if this might be the simplest approach, assuming it suits the needs mentioned here? It basically circumvents the need for parameters to be managed by Porter.

Alternatively, if the bundle author wants installers of the bundle to supply their own tfvars file at runtime, the parameter could just be a 'classic' parameter with no output source, like so:

parameters:
  - name: tfvars
    type: file
    path: /cnab/app/terraform/terraform.tfvars

Then, a parameter set for this bundle could be generated with the location to the user's tfvars file supplied:

 $ porter params show terraform-vars
Name: terraform-vars
Created: 10 minutes ago
Modified: 6 minutes ago

---------------------------------------------------------
  Name    Local Source                      Source Type
---------------------------------------------------------
  tfvars  /Users/vdice/scratch/test.tfvars  path

...and used for any bundle action, e.g.

 $ porter install -p terraform-vars
installing terraform-vars...
...
Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Outputs:

myvar = "testing 1, 2, 3"
execution completed successfully!

This latter use case would tie in nicely with @carolynvs 's proposal in getporter/porter#1672, specifically the persistent: true component, assuming I understand it correctly. The user's tfvars file would only need to be supplied via the parameter set on install -- for every other action for the lifetime of the installation, it needn't be supplied, as it would be persisted across actions.

Otherwise, if the bundle author does wish to have a bundle parameter for individual terraform vars, it seems like the persistent: true feature could work here as well -- the values for these vars, if needed (no default defined), could be supplied once on install and then be persisted across actions.

What do you think of those thoughts/approaches? Let me know if I may be missing (or misunderstanding) use case(s) here. Thanks!

[bdegeeter]

Here's how I've applied it to a working example. https://github.com/bdegeeter/porter-terraform-example/pull/3/files

It's still desirable for my case to use bundle parameters for every TF var. It provides a more uniform interface to bundles in general. I could see the same pattern apply to generation of ansible inventory vars. The bundle user doesn't need to care about the specific format for terraform, ansible or some other tool.

The primary goal I was going for is to have mixin vars block written as a tfvar file similar to tfstate so I did not have to maintain a duplicate vars block for upgrade and uninstall. I would also expect to be able to assign bundle credentials to a tf var in addition to a parameter if needed, but have not tested this. With persist: true for a parameter, I'd still need it in the vars block.

Maybe there's a different strategy to have a "global" vars block?

Having users or a service craft a file specific to a technology like terraform is not desirable over more "specific" types like strings, ints, numbers, and booleans.

[bdegeeter]

On a code review note (if the PR is desired). I made the change with go1.16 and did not see the build error. When I switch to go1.13 I can repro the issue. I can fix for 1.13 or update to use 1.16 depending on preference.

[vdice]

It's still desirable for my case to use bundle parameters for every TF var. It provides a more uniform interface to bundles in general.

Indeed, excellent point around expressing terraform vars as bundle parameters to the user -- terraform can just be an implementation detail, among others, that the bundle author has decided to use, and which perhaps needn't be obvious/explicit to the bundle user.

The one thing this brings up for me, which isn't an issue for this PR, is how the tfvars and tfstate params are still shown to the user when/if they generate a parameter set from bundles that use these. However, the pattern is that users of the bundle should not provide values for either. This is a good issue for us to file in Porter (maybe allowing bundle authors to 'hide' such parameters with output sources -- or at least parameters that aren't expected to be set by users installing the bundle)... anyway, for another time :)

Maybe there's a different strategy to have a "global" vars block?

That's an interesting idea. My first idea was perhaps adding a persistedVars or some such to the terraform mixin config (currently, clientVersion is the one supported config option)... maybe like:

mixins:
  - terraform:
      persistedVars:
        - myvar
        - username
        - ...

However, not sure this would be any better... it would also be a bit of a paradigm shift for a mixin config section -- classically it is used for build-time options as opposed to run-time.

Ok, thanks so much for supplying a link to your example bundle using this feature. I'm definitely getting a better picture of the functionality here. I'll start checking out the code!

[vdice]

I wonder if we should remove this log line? Perhaps to avoid printing sensitive parameter values? Or perhaps a compromise would be to conditionally print if debug is set to true?

[vdice]

nit: this could be written as m.FileSystem.Create(path.Join(m.WorkingDir, m.TerraformVarsFilename))

[vdice]

On a code review note (if the PR is desired). I made the change with go1.16 and did not see the build error. When I switch to go1.13 I can repro the issue. I can fix for 1.13 or update to use 1.16 depending on preference.

Oh, I forgot to mention. I believe the build failure was occurring on the main branch as well. A fix was applied in #66, so if you rebase this PR off of main, I bet we'll see it go green. Thanks!

[vdice]

This is looking great, thanks for collaborating! A few last items to look at...

[vdice]

Thank you @bdegeeter ! LGTM.