Extra arguments environment variables #150
Conversation
Codecov Report
@@ Coverage Diff @@
## master #150 +/- ##
==========================================
- Coverage 37.27% 31.64% -5.63%
==========================================
Files 16 17 +1
Lines 1261 1485 +224
==========================================
Hits 470 470
- Misses 773 997 +224
Partials 18 18
Continue to review full report at Codecov.
|
server/utils.go
Outdated
|
|
||
| // populateRuntimeEnvironmentVariables populates the terraform extra vars specified in the project config file | ||
| // with atlantis specific environment variables | ||
| func populateRuntimeEnvironmentVariables(extraArgs []string, workspaceDir string, tfEnv string, tfVersion *version.Version) []string { |
There was a problem hiding this comment.
This is the function that replaces the arguments for both plan_executor.go and apply_executor.go.
There was a problem hiding this comment.
We've been using object-oriented style functions throughout the codebase so I think we should stick with that for now. That means associating this method with an actual struct.
server/utils.go
Outdated
| @@ -0,0 +1,22 @@ | |||
| package server | |||
There was a problem hiding this comment.
Added utils.go to be used for functions like these.
There was a problem hiding this comment.
I think this doesn't belong in a server util. It is used for the execution so can we not put it in a command runner? Util classes should be reserved for the most generic of functions. Otherwise it just becomes a dumping ground. This function doesn't strike me as generic enough.
|
I have some feedback on the exact code changes but my first question is why we're doing string substitution instead of using the shell to do the interpretation itself. This seems a lot safer? cmd := exec.Command("bash", "-c", "echo bye $TEST")
env := os.Environ()
env = append(env, "TEST=hi")
cmd.Env = env
out, err := cmd.CombinedOutput()
fmt.Printf("out: %v, err: %v", string(out), err)If we do |
|
This requires you to read the OS environment variables and then pass that down with the exec func. That seemed hacky that is why I didn't want to do it. But sure, I can put that back. |
|
I don't like trying to emulate shell substitution ourselves. That's the
part that seems more hacky to me.
…On Sun, Sep 24, 2017, 12:01 AM Anubhav Mishra ***@***.***> wrote:
This requires you to read the OS environment variables and then pass that
down with the exec func. That seemed hacky that is why I didn't want to do
it. But sure, I can put that back.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#150 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA_IvXI7LAv7zffLzPBXpod8O8G_l90lks5slf4vgaJpZM4PhvUe>
.
|
…r variables subsitution
There was a problem hiding this comment.
That really does clean up the code. If you're good with it then I'm good. I was thinking that maybe it was overkill to run with bash -c. There are definitely some security problems with this now since you could close out the quote and then execute anything you wanted. We should add a security section to our README.
terraform/terraform_client.go
Outdated
| @@ -52,14 +53,31 @@ func (c *Client) Version() *version.Version { | |||
|
|
|||
| // RunCommandWithVersion executes the provided version of terraform with | |||
| // the provided args in path. | |||
There was a problem hiding this comment.
we should document what v and env do in the docs here
terraform/terraform_client.go
Outdated
| // append terraform executable name with args | ||
| tfCmd := fmt.Sprintf("%s %s", tfExecutable, strings.Join(args, " ")) | ||
|
|
||
| terraformCmd := exec.Command("bash", "-c", tfCmd) |
There was a problem hiding this comment.
I think sh....
|
Want to look at tests for this after? |
…ed docs about v and env after review
anubhavmishra
force-pushed
the
extra-arguments-environment-variables
branch
from
4c06e92
to
db31eda
Compare
${ENVIRONMENT},${ATLANTIS_TERRAFORM_VERSION}and${WORKSPACE}inextra_argumentssection inatlantis.yamlproject config file.