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

fix(vm): ignore ssd disk flag with virtio interface #231

Merged
merged 1 commit into from
Feb 7, 2023
Merged

Conversation

abdo-farag
Copy link
Contributor

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @abdo-farag! Thanks for the PR!

I have couple of comments, could you take a look pls?

Makefile Outdated
@@ -2,7 +2,8 @@ GOFMT_FILES?=$$(find . -name '*.go' | grep -v vendor)
NAME=terraform-provider-proxmox
TARGETS=darwin linux windows
TERRAFORM_PLUGIN_EXTENSION=
VERSION=0.12.0 # x-release-please-version
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bpg

This space makes some problems with me when building on Linux with zsh.
So when I run init with this space I get error.
terraform init
Error: Failed to query available provider packages

Could not retrieve the list of available versions for provider bpg/proxmox: provider registry.terraform.io/bpg/proxmox was not found in any of the search locations

It's wierd somehow. but anyway if it bother you I will remove it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, that's interesting. My guess is when you run make it sets VERSION exactly to 0.12.0 # x-release-please-version 🤦🏻
Good find, I'll take a look.

@@ -2488,7 +2488,9 @@ func resourceVirtualEnvironmentVMGetDiskDeviceObjects(
diskDevice.Size = &sizeString
diskDevice.SizeInt = &size
diskDevice.IOThread = &ioThread
diskDevice.SSD = &ssd
if strings.Contains(diskInterface, "scsi") {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the PVE docs:

Note that SSD emulation is not supported on VirtIO Block drives.

so I think this should be reflected in the condition then, i.e. if !strings.HasPrefix(diskInterface, 'virtio') {...}.

Also linter doesn't like cobbling if statements with unrelated assignments, so you may want to move this if block down a little bit, after the line 2494

@bpg bpg changed the title fix: ignore ssd disk flag with virtio interface fix(vm): ignore ssd disk flag with virtio interface Feb 6, 2023
@bpg bpg merged commit 1de9294 into bpg:main Feb 7, 2023
@ghost ghost mentioned this pull request Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants