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

virtual_environment_datastores.go: Update remote command to get datasource path #49

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

mattburchett
Copy link

@mattburchett mattburchett commented Mar 19, 2022

During the setup of this Terraform module, I was trying to create a cloud-init template based on this example. I noticed that when trying to retrieve the datasource path for NFS mounts, it was failing with the following error:

proxmox_virtual_environment_file.ubuntu_cloud_config["lrhq-ubuntu-test01"]: Creating...
proxmox_virtual_environment_file.test: Creating...
╷
│ Error: Failed to determine the datastore path
│ 
│   with proxmox_virtual_environment_file.ubuntu_cloud_config["lrhq-ubuntu-test01"],
│   on files.tf line 1, in resource "proxmox_virtual_environment_file" "ubuntu_cloud_config":
│    1: resource "proxmox_virtual_environment_file" "ubuntu_cloud_config" {
│ 
╵
╷
│ Error: Failed to determine the datastore path
│ 
│   with proxmox_virtual_environment_file.test,
│   on files2.tf line 1, in resource "proxmox_virtual_environment_file" "test":
│    1: resource "proxmox_virtual_environment_file" "test" { 
│ 
╵

When running the original shell command in the code from my Proxmox host, it returns nothing:

# grep -Pzo ': storage-arn\s+path\s+[^\s]+' /etc/pve/storage.cfg | grep -Pzo '/[^\s]*' | tr -d '\000'
#

However, testing it with the 'local' datasource, it works as expected.

# grep -Pzo ': local\s+path\s+[^\s]+' /etc/pve/storage.cfg | grep -Pzo '/[^\s]*' | tr -d '\000'
/var/lib/vz
#

That's when we noticed that the regex was assuming that "path" would be available on the next line. We rewrote the command to be agnostic of which line "path" exists on.

Credit to @AndrewPaglusch for helping solve this issue with me.

…ource path

This commit fixes the shell command that is being executed to get the datasource
path which appears to be no longer functional.

The previous command assumed that "path" is going to be the next line after the
datasource name, leaving NFS mounts and other types of mount points non-functional.
@bpg
Copy link
Owner

bpg commented Mar 19, 2022

Thanks @mattburchett & @AndrewPaglusch for the fix! Could you please attach an example of a /etc/pve/storage.cfg section that the new code should parse?

@mattburchett
Copy link
Author

Hey @bpg, sorry for the delay. I'm attaching a file containing my /etc/pve/storage.cfg for testing.

storage.cfg.txt

@bpg
Copy link
Owner

bpg commented Mar 22, 2022

The original code doesn't have much of unit testing around that area, and adding them is not a trivial task :(
I've manually test the new code with the provided example and few other examples from my own environment, and everything seems to be working as expected.

Thanks @mattburchett !

@bpg bpg merged commit 065e859 into bpg:main Mar 22, 2022
@bpg bpg added the 🐛 bug Something isn't working label Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants