Skip to content
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

use 'terraform workspace' for terraform versions >= 0.10.x. Fixes #142 #172

Closed
wants to merge 7 commits into from

Conversation

anubhavmishra
Copy link
Collaborator

We want to use terraform workspace for terraform versions >= 0.10.x

@codecov-io
Copy link

codecov-io commented Oct 28, 2017

Codecov Report

Merging #172 into master will decrease coverage by 0.17%.
The diff coverage is 7.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage   48.42%   48.24%   -0.18%     
==========================================
  Files          29       29              
  Lines        1615     1621       +6     
==========================================
  Hits          782      782              
- Misses        806      812       +6     
  Partials       27       27
Impacted Files Coverage Δ
server/events/terraform/terraform_client.go 0% <0%> (ø) ⬆️
server/events/project_pre_execute.go 95.23% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96be4e9...250fd28. Read the comment docs.

outputs = append(outputs, output)
if err != nil {
return outputs, err
}

// terraform env or workspace selection based on version
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Golang comments should be full sentences with capitalization and periods. For here I'd say something like

// Terraform versions >0.8 and <0.10 use the env command.
// Versions >= 0.10 have env renamed to workspace.

// terraform env or workspace selection based on version
// we will use 'terraform env' for terraform versions < 0.10.x
// and we will use 'terraform workspace' for terraform versions >= 0.10.x
tfCmdStr := "env"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tfCmdStr could be named more explicitly so the reader can understand by the name what it does.

  • tf is implied by the fact that we're in the terraform_client struct so it doesn't tell us anything
  • Str is the type which can easily be figured out so it doesn't add anything either really unless you're trying to distinguish between another tfCmd that isn't a string

Can we call this workspaceCmd? I know that it won't make sense for tf < 0.10 but we could treat that as a special case. So I'd default to workspaceCmd := "workspace" and then do a check on the version for <0.10.0 and change it to env

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like workspaceCmd since it doesn't make sense for tf < 0.10. I will find a better name and push updates.

// run terraform env new and select
output, err = c.RunCommandWithVersion(log, path, []string{"env", "select", "-no-color", env}, version, env)
output, err = c.RunCommandWithVersion(log, path, []string{tfCmdStr, "select", "-no-color", env}, v, env)
outputs = append(outputs, output)
if err != nil {
// if terraform env select fails we will run terraform env new
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to update the comment here. I think we can just go with workspace everywhere

@@ -100,22 +100,31 @@ func (c *Client) RunCommandWithVersion(log *logging.SimpleLogger, path string, a
// RunInitAndEnv executes "terraform init" and "terraform env select" in path.
// env is the environment to select and extraInitArgs are additional arguments
// applied to the init command.
func (c *Client) RunInitAndEnv(log *logging.SimpleLogger, path string, env string, extraInitArgs []string, version *version.Version) ([]string, error) {
func (c *Client) RunInitAndEnv(log *logging.SimpleLogger, path string, env string, extraInitArgs []string, v *version.Version) ([]string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update the function name. Let's go with Workspace everywhere since 0.9 is the only version that called it an Env

@@ -100,22 +100,31 @@ func (c *Client) RunCommandWithVersion(log *logging.SimpleLogger, path string, a
// RunInitAndEnv executes "terraform init" and "terraform env select" in path.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update the comment

@lkysow
Copy link
Collaborator

lkysow commented Oct 29, 2017

Can we use this opportunity to test this class?

Copy link
Collaborator

@lkysow lkysow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. What about tests?

// 2. "terraform workspace or env select" - This selects the workspace or environment for the terraform project
// [optional] 3. "terraform workspace or env new" - This creates a new workspace or environment for the terraform project
// env is the environment supplied by the atlantis user that is used to
// select or create a new workspace or environment for terraform
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs . at end

tfCmdStr := "env"
constraints, _ := version.NewConstraint(">= 0.10.0")
// Terraform uses 'terraform env' command for versions > 0.8 and < 0.10.
// Versions >= 0.10 use 'terraform workspace'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need . at end

}

// run terraform env new and select
output, err = c.RunCommandWithVersion(log, path, []string{tfCmdStr, "select", "-no-color", env}, v, env)
// Run 'terraform workspace/env select'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is necessary because it's obvious from reading the line below what's happening. I try to reserve comments for explaining why code is doing what it is rather than what. So the comment above where we're saying why we're switching between env/workspace is a good one

output, err = c.RunCommandWithVersion(log, path, []string{tfCmdStr, "new", "-no-color", env}, v, env)
// If 'terraform workspace/env select' fails we will run 'terraform workspace new'
// to create a new environment. This is done for ease of use so that the atlantis
// user doesn't have to create a new workspace/env manually.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice comment

@anubhavmishra
Copy link
Collaborator Author

@lkysow I tend on pushing stuff as I work on it. Yep, tests are the next push hopefully.

@lkysow
Copy link
Collaborator

lkysow commented Dec 18, 2017

Same fix contained in #219

@lkysow lkysow closed this Dec 18, 2017
@lkysow lkysow deleted the use-workspace branch December 18, 2017 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants