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

VirtualMachineCloneOptions{full} should be type bool #102

Open
b- opened this issue Nov 5, 2023 · 1 comment
Open

VirtualMachineCloneOptions{full} should be type bool #102

b- opened this issue Nov 5, 2023 · 1 comment

Comments

@b-
Copy link

b- commented Nov 5, 2023

Hi,

I'm not sure if there's something I'm overlooking — maybe the IntOrBool type in the same file is relevant here? But I think this should accept a bool

https://github.com/luthermonson/go-proxmox/blob/6c8706ed43a8313502d4fbe4d8bd720fe86c5011/types.go#L543C1

@jqueuniet
Copy link
Contributor

jqueuniet commented Nov 5, 2023

On one hand, I agree a bool would feel intuitive and the official API spec says it's a boolean. On the other hand, the API never uses actual JSON booleans as far as I known, it's always integers with either 1 or 0.

So, that means accepting booleans as user input would require a custom type with custom Marshal/Unmarshal methods. The IntOrBool type does already provide those as far as I can see, so why not use it. Also, this means a potential API breakage if PVE ever starts sending or requesting integers other than 0 or 1, which technically they could do without breaking the current API signatures (but rather unlikely as they do pretend it's booleans in the spec).

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

No branches or pull requests

2 participants