New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding suport for -var-file and -target argument #201
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks! Could you add some sort of test for this? E.g., deploy one of the existing examples by creating and pass some required params via a var file?
Any tips on how to make it simple as possible? |
What do you think ? I didn't test yet, but Ill do as soon as I can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test looks perfect, thanks! Let me know once you've run it and confirmed it works, and this is ready to merge.
I'm struggling with running the tests :S For some reason go isn't aware of the change in the files..
|
That's weird. Did you set up your |
The working directory is TerraformDir, so full path isn't necessary
I tried on a different machine and it worked!. I think it's because I had a symbolic link to a different directory.. I guess go doesn't like that. Now the test pass, I need to add a test for -target now |
The local_file has both data so the output would have both of them too
Are all the tests added and passing now? |
Yes
…On Mon, 3 Dec 2018 at 16:58, Yevgeniy Brikman ***@***.***> wrote:
Are all the tests added and passing now?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#201 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB9y2Y-2_ANKAfiIUyM6HzudpH_pqGM2ks5u1TwOgaJpZM4Y5iqv>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is just about ready to merge. Just a couple last minor items. Thanks!
modules/terraform/format.go
Outdated
// FormatTerraformArgs will format multiple args with the arg name (e.g. arg-file a arg-file b) | ||
func FormatTerraformArgs(argName string, args []string) []string { | ||
argsList := []string{} | ||
for _, varFile := range args { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argValue
is probably a better name here than varFile
modules/terraform/format.go
Outdated
@@ -19,6 +23,15 @@ func FormatTerraformVarsAsArgs(vars map[string]interface{}) []string { | |||
return formatTerraformArgs(vars, "-var") | |||
} | |||
|
|||
// FormatTerraformArgs will format multiple args with the arg name (e.g. arg-file a arg-file b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more realistic example might help explain what this method does. E.g.,
FormatTerraformArgs("-var-file", []string{"foo.tfvars", "bar.tfvars"}) returns "-var-file foo.tfvars -var-file bar.tfvars"
test/terraform_basic_example_test.go
Outdated
Targets: []string{"local_file.example"}, | ||
|
||
// Use the var files | ||
VarFiles: []string{"varfile.tfvars"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, terraform_basic_example_test.go
should be, well, a basic example intended to provide simple example code for Terratest newbies. I think Targets
and VarFiles
and the corresponding checks may be a little too complex for someone just starting out. Perhaps move this example + corresponding test code to a different example?
@aclowkey What is the latest status with this. Have all comments been resolved? |
No. The only unresolved issue is that of having it in another example. I will comment about it |
Will this be merged soon by any chance? |
@aclowkey Did you add all tests/examples you were planning to? |
Yes
…On Sun, 17 Feb 2019 at 14:11, Yevgeniy Brikman ***@***.***> wrote:
@aclowkey <https://github.com/aclowkey> Did you add all tests/examples
you were planning to?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#201 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB9y2Q801HuS02DaKjj_uHkdLllf8P5wks5vOUbbgaJpZM4Y5iqv>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks! I'll merge this now and let the tests run. If they pass, I'll issue a new release.
#192 #191