destroy/bootstrap: explicit pass disable-bootstrap.tfvars for libvirt
#900
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The old implementation depended on terraform auto picking up `*.auto.tfvars` in the current directory. And since we ran terraform as separate process we changes the CWD of that to the `tempDir`. With openshift#822 terraform in run as part of installer so var and state files need to be explicit to the `tempDir`
openshift-ci-robot
added
size/XS
|
fixes #878 $ ./bin/openshift-install --dir dev create cluster
WARNING Found override for ReleaseImage. Please be warned, this is not advised
INFO Consuming "Install Config" from target directory
INFO Creating cluster...
INFO Waiting up to 30m0s for the Kubernetes API...
INFO API v1.11.0+7967044 up
INFO Waiting up to 30m0s for the bootstrap-complete event...
INFO Destroying the bootstrap resources...
INFO Waiting up to 10m0s for the openshift-console route to be created...dig adahiya-0-api.tt.testing +noall +answer
; <<>> DiG 9.11.4-P1-RedHat-9.11.4-5.P1.fc28 <<>> adahiya-0-api.tt.testing +noall +answer
;; global options: +cmd
adahiya-0-api.tt.testing. 0 IN A 192.168.126.11/cc @rphillips |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, crawford 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:
Approvers can indicate their approval by writing |
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Feb 2, 2019
The previous implementation pulled from the install-config, but that missed downstream changes like AWS instance type defaults being applied in pkg/asset/machines. With this commit, we pull that information from the cluster-API types, since they're the last touch point for that data. Some remaining information still comes from the InstallConfig, but I've split it into explicit arguments to avoid confusion about where data is coming from when InstallConfig's machine pools, etc. overlap with clusterapi.Machine fields. This commit also splits the platform-specific Terraform variable generation into separate functions generating separate files. I've used *.auto.tfvars filenames because Terraform loads those automatically from the current directory [1]. But that only helps folks who are trying to run our generated Terraform by hand; as described in d19cad5 (destroy/bootstrap: explicit pass `disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the installer runs outside the Terraform directory and needs to pass this through to Terraform explicitly. [1]: https://www.terraform.io/docs/configuration/variables.html#variable-files
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Feb 2, 2019
The previous implementation pulled from the install-config, but that missed downstream changes like AWS instance type defaults being applied in pkg/asset/machines. With this commit, we pull that information from the cluster-API types, since they're the last touch point for that data. Some remaining information still comes from the InstallConfig, but I've split it into explicit arguments to avoid confusion about where data is coming from when InstallConfig's machine pools, etc. overlap with clusterapi.Machine fields. This commit also splits the platform-specific Terraform variable generation into separate functions generating separate files. I've used *.auto.tfvars filenames because Terraform loads those automatically from the current directory [1]. But that only helps folks who are trying to run our generated Terraform by hand; as described in d19cad5 (destroy/bootstrap: explicit pass `disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the installer runs outside the Terraform directory and needs to pass this through to Terraform explicitly. [1]: https://www.terraform.io/docs/configuration/variables.html#variable-files
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Feb 4, 2019
The previous implementation pulled from the install-config, but that missed downstream changes like AWS instance type defaults being applied in pkg/asset/machines. With this commit, we pull that information from the cluster-API types, since they're the last touch point for that data. Some remaining information still comes from the InstallConfig, but I've split it into explicit arguments to avoid confusion about where data is coming from when InstallConfig's machine pools, etc. overlap with clusterapi.Machine fields. This commit also splits the platform-specific Terraform variable generation into separate functions generating separate files. I've used *.auto.tfvars filenames because Terraform loads those automatically from the current directory [1]. But that only helps folks who are trying to run our generated Terraform by hand; as described in d19cad5 (destroy/bootstrap: explicit pass `disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the installer runs outside the Terraform directory and needs to pass this through to Terraform explicitly. I'd prefer if FileList could be an internal property (.files?), but we currently require public fields for reloading from disk during multiple-invocation creation [2]. [1]: https://www.terraform.io/docs/configuration/variables.html#variable-files [2]: openshift#792 (comment)
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Feb 4, 2019
The previous implementation pulled from the install-config, but that missed downstream changes like AWS instance type defaults being applied in pkg/asset/machines. With this commit, we pull that information from the cluster-API types, since they're the last touch point for that data. Some remaining information still comes from the InstallConfig, but I've split it into explicit arguments to avoid confusion about where data is coming from when InstallConfig's machine pools, etc. overlap with clusterapi.Machine fields. This commit also splits the platform-specific Terraform variable generation into separate functions generating separate files. I've used *.auto.tfvars filenames because Terraform loads those automatically from the current directory [1]. But that only helps folks who are trying to run our generated Terraform by hand; as described in d19cad5 (destroy/bootstrap: explicit pass `disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the installer runs outside the Terraform directory and needs to pass this through to Terraform explicitly. I'd prefer if FileList could be an internal property (.files?), but we currently require public fields for reloading from disk during multiple-invocation creation [2]. [1]: https://www.terraform.io/docs/configuration/variables.html#variable-files [2]: openshift#792 (comment)
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Feb 4, 2019
The previous implementation pulled from the install-config, but that missed downstream changes like AWS instance type defaults being applied in pkg/asset/machines. With this commit, we pull that information from the cluster-API types, since they're the last touch point for that data. Some remaining information still comes from the InstallConfig, but I've split it into explicit arguments to avoid confusion about where data is coming from when InstallConfig's machine pools, etc. overlap with clusterapi.Machine fields. This commit also splits the platform-specific Terraform variable generation into separate functions generating separate files. I've used *.auto.tfvars filenames because Terraform loads those automatically from the current directory [1]. But that only helps folks who are trying to run our generated Terraform by hand; as described in d19cad5 (destroy/bootstrap: explicit pass `disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the installer runs outside the Terraform directory and needs to pass this through to Terraform explicitly. I'd prefer if FileList could be an internal property (.files?), but we currently require public fields for reloading from disk during multiple-invocation creation [2]. [1]: https://www.terraform.io/docs/configuration/variables.html#variable-files [2]: openshift#792 (comment)
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Feb 4, 2019
The previous implementation pulled from the install-config, but that missed downstream changes like AWS instance type defaults being applied in pkg/asset/machines. With this commit, we pull that information from the cluster-API types, since they're the last touch point for that data. Some remaining information still comes from the InstallConfig, but I've split it into explicit arguments to avoid confusion about where data is coming from when InstallConfig's machine pools, etc. overlap with clusterapi.Machine fields. This commit also splits the platform-specific Terraform variable generation into separate functions generating separate files. I've used *.auto.tfvars filenames because Terraform loads those automatically from the current directory [1]. But that only helps folks who are trying to run our generated Terraform by hand; as described in d19cad5 (destroy/bootstrap: explicit pass `disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the installer runs outside the Terraform directory and needs to pass this through to Terraform explicitly. I'd prefer if FileList could be an internal property (.files?), but we currently require public fields for reloading from disk during multiple-invocation creation [2]. The AWS volume defaults are from data/data/aws/variables-aws.tf. We'll be able to drop the Terraform defaults soon [3]. [1]: https://www.terraform.io/docs/configuration/variables.html#variable-files [2]: openshift#792 (comment) [3]: openshift#1128
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Feb 5, 2019
The previous implementation pulled from the install-config, but that missed downstream changes like AWS instance type defaults being applied in pkg/asset/machines. With this commit, we pull that information from the cluster-API types, since they're the last touch point for that data. Some remaining information still comes from the InstallConfig, but I've split it into explicit arguments to avoid confusion about where data is coming from when InstallConfig's machine pools, etc. overlap with clusterapi.Machine fields. Unfortunately, Master.Machines is not loadable. This commit fixes the "Go defaults applied to Master.Machines do not affect Terraform" issue (which is good), but it will not fix the "I ran create manifests and edited openshift/99_openshift-cluster-api_master-machines.yaml" because that is loaded back in downstream of the Master asset. We'll address that in follow-up work. This commit also splits the platform-specific Terraform variable generation into separate functions generating separate files. I've used *.auto.tfvars filenames because Terraform loads those automatically from the current directory [1]. But that only helps folks who are trying to run our generated Terraform by hand; as described in d19cad5 (destroy/bootstrap: explicit pass `disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the installer runs outside the Terraform directory and needs to pass this through to Terraform explicitly. The code copying the platform-specific Terraform variables file down into the temporary directory for bootstrap deletion has an IsNotExist check. At the moment, all of our platforms except for 'none' generate a platform-specific Terraform variables file. But we're starting to move towards "treat platforms you don't recognize as no-ops" in most locations (e.g. [2]), so we have the check to make "I guess there wasn't a platform-specific Terraform variables file for this platform" non-fatal. I'd prefer if FileList could be an internal property (.files?), but we currently require public fields for reloading from disk during multiple-invocation creation [3]. [1]: https://www.terraform.io/docs/configuration/variables.html#variable-files [2]: https://github.com/openshift/installer/pull/1112/files#diff-6532666297c6115f7774f93ebab396c4R156 [3]: openshift#792 (comment)
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Feb 7, 2019
The previous implementation pulled from the install-config, but that missed downstream changes like AWS instance type defaults being applied in pkg/asset/machines. With this commit, we pull that information from the cluster-API types, since they're the last touch point for that data. Some remaining information still comes from the InstallConfig, but I've split it into explicit arguments to avoid confusion about where data is coming from when InstallConfig's machine pools, etc. overlap with clusterapi.Machine fields. Unfortunately, Master.Machines is not loadable. This commit fixes the "Go defaults applied to Master.Machines do not affect Terraform" issue (which is good), but it will not fix the "I ran create manifests and edited openshift/99_openshift-cluster-api_master-machines.yaml" because that is loaded back in downstream of the Master asset. We'll address that in follow-up work. This commit also splits the platform-specific Terraform variable generation into separate functions generating separate files. I've used *.auto.tfvars filenames because Terraform loads those automatically from the current directory [1]. But that only helps folks who are trying to run our generated Terraform by hand; as described in d19cad5 (destroy/bootstrap: explicit pass `disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the installer runs outside the Terraform directory and needs to pass this through to Terraform explicitly. The code copying the platform-specific Terraform variables file down into the temporary directory for bootstrap deletion has an IsNotExist check. At the moment, all of our platforms except for 'none' generate a platform-specific Terraform variables file. But we're starting to move towards "treat platforms you don't recognize as no-ops" in most locations (e.g. [2]), so we have the check to make "I guess there wasn't a platform-specific Terraform variables file for this platform" non-fatal. I'd prefer if FileList could be an internal property (.files?), but we currently require public fields for reloading from disk during multiple-invocation creation [3]. [1]: https://www.terraform.io/docs/configuration/variables.html#variable-files [2]: https://github.com/openshift/installer/pull/1112/files#diff-6532666297c6115f7774f93ebab396c4R156 [3]: openshift#792 (comment)
wking
added a commit
to wking/openshift-installer
that referenced
this pull request
Feb 7, 2019
The previous implementation pulled from the install-config, but that missed downstream changes like AWS instance type defaults being applied in pkg/asset/machines. With this commit, we pull that information from the cluster-API types, since they're the last touch point for that data. Some remaining information still comes from the InstallConfig, but I've split it into explicit arguments to avoid confusion about where data is coming from when InstallConfig's machine pools, etc. overlap with clusterapi.Machine fields. Unfortunately, Master.Machines is not loadable. This commit fixes the "Go defaults applied to Master.Machines do not affect Terraform" issue (which is good), but it will not fix the "I ran create manifests and edited openshift/99_openshift-cluster-api_master-machines.yaml" because that is loaded back in downstream of the Master asset. We'll address that in follow-up work. This commit also splits the platform-specific Terraform variable generation into separate functions generating separate files. I've used *.auto.tfvars filenames because Terraform loads those automatically from the current directory [1]. But that only helps folks who are trying to run our generated Terraform by hand; as described in d19cad5 (destroy/bootstrap: explicit pass `disable-bootstrap.tfvars` for libvirt, 2018-12-13, openshift#900), the installer runs outside the Terraform directory and needs to pass this through to Terraform explicitly. The code copying the platform-specific Terraform variables file down into the temporary directory for bootstrap deletion has an IsNotExist check. At the moment, all of our platforms except for 'none' generate a platform-specific Terraform variables file. But we're starting to move towards "treat platforms you don't recognize as no-ops" in most locations (e.g. [2]), so we have the check to make "I guess there wasn't a platform-specific Terraform variables file for this platform" non-fatal. I'd prefer if FileList could be an internal property (.files?), but we currently require public fields for reloading from disk during multiple-invocation creation [3]. [1]: https://www.terraform.io/docs/configuration/variables.html#variable-files [2]: https://github.com/openshift/installer/pull/1112/files#diff-6532666297c6115f7774f93ebab396c4R156 [3]: openshift#792 (comment)
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The old implementation depended on terraform auto picking up
*.auto.tfvarsin the current directory.And since we ran terraform as separate process we changes the CWD of that to the
tempDir.With #822 terraform in run as part of installer so var and state files need to be explicit to the
tempDirFixes #878