[wip] vSphere - Use extra_config inplace of vApp; use terraform file for ign #2479
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jcpowermac The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jcpowermac
force-pushed
the
vsphere_extra_config
branch
2 times, most recently
from
d05199c
to
281fd17
Compare
NOTE: This will fail CI - Remove the use of the bootstrap url - Remove the use of vApp properties, replace with extra_config. Limitations with vApp properties max length of 64kb - In place of ignition files in variables.tf use terraform file to read files from `./ignition` directory
jcpowermac
force-pushed
the
vsphere_extra_config
branch
from
281fd17
to
25256a0
Compare
upi/vsphere/main.tf
Outdated
| @@ -29,7 +29,7 @@ module "bootstrap" { | |||
|
|
|||
| name = "bootstrap" | |||
| instance_count = "${var.bootstrap_complete ? 0 : 1}" | |||
| ignition_url = "${var.bootstrap_ignition_url}" | |||
| ignition = "${file("${path.module}/ignition/bootstrap.ign")}" | |||
There was a problem hiding this comment.
hmm, should we be using a variable for this? this makes the ignition in {root}/ignition/bootstrap.ign ??
who puts that file there?
There was a problem hiding this comment.
we should take a variable the points the file's location..
There was a problem hiding this comment.
What I was thinking was copying the ignition files to a certain location within the terraform directory path to reduce the quantity of variables that a user would need to modify.
Sure I could create a variable to hold the path. Then in the terraform.tfvars.example use ${path.module}/ignition/node_type.ign? Or do you not want to modify the directory structure within the ./upi/vsphere ?
There was a problem hiding this comment.
Perhaps everything can be done inside of the same (upi/vsphere) directory? (openshift-install create ignition-configs + terraform apply) You could have variables passed into main.yaml with default path ${path.module}/node_type.ign.
There was a problem hiding this comment.
personally, i would like users to provide the filepath, and fail if they don't.
Because the installer generates all the ign files in a directory and the terraform {root} might be in a separate one, it makes sense to ask users to give us the location.
I'm fine if we think the user can point to the directory of the install where all the assets are, but explicit paths are more sustainable.
|
I tested this today and it works great. LGTM |
Remove ignition directory Add tfvars for path to ignition files Add tfvars in examples
|
I need to retest this. Removed the restart [0] and added the new vars to the path of the ignition files. |
|
@jcpowermac: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@jcpowermac you still okay to get this work merged ? |
|
Closing due to this being open for a long time, Please feel free to reopen /close |
|
@abhinavdahiya: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
NOTE: This will fail CI
Limitations with vApp properties max length of 64kb
to read files from
./ignitiondirectory