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

asset/cluster: Invoke terraform in a temp dir. #319

Merged

Conversation

yifan-gu
Copy link
Contributor

In order to be able to use openshift-install to launch multiple
clusters we need to create a working dir for each invocation.

Here we create a tmp dir that has the terraform.tfvars file and
invoke the cluster within the dir.

The dir will be removed after the invocation is done.

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 24, 2018
@wking
Copy link
Member

wking commented Sep 24, 2018

Does this mean we won't have the Terraform state around for cluster deletion?

if err != nil {
return nil, fmt.Errorf("failed to create temp dir: %v", err)
}
defer os.RemoveAll(tmpDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to unconditionally remove the terraform state? We could gate this with an option to keep tempfiles if someone wishes to, so we can use it for destruction later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -65,11 +72,11 @@ func (c *Cluster) Generate(parents map[asset.Asset]*asset.State) (*asset.State,
}

// This runs the terraform in the terraform template directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is not true anymore with this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment also mention that the tfstate file will be persisted on the disk.

@yifan-gu
Copy link
Contributor Author

yifan-gu commented Sep 24, 2018

@wking No, the terraform.state will be persisted into the cluster dir too, but it's done by the asset store when it's resolving the dependency.
See #319 (comment)

In order to be able to use openshift-install to launch multiple
clusters we need to create a working dir for each invocation.

Here we create a tmp dir that has the terraform.tfvars file and
invoke the cluster within the dir.

The dir will be removed after the invocation is done.
@steveej
Copy link
Contributor

steveej commented Sep 24, 2018

/approve
/retest

(assuming the tests are a flake but haven't checked)

@yifan-gu
Copy link
Contributor Author

/retest

@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, steveeJ, yifan-gu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,steveeJ,yifan-gu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit d459ca5 into openshift:master Sep 25, 2018
@yifan-gu yifan-gu deleted the run_terraform_in_temp branch September 25, 2018 00:47
wking added a commit to wking/openshift-installer that referenced this pull request Oct 19, 2018
From [1]:

  If dir is the empty string, TempDir uses the default directory for
  temporary files (see os.TempDir).

so there's no point in us calling TempDir() directly.

The explicit call is from 408c066 (asset/cluster: Invoke terraform in
a temp dir, 2018-09-24, openshift#319).

[1]: https://golang.org/pkg/io/ioutil/#TempDir
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants