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: disk id enumeration overwrite #254

Closed
wants to merge 2 commits into from

Conversation

lsjostro
Copy link
Contributor

this fixes a regression where disk ids get overwritten when you for example have a ide0 and scsi0 disk device.

@lsjostro
Copy link
Contributor Author

was not that easy as the API will return the order randomly of the disks. so need to figure out a better solution.

@lsjostro
Copy link
Contributor Author

need to search for disks in a predictable order

@mleone87
Copy link
Collaborator

rewrite some logic of disks handling in API is critical, actually is broken with more than a couple of disks

@lsjostro
Copy link
Contributor Author

yes, I agree! but this fixes it at least and makes it predictable. but it might break for people upgrading that relay of a certain order. So what do use say? a big disclaimer somewhere? I think people will hit this mostly from the tf-provider?

@mleone87
Copy link
Collaborator

Fixing automatons will be up to user, priority is to fix it ;)

@lsjostro
Copy link
Contributor Author

some issues still with this, don't know if this code or something with the tf-provider? so need to think about it over the weekend I guess. Feel free to think about it as well. 😄

@mleone87
Copy link
Collaborator

My point of view on this topic is to build a strong code base in the api to handle all disks use cases and the fix the provider side with changes, also breaking changes, we will handle that.

On API side we are open to any change unless they are stable enough

@Tinyblargon
Copy link
Collaborator

sorry to interject, but I've actually been working on a reimplementation of the qemu disks in #187 I can look into making it mergeable this weekend or sometime next week.

@mleone87
Copy link
Collaborator

hey @Tinyblargon glad to hear about that, I'll be more than happy to test and debug the code

@lsjostro
Copy link
Contributor Author

That would be awesome @Tinyblargon!

@lsjostro
Copy link
Contributor Author

moving ide to last works (proxmox cloudinit uses ide). but adding additional ide disks/devices will fail tf-provider. But it works in a predictable way at least. 😄. But as long as one pick any other device type it will work as expected.

@mleone87
Copy link
Collaborator

hey @lsjostro still needed now with big merge?

@lsjostro
Copy link
Contributor Author

nope. :) I will close this one! cheers!

@lsjostro lsjostro closed this May 14, 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.

3 participants