Fix openstack terraform group vars#13275
Conversation
Signed-off-by: Pushpak-23 <pushpakithule@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pushpak-23 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @pushpak-23. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves the reliability of generating Ansible group_vars output from the OpenStack Terraform configuration by ensuring the destination directory exists and by normalizing the group_vars_path to an absolute path.
Changes:
- Pre-create
group_vars_pathviamkdir -pbefore writingno_floating.ymlin severallocal-execprovisioners. - Quote template and output paths in shell commands to better handle paths with spaces.
- Pass
group_vars_pathto the compute module as an absolute path usingabspath().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| contrib/terraform/openstack/modules/compute/main.tf | Ensures the group vars directory exists and quotes paths before writing no_floating.yml from the bastion template. |
| contrib/terraform/openstack/kubespray.tf | Normalizes group_vars_path to an absolute path when passed into the compute module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| provisioner "local-exec" { | ||
| command = "sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${var.bastion_fips[0]}/ ${path.module}/ansible_bastion_template.txt > ${var.group_vars_path}/no_floating.yml" | ||
| command = "mkdir -p \"${var.group_vars_path}\" && sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${var.bastion_fips[0]}/ \"${path.module}/ansible_bastion_template.txt\" > \"${var.group_vars_path}/no_floating.yml\"" |
|
|
||
| provisioner "local-exec" { | ||
| command = "%{if each.value.floating_ip}sed s/USER/${var.ssh_user}/ ${path.module}/ansible_bastion_template.txt | sed s/BASTION_ADDRESS/${element(concat(var.bastion_fips, [for key, value in var.k8s_masters_fips : value.address]), 0)}/ > ${var.group_vars_path}/no_floating.yml%{else}true%{endif}" | ||
| command = "%{if each.value.floating_ip}mkdir -p \"${var.group_vars_path}\" && sed s/USER/${var.ssh_user}/ \"${path.module}/ansible_bastion_template.txt\" | sed s/BASTION_ADDRESS/${element(concat(var.bastion_fips, [for key, value in var.k8s_masters_fips : value.address]), 0)}/ > \"${var.group_vars_path}/no_floating.yml\"%{else}true%{endif}" |
|
|
||
| provisioner "local-exec" { | ||
| command = "sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${element(concat(var.bastion_fips, var.k8s_master_fips), 0)}/ ${path.module}/ansible_bastion_template.txt > ${var.group_vars_path}/no_floating.yml" | ||
| command = "mkdir -p \"${var.group_vars_path}\" && sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${element(concat(var.bastion_fips, var.k8s_master_fips), 0)}/ \"${path.module}/ansible_bastion_template.txt\" > \"${var.group_vars_path}/no_floating.yml\"" |
|
|
||
| provisioner "local-exec" { | ||
| command = "sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${element(concat(var.bastion_fips, var.k8s_node_fips), 0)}/ ${path.module}/ansible_bastion_template.txt > ${var.group_vars_path}/no_floating.yml" | ||
| command = "mkdir -p \"${var.group_vars_path}\" && sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${element(concat(var.bastion_fips, var.k8s_node_fips), 0)}/ \"${path.module}/ansible_bastion_template.txt\" > \"${var.group_vars_path}/no_floating.yml\"" |
|
|
||
| provisioner "local-exec" { | ||
| command = "%{if each.value.floating_ip}sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${element(concat(var.bastion_fips, [for key, value in var.k8s_nodes_fips : value.address]), 0)}/ ${path.module}/ansible_bastion_template.txt > ${var.group_vars_path}/no_floating.yml%{else}true%{endif}" | ||
| command = "%{if each.value.floating_ip}mkdir -p \"${var.group_vars_path}\" && sed -e s/USER/${var.ssh_user}/ -e s/BASTION_ADDRESS/${element(concat(var.bastion_fips, [for key, value in var.k8s_nodes_fips : value.address]), 0)}/ \"${path.module}/ansible_bastion_template.txt\" > \"${var.group_vars_path}/no_floating.yml\"%{else}true%{endif}" |
|
|
||
| provisioner "local-exec" { | ||
| command = "%{if each.value.floating_ip}sed s/USER/${var.ssh_user}/ ${path.module}/ansible_bastion_template.txt | sed s/BASTION_ADDRESS/${element(concat(var.bastion_fips, [for key, value in var.k8s_masters_fips : value.address]), 0)}/ > ${var.group_vars_path}/no_floating.yml%{else}true%{endif}" | ||
| command = "%{if each.value.floating_ip}mkdir -p \"${var.group_vars_path}\" && sed s/USER/${var.ssh_user}/ \"${path.module}/ansible_bastion_template.txt\" | sed s/BASTION_ADDRESS/${element(concat(var.bastion_fips, [for key, value in var.k8s_masters_fips : value.address]), 0)}/ > \"${var.group_vars_path}/no_floating.yml\"%{else}true%{endif}" |
|
/ok-to-test |
/kind bug
What this PR does / why we need it:
Fixes an OpenStack Terraform workflow issue where the
local-execprovisioner could fail while generatinggroup_vars/no_floating.yml.The failure occurs because the module assumed the target
group_varsdirectory existed relative to the module execution path. This breaks when Terraform is executed using-chdiror from automation environments where the working directory differs from the module path.This PR:
group_vars_pathmkdir -pWhich issue(s) this PR fixes:
Fixes #13201
Special notes for your reviewer:
Validated using:
terraform -chdir="contrib/terraform/openstack" validateand verified successful execution of:
Does this PR introduce a user-facing change?: