[GH-ISSUE #137] Proxmox component-specific methods are sometimes not ideal

kerem commented

2026-03-03 15:29:42 +03:00

Owner

Originally created by @Starttoaster on GitHub (Mar 25, 2024).
Original GitHub issue: https://github.com/luthermonson/go-proxmox/issues/137

The title of this Issue is probably a little unclear what I mean. But as an example, take the function to request a node's version details: github.com/luthermonson/go-proxmox@0a31b09c4f/nodes.go (L24)

This API method requires me to make a request to Node(ctx, nodeName) and then make the request to get the VE node's version from the Node struct that it returned. So, I'm making 2 API requests to get that version data, when I could do this in 1 API request if the method took a node name string instead.

I see the design decision, but I don't think it's a good assumption that I'm always going to care about all the output from Node(...) when I want the data from Version(...). For example, maybe I just made a request to Nodes(ctx), got my list of nodes (and their names) and now just want to hit the Version method, or maybe I know my node names some other way (I did name them in the first place.)

The Node/Version API method issue is just one example, that design decision with that assumption is seen across many other API methods in this library.

I actually wrote my own library for a PVE prometheus metrics exporter product I work on partially because of this constraint, but it's maybe something to think about here, since your library has more users it services.

Originally created by @Starttoaster on GitHub (Mar 25, 2024). Original GitHub issue: https://github.com/luthermonson/go-proxmox/issues/137 The title of this Issue is probably a little unclear what I mean. But as an example, take the function to request a node's version details: https://github.com/luthermonson/go-proxmox/blob/0a31b09c4fdd6a42813053a7423a56a23c378b1d/nodes.go#L24 This API method requires me to make a request to `Node(ctx, nodeName)` and then make the request to get the VE node's version from the Node struct that it returned. So, I'm making 2 API requests to get that version data, when I could do this in 1 API request if the method took a node name string instead. I see the design decision, but I don't think it's a good assumption that I'm always going to care about all the output from `Node(...)` when I want the data from `Version(...)`. For example, maybe I just made a request to `Nodes(ctx)`, got my list of nodes (and their names) and now just want to hit the Version method, or maybe I know my node names some other way (I did name them in the first place.) The Node/Version API method issue is just one example, that design decision with that assumption is seen across many other API methods in this library. I actually wrote my own library for a PVE prometheus metrics exporter product I work on partially because of this constraint, but it's maybe something to think about here, since your library has more users it services.

kerem closed this issue

2026-03-03 15:29:43 +03:00

kerem commented

2026-03-03 15:29:43 +03:00

Author

Owner

@Starttoaster commented on GitHub (Mar 25, 2024):

I'm also kind of curious, do you test this library against specific Proxmox VE server versions? I've noticed some API backwards incompatibility issues with the PVE API between 7.x.x and 8.x.x. Curious how you maintain backwards compatibility or where you notate which versions of your library support which PVE versions.

@Starttoaster commented on GitHub (Mar 25, 2024): I'm also kind of curious, do you test this library against specific Proxmox VE server versions? I've noticed some API backwards incompatibility issues with the PVE API between 7.x.x and 8.x.x. Curious how you maintain backwards compatibility or where you notate which versions of your library support which PVE versions.

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@luthermonson commented on GitHub (Mar 26, 2024):

RE the compatibility, we tend to mostly move forward and people test with what they have in their home labs and i've noticed that most people keep their labs up to date when new versions drop. open to branching ideas or if anyone has hardware and nested virtualization to offer e2e tests on different versions. ill answer the other question in another comment

@luthermonson commented on GitHub (Mar 26, 2024): RE the compatibility, we tend to mostly move forward and people test with what they have in their home labs and i've noticed that most people keep their labs up to date when new versions drop. open to branching ideas or if anyone has hardware and nested virtualization to offer e2e tests on different versions. ill answer the other question in another comment

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@luthermonson commented on GitHub (Mar 26, 2024):

RE Version()... there are two... the cluster https://github.com/luthermonson/go-proxmox/blob/main/proxmox.go#L84-L86 which uses this endpoint: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/version and the node https://github.com/luthermonson/go-proxmox/blob/main/nodes.go#L24-L26 which uses this endopint: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/version

dont ask me what is different about them as i havent looked into it... i just know they exist and i think the node.Version func is probably where it should be for now.

I understand your concern in your example for version having an extra call to "initialize" (for lack of a better term) the Node but once the node is setup you can use it repeatedly like polling status etc. and that initial call to load the Node helps us error check the node exists which I think has benefit.

I think in your version scenario where you don't want to make the two calls you can make a helper function which calls client.Get yourself and bypass creating the Node if you really want too.

I also concede that the lister calls like Nodes() or VirtualMachines() or Containers() should probably return lists of the Type they claim to be listing and Nodes() returning NodeStatus is kinda dumb and perhaps if that was rewritten it would help solve your problem as well.

@luthermonson commented on GitHub (Mar 26, 2024): RE Version()... there are two... the cluster https://github.com/luthermonson/go-proxmox/blob/main/proxmox.go#L84-L86 which uses this endpoint: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/version and the node https://github.com/luthermonson/go-proxmox/blob/main/nodes.go#L24-L26 which uses this endopint: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/version dont ask me what is different about them as i havent looked into it... i just know they exist and i think the node.Version func is probably where it should be for now. I understand your concern in your example for version having an extra call to "initialize" (for lack of a better term) the Node but once the node is setup you can use it repeatedly like polling status etc. and that initial call to load the Node helps us error check the node exists which I think has benefit. I think in your version scenario where you don't want to make the two calls you can make a helper function which calls client.Get yourself and bypass creating the Node if you really want too. I also concede that the lister calls like Nodes() or VirtualMachines() or Containers() should probably return lists of the Type they claim to be listing and Nodes() returning NodeStatus is kinda dumb and perhaps if that was rewritten it would help solve your problem as well.

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@Starttoaster commented on GitHub (Mar 26, 2024):

and that initial call to load the Node helps us error check the node exists which I think has benefit.

We get the same error checking from just requesting the node's version. If the node doesn't exist, the version function should also return an error, which we should check regardless of whether we know the node actually exists or not.

I think in your version scenario where you don't want to make the two calls you can make a helper function which calls client.Get yourself and bypass creating the Node if you really want too.

That feels like getting a tiny bit of the benefit of using a client library, I suppose 😄

Anyway, this is your product. Seems like we disagree about the exact level of unnecessary entanglement it causes so I'll just continue on with my own library, so no worries there.

I also concede that the lister calls like Nodes() or VirtualMachines() or Containers() should probably return lists of the Type they claim to be listing and Nodes() returning NodeStatus is kinda dumb and perhaps if that was rewritten it would help solve your problem as well.

The API return for /nodes is actually a bit different than /nodes/{name}/status (other than the obvious difference of one being a slice) so I don't know that I'd call it dumb :)

we tend to mostly move forward and people test with what they have in their home labs

That makes sense too, thanks for the answers to everything. It sounds like the outcome of this Issue requires no action items. If you agree, feel free to close this Issue :)

@Starttoaster commented on GitHub (Mar 26, 2024): > and that initial call to load the Node helps us error check the node exists which I think has benefit. We get the same error checking from just requesting the node's version. If the node doesn't exist, the version function should also return an error, which we should check regardless of whether we know the node actually exists or not. > I think in your version scenario where you don't want to make the two calls you can make a helper function which calls client.Get yourself and bypass creating the Node if you really want too. That feels like getting a tiny bit of the benefit of using a client library, I suppose :smile: Anyway, this is your product. Seems like we disagree about the exact level of unnecessary entanglement it causes so I'll just continue on with my own library, so no worries there. > I also concede that the lister calls like Nodes() or VirtualMachines() or Containers() should probably return lists of the Type they claim to be listing and Nodes() returning NodeStatus is kinda dumb and perhaps if that was rewritten it would help solve your problem as well. The API return for `/nodes` is actually a bit different than `/nodes/{name}/status` (other than the obvious difference of one being a slice) so I don't know that I'd call it dumb :) > we tend to mostly move forward and people test with what they have in their home labs That makes sense too, thanks for the answers to everything. It sounds like the outcome of this Issue requires no action items. If you agree, feel free to close this Issue :)

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@Starttoaster commented on GitHub (Mar 26, 2024):

RE Version()... there are two... the cluster https://github.com/luthermonson/go-proxmox/blob/main/proxmox.go#L84-L86 which uses this endpoint: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/version and the node https://github.com/luthermonson/go-proxmox/blob/main/nodes.go#L24-L26 which uses this endopint

I glean from this that you decided on the pattern you did (of using methods tied to API object returns) to organize your different API methods for mostly this reason.

Fwiw, I think the API client service pattern offers the same separation/organization of your API methods, without tying the user down into making extraneous API requests https://github.com/starttoaster/go-proxmox?tab=readme-ov-file#usage

In my usage, client.Nodes.<Method> offers me the same method cataloging capabilities it seems like you wanted here, without planting unexported clients into the structs my API methods return.

Anyway, apologies if my opinions are unwanted here 😄 Throw out my opinion if it differs from your own, I just set up that documentation to kind of display how I'd have rather used your library, in case the method of organizing your API methods you landed on was the only way you had considered. In my opinion, API client libraries that make too many assumptions about how the user would use it end up causing unnecessary headaches, because I just want them to be syntax sugar around making HTTP requests and getting a struct response. I'd rather the library not try to assume what order I want to make API requests in, what API methods I need to hit first before I can hit others, how to handle unexpected conditions like a node not existing, or etc, for me.

@Starttoaster commented on GitHub (Mar 26, 2024): > RE Version()... there are two... the cluster https://github.com/luthermonson/go-proxmox/blob/main/proxmox.go#L84-L86 which uses this endpoint: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/version and the node https://github.com/luthermonson/go-proxmox/blob/main/nodes.go#L24-L26 which uses this endopint I glean from this that you decided on the pattern you did (of using methods tied to API object returns) to organize your different API methods for mostly this reason. Fwiw, I think the API client service pattern offers the same separation/organization of your API methods, without tying the user down into making extraneous API requests https://github.com/starttoaster/go-proxmox?tab=readme-ov-file#usage In my usage, `client.Nodes.<Method>` offers me the same method cataloging capabilities it seems like you wanted here, without planting unexported clients into the structs my API methods return. Anyway, apologies if my opinions are unwanted here :smile: Throw out my opinion if it differs from your own, I just set up that documentation to kind of display how I'd have rather used your library, in case the method of organizing your API methods you landed on was the only way you had considered. In my opinion, API client libraries that make too many assumptions about how the user would use it end up causing unnecessary headaches, because I just want them to be syntax sugar around making HTTP requests and getting a struct response. I'd rather the library not try to assume what order I want to make API requests in, what API methods I need to hit first before I can hit others, how to handle unexpected conditions like a node not existing, or etc, for me.

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@Starttoaster commented on GitHub (Apr 8, 2024):

Sorry if I overstepped my bounds here suggesting a whole kind of API backbone change. I actually think it's a cool client library pattern. I just think it makes more sense in, for an example, a situation where the first request to an API server responds with some kind of session or secondary authentication token of some kind, that needs to be passed along in the headers or something of the next request. I don't think it really makes sense for a string name input, but to each their own. Anyway, closing since there's nothing to do if you prefer the current pattern.

@Starttoaster commented on GitHub (Apr 8, 2024): Sorry if I overstepped my bounds here suggesting a whole kind of API backbone change. I actually think it's a cool client library pattern. I just think it makes more sense in, for an example, a situation where the first request to an API server responds with some kind of session or secondary authentication token of some kind, that needs to be passed along in the headers or something of the next request. I don't think it really makes sense for a string name input, but to each their own. Anyway, closing since there's nothing to do if you prefer the current pattern.

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@luthermonson commented on GitHub (Apr 8, 2024):

@Starttoaster no bounds overstepped in open source. no worries. I hope you weren't worrying about your last comments the last 2wks? Come join the slack channel and let's have a chat, I think you offer some insight and I'd love to chat with you in real time

@luthermonson commented on GitHub (Apr 8, 2024): @Starttoaster no bounds overstepped in open source. no worries. I hope you weren't worrying about your last comments the last 2wks? Come join the slack channel and let's have a chat, I think you offer some insight and I'd love to chat with you in real time

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@Starttoaster commented on GitHub (Apr 8, 2024):

Haha, I wouldn't say I was worried. I just figured a suggestion to change a common pattern in your library you've been maintaining for a few years might be seen as potentially audacious depending on who I'm talking to 😄 I'd be glad to join the server, I'll drop by tomorrow sometime :)

@Starttoaster commented on GitHub (Apr 8, 2024): Haha, I wouldn't say I was _worried_. I just figured a suggestion to change a common pattern in your library you've been maintaining for a few years might be seen as potentially audacious depending on who I'm talking to 😄 I'd be glad to join the server, I'll drop by tomorrow sometime :)

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@luthermonson commented on GitHub (Apr 8, 2024):

chat soon then!

@luthermonson commented on GitHub (Apr 8, 2024): chat soon then!

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@Starttoaster commented on GitHub (Apr 8, 2024):

@luthermonson I did try to join, and maybe it's pebkac as I don't actually join Slack workspaces all that often, but when I click the Slack link on the readme it seems to gate me for not having an account in that workspace, so I think it might need to be replaced with an invite link, which either have an expiration of some number of days, or a limit of 400 uses by different users. Unless I'm somehow doing the wrong thing 😂

@Starttoaster commented on GitHub (Apr 8, 2024): @luthermonson I did try to join, and maybe it's pebkac as I don't actually join Slack workspaces all that often, but when I click the Slack link on the readme it seems to gate me for not having an account in that workspace, so I think it might need to be replaced with an invite link, which either have an expiration of some number of days, or a limit of 400 uses by different users. Unless I'm somehow doing the wrong thing 😂

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@luthermonson commented on GitHub (Apr 8, 2024):

https://gophers.slack.com/ try creating a workspace account here

@luthermonson commented on GitHub (Apr 8, 2024): https://gophers.slack.com/ try creating a workspace account here

kerem commented

2026-03-03 15:29:44 +03:00

Author

Owner

@Starttoaster commented on GitHub (Apr 8, 2024):

That's where the link in the readme would take me, yeah. Signing into my account that I'm signed into in my client would lead me to this bad authorization page which led me to think it was potentially private/invite-only.

@Starttoaster commented on GitHub (Apr 8, 2024): ![image](https://github.com/luthermonson/go-proxmox/assets/13720491/3cba0a7c-53e0-41da-8a00-3d5c42a71455) That's where the link in the readme would take me, yeah. Signing into my account that I'm signed into in my client would lead me to this bad authorization page which led me to think it was potentially private/invite-only.

kerem commented

2026-03-03 15:29:45 +03:00

Author

Owner

@luthermonson commented on GitHub (Apr 8, 2024):

uhhhh that is weird. im like 99.9% sure it's public... i'm on kube slack if you want to reach out there too

@luthermonson commented on GitHub (Apr 8, 2024): uhhhh that is weird. im like 99.9% sure it's public... i'm on kube slack if you want to reach out there too

kerem referenced this issue

2026-03-03 15:30:08 +03:00

[PR #30] [MERGED] IsRunning and IsStopped added #87

Rows
Columns

[GH-ISSUE #137] Proxmox component-specific methods are sometimes not ideal #30