Skip to content

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

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
yifan-gu:run_terraform_in_temp
Sep 25, 2018
Merged

asset/cluster: Invoke terraform in a temp dir.#319
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
yifan-gu:run_terraform_in_temp

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?

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.

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.
@yifan-gu yifan-gu force-pushed the run_terraform_in_temp branch from 7f3ae0a to 408c066 Compare September 24, 2018 21:21
@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

Details 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