adding function to extract terraform vars from var file #733

alpacamybags118 · 2020-12-09T21:29:45Z

This PR adds the following functionality to the terraform Options module:

Exposes a function that allows you to get a map of all the variables contained in each terraform VarFile specified in a terraform options object. This would allow you to reference these easily later on in a test for comparison, etc.

This is my first time contributing to this repo, so I tried to follow the style guides and everything to the best of my ability. Any feedback would be greatly appreciated!

brikis98

Thanks for the PR!

A few thoughts:

What is the use case you have for this? I'd like to hear concrete details to make sure we're adding support for the right thing.
Following from the previous question, my guess is that it makes more sense to have a method to read a single value from a var file, rather than the whole var file. That way, you can parse it as a specific type (instead of interface{}), and use that value explicitly (whereas using the entire file seems unlikely). But again, this depends on the particular use case.
Please see our error handling guidelines.
Please make sure to run goimports.

alpacamybags118 · 2020-12-15T17:05:14Z

Thanks for the feedback @brikis98 !

As for the use case, I have found frequently that I've wanted to access a setting I've set in a tfvars file later in a test for use in comparison for assertion/validation. For example, I was changing a module that exposes an option to set a CNAME record. I was planning to use the value we set the CNAME record to for some validation. Since I couldn't access the value from the tfvars file, I had to end up taking it out and hard-code the value in the test and pass in both the var file and that individual var into the terraform options.

So, while this functionally isn't needed since you can get around cases where you do need to access certain variables, you would always need to hard-code them into your test and add additional code to import both a var and a var file in your terraform options. (unless I'm totally missing another way to do this).

Overall my argument is this functionality would allow you to not have to write additional code for use cases like this and keeps configuration more separated from the code. Hopefully what I'm saying makes sense 🤣

If you are OK with adding this, I'll go ahead and address the additional feedback you provided.

brikis98 · 2020-12-17T08:45:16Z

Thanks for the feedback @brikis98 !

As for the use case, I have found frequently that I've wanted to access a setting I've set in a tfvars file later in a test for use in comparison for assertion/validation. For example, I was changing a module that exposes an option to set a CNAME record. I was planning to use the value we set the CNAME record to for some validation. Since I couldn't access the value from the tfvars file, I had to end up taking it out and hard-code the value in the test and pass in both the var file and that individual var into the terraform options.

So, while this functionally isn't needed since you can get around cases where you do need to access certain variables, you would always need to hard-code them into your test and add additional code to import both a var and a var file in your terraform options. (unless I'm totally missing another way to do this).

Overall my argument is this functionality would allow you to not have to write additional code for use cases like this and keeps configuration more separated from the code. Hopefully what I'm saying makes sense 🤣

If you are OK with adding this, I'll go ahead and address the additional feedback you provided.

Makes sense to me!

alpacamybags118 · 2020-12-23T20:52:24Z

Crap, pushed too soon lol. Don't review yet - still need to update/add tests.

alpacamybags118 · 2021-01-11T03:50:26Z

@brikis98 happy (late) new year! I finally got a chance to come back to refactoring this PR a bit. I decided to structure it more like the output module and provide different functions for getting input file values as their different types (string, map, list), along with adding some new error types.

brikis98

Thanks! Left some comments/thoughts on the API.

modules/terraform/errors.go

brikis98 · 2021-01-18T17:55:58Z

modules/terraform/errors.go

+type InputFileKeyNotFound string
+
+func (err InputFileKeyNotFound) Error() string {
+	return fmt.Sprintf("tfvar file doesn't contain a value for the key %q", string(err))


NIT: Could the error message include the path to the tfvar file?

brikis98 · 2021-01-18T17:57:41Z

modules/terraform/options.go

+		return nil, err
+	}
+
+	_, exists := variables[key]


NIT: any reason not to store the returned value here instead of looking up variables[key] multiple times below?

Good catch, will update.

brikis98 · 2021-01-18T17:58:12Z

modules/terraform/options.go

+	}
+
+	if reflect.TypeOf(variables[key]).String() != "[]map[string]interface {}" {
+		return nil, UnexpectedOutputType{Key: key, ExpectedType: "map", ActualType: reflect.TypeOf(variables[key]).String()}


ExpectedType is misleading here, as you're actually looking for []map[string]interface {}...

Ah, good catch! Probably a bad copy-paste :P

brikis98 · 2021-01-18T17:58:33Z

modules/terraform/options.go

+		return nil, UnexpectedOutputType{Key: key, ExpectedType: "map", ActualType: reflect.TypeOf(variables[key]).String()}
+	}
+
+	mapKeys := variables[key].([]map[string]interface{})[0]


Check that the slice is not empty before looking up index 0 to avoid a potential panic.

brikis98 · 2021-01-18T18:00:19Z

modules/terraform/options.go

+	return fmt.Sprintf("%v", variable), nil
+}
+
+// GetVariableAsMapFromVarFile Gets the map represention of a variable from a provided input file found in VarFile


NIT: in both descriptions, it's probably worth calling out that this is specifically a map of strings. For maps with more complicated nested types, or other types in general, use GetAllVariablesFromVarFile.

brikis98 · 2021-01-18T18:00:46Z

modules/terraform/options.go

+	return resultMap, nil
+}
+
+// GetVariableAsListFromVarFile Gets the string list represention of a variable from a provided input file found in VarFile


Same note here that this only works for lists of strings and to use GetAllVariablesFromVarFile for more complicated types.

brikis98 · 2021-01-18T18:01:05Z

modules/terraform/options.go

+		return nil, err
+	}
+
+	if _, exists := variables[key]; !exists {


Same NIT here about storing the return value instead of looking it up again several times below.

brikis98 · 2021-01-18T18:02:52Z

modules/terraform/options.go

+}
+
+// GetAllVariablesFromVarFile Parses all data from a provided input file found in VarFile and returns them as a key-value map
+func GetAllVariablesFromVarFile(t *testing.T, option *Options, fileName string) map[string]interface{} {


A map[string]interface{} is a bit unwiedly and error prone to use, as you have to cast all the values within the map. What are your thoughts instead on an interface like:

func GetAllVariablesFromVarFile(t *testing.T, option *Options, fileName string, returnValue interface{})

Where returnValue is a pointer the user can pass in with any custom struct, and we will parse the tfvar file into that struct, including all the types within it. This is similar to OutputStruct.

That's valid. It would certainly make it easier for the user. I did see to minimize using pointers in the best practices for this repo and this is a bit unique compared to the other functions, but it probably makes sense given the complexity of the return type.

brikis98 · 2021-01-18T18:04:54Z

modules/terraform/options.go

+
+// GetAllVariablesFromVarFileE Parses all data from a provided input file found ind in VarFile and returns them as a key-value map
+// Retursn an error if the specified file does not exist, the specified file is not readable, or the specified file cannot be decoded from HCL
+func GetAllVariablesFromVarFileE(t *testing.T, option *Options, fileName string) (map[string]interface{}, error) {


Why take in a fileName and option struct, and have to search for the file within it, rather than take in solely the path directly to a var file? This applies to all the methods in this PR.

That makes sense. I think I wanted to use the options struct since I was originally thinking in just the scope of what is provided to options, but its just adding more unneeded logic. I'll adjust this.

alpacamybags118 · 2021-01-20T22:58:08Z

Thank you for the feedback! I'll get to making the updates later this week.

alpacamybags118 · 2021-01-29T08:01:54Z

@brikis98 I addressed all your feedback! I do have one thought, though, that I wanted to run by you. With these changes, it seems like maybe this code doesn't quite fit into the options file since it no longer interfaces with the options object. Maybe should be separated into its own module, like input? What are your thoughts?

brikis98 · 2021-02-01T14:51:34Z

@brikis98 I addressed all your feedback! I do have one thought, though, that I wanted to run by you. With these changes, it seems like maybe this code doesn't quite fit into the options file since it no longer interfaces with the options object. Maybe should be separated into its own module, like input? What are your thoughts?

Sorry for the delay. Agreed it doesn't quite fit into options.go. Perhaps var-files.go or similar instead?

alpacamybags118 · 2021-02-03T02:58:29Z

Updated to move the code to a different file!

brikis98 · 2021-02-08T11:56:39Z

Great, thanks! Would you mind rebasing on master? I can then kick off tests.

… functionality

Co-authored-by: Yevgeniy Brikman <[email protected]>

alpacamybags118 · 2021-02-09T00:41:13Z

Done! @brikis98

go.mod

brikis98

Apologies, found two more NITs, and then we're good!

brikis98 · 2021-02-10T10:40:14Z

modules/terraform/var-file_test.go

+	if err == nil {
+		t.FailNow()
+	}


NIT: use require.Error instead.

modules/terraform/var-file_test.go

Co-authored-by: Yevgeniy Brikman <[email protected]>

alpacamybags118 · 2021-02-12T01:54:27Z

Updated!

brikis98

LGTM! I'll kick off tests now.

alpacamybags118 · 2021-02-15T16:17:50Z

Hm, looks like a test not related to my code changes failed TestTerraformAwsRdsExample - test . It does look like my tests passed though:

=== RUN   TestGetVariablesFromVarFilesAsString
--- PASS: TestGetVariablesFromVarFilesAsString (0.00s)
=== RUN   TestGetVariablesFromVarFilesAsStringKeyDoesNotExist
--- PASS: TestGetVariablesFromVarFilesAsStringKeyDoesNotExist (0.00s)
=== RUN   TestGetVariableAsMapFromVarFile
--- PASS: TestGetVariableAsMapFromVarFile (0.00s)
=== RUN   TestGetVariableAsMapFromVarFileNotMap
--- PASS: TestGetVariableAsMapFromVarFileNotMap (0.00s)
=== RUN   TestGetVariableAsMapFromVarFileKeyDoesNotExist
--- PASS: TestGetVariableAsMapFromVarFileKeyDoesNotExist (0.00s)
=== RUN   TestGetVariableAsListFromVarFile
--- PASS: TestGetVariableAsListFromVarFile (0.00s)
=== RUN   TestGetVariableAsListNotList
--- PASS: TestGetVariableAsListNotList (0.00s)
=== RUN   TestGetVariableAsListKeyDoesNotExist
--- PASS: TestGetVariableAsListKeyDoesNotExist (0.00s)
=== RUN   TestGetAllVariablesFromVarFileEFileDoesNotExist
--- PASS: TestGetAllVariablesFromVarFileEFileDoesNotExist (0.00s)
=== RUN   TestGetAllVariablesFromVarFileBadFile
--- PASS: TestGetAllVariablesFromVarFileBadFile (0.00s)
=== RUN   TestGetAllVariablesFromVarFile
--- PASS: TestGetAllVariablesFromVarFile (0.00s)

alpacamybags118 · 2021-02-15T16:21:10Z

Ah, looks like it might be using an instance that isn't valid? 2021-02-15T15:38:55Z logger.go:66: Error: Error creating DB Instance: InvalidParameterValue: Invalid DB Instance class: db.t2.micro . I thought that was a valid one? https://aws.amazon.com/rds/instance-types/

brikis98 · 2021-02-15T16:44:03Z

Yup, it's an unrelated intermittent issue in some AWS regions: #783. All other tests passed, so I'm going to merge, and we'll deal with that issue later. Thanks!

brikis98 · 2021-02-15T16:45:41Z

https://github.com/gruntwork-io/terratest/releases/tag/v0.32.6

WafflesMcDuff · 2021-02-15T18:18:25Z

#784

alpacamybags118 requested review from brikis98 and yorinasub17 as code owners December 9, 2020 21:29

brikis98 reviewed Dec 14, 2020

View reviewed changes

alpacamybags118 force-pushed the varfile_extract branch from bdf8ed1 to 258eb4e Compare January 11, 2021 03:48

brikis98 reviewed Jan 18, 2021

View reviewed changes

Kyle Robertson and others added 6 commits February 8, 2021 19:39

adding function to extract terraform vars from var file

a59540c

refactored extracting vars from var file to work more like the output…

0207e62

… functionality

adding testing and comments

aa128a1

Update modules/terraform/errors.go

b156172

Co-authored-by: Yevgeniy Brikman <[email protected]>

addressed feedback

cbe3ee7

moving clibrary to new file

d1985c0

alpacamybags118 force-pushed the varfile_extract branch from 3746aad to d1985c0 Compare February 9, 2021 00:40

brikis98 reviewed Feb 9, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

update go.mod

72180fe

brikis98 reviewed Feb 10, 2021

View reviewed changes

alpacamybags118 and others added 2 commits February 11, 2021 20:49

Update modules/terraform/var-file_test.go

b5d2235

Co-authored-by: Yevgeniy Brikman <[email protected]>

addressing test feedback

8192f26

brikis98 approved these changes Feb 15, 2021

View reviewed changes

brikis98 merged commit 88ba3f0 into gruntwork-io:master Feb 15, 2021

WafflesMcDuff mentioned this pull request Feb 15, 2021

modules/terraform/var-file.go:142:8: undefined: hcl.Decode #784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding function to extract terraform vars from var file #733

adding function to extract terraform vars from var file #733

alpacamybags118 commented Dec 9, 2020 •

edited

Loading

brikis98 left a comment

alpacamybags118 commented Dec 15, 2020

brikis98 commented Dec 17, 2020

alpacamybags118 commented Dec 23, 2020

alpacamybags118 commented Jan 11, 2021

brikis98 left a comment

brikis98 Jan 18, 2021

brikis98 Jan 18, 2021

alpacamybags118 Jan 20, 2021

brikis98 Jan 18, 2021

alpacamybags118 Jan 20, 2021

brikis98 Jan 18, 2021

brikis98 Jan 18, 2021

brikis98 Jan 18, 2021

brikis98 Jan 18, 2021

brikis98 Jan 18, 2021

alpacamybags118 Jan 20, 2021

brikis98 Jan 18, 2021

alpacamybags118 Jan 20, 2021

alpacamybags118 commented Jan 20, 2021

alpacamybags118 commented Jan 29, 2021

brikis98 commented Feb 1, 2021

alpacamybags118 commented Feb 3, 2021

brikis98 commented Feb 8, 2021

alpacamybags118 commented Feb 9, 2021

brikis98 left a comment

brikis98 Feb 10, 2021

alpacamybags118 commented Feb 12, 2021

brikis98 left a comment

alpacamybags118 commented Feb 15, 2021

alpacamybags118 commented Feb 15, 2021

brikis98 commented Feb 15, 2021

brikis98 commented Feb 15, 2021

WafflesMcDuff commented Feb 15, 2021

adding function to extract terraform vars from var file #733

adding function to extract terraform vars from var file #733

Conversation

alpacamybags118 commented Dec 9, 2020 • edited Loading

brikis98 left a comment

Choose a reason for hiding this comment

alpacamybags118 commented Dec 15, 2020

brikis98 commented Dec 17, 2020

alpacamybags118 commented Dec 23, 2020

alpacamybags118 commented Jan 11, 2021

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

alpacamybags118 commented Jan 20, 2021

alpacamybags118 commented Jan 29, 2021

brikis98 commented Feb 1, 2021

alpacamybags118 commented Feb 3, 2021

brikis98 commented Feb 8, 2021

alpacamybags118 commented Feb 9, 2021

brikis98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpacamybags118 commented Feb 12, 2021

brikis98 left a comment

Choose a reason for hiding this comment

alpacamybags118 commented Feb 15, 2021

alpacamybags118 commented Feb 15, 2021

brikis98 commented Feb 15, 2021

brikis98 commented Feb 15, 2021

WafflesMcDuff commented Feb 15, 2021

alpacamybags118 commented Dec 9, 2020 •

edited

Loading