✨feat: Enhance support for containers #126
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
==========================================
+ Coverage 25.95% 27.12% +1.17%
==========================================
Files 15 15
Lines 1753 1928 +175
==========================================
+ Hits 455 523 +68
- Misses 1278 1377 +99
- Partials 20 28 +8 ☔ View full report in Codecov by Sentry. |
|
I'm not sure if this PR satisfies #118. |
| @@ -52,24 +52,28 @@ func (c *Container) Start(ctx context.Context) (status string, err error) { | |||
| return status, c.client.Post(ctx, fmt.Sprintf("/nodes/%s/lxc/%d/status/start", c.Node, c.VMID), nil, &status) | |||
| } | |||
|
|
|||
| func (c *Container) Stop(ctx context.Context) (status *ContainerStatus, err error) { | |||
| func (c *Container) Stop(ctx context.Context) (status string, err error) { | |||
There was a problem hiding this comment.
oh did we really add this before the the data field was being auto-parsed... goodness that was a long time ago. great fix and probably can remove the ContainerStatus type too right? it's at this point useless
There was a problem hiding this comment.
Actually when I checked the API doc realized it turns the UPID as a string so I don't think we need something like containerStatus. IMO returning the UPID as it is might not be the correct way to do it so we can convert them to tasks. Here is an example of start but any status command returns the same object:
root@lowtower:~# pvesh create /nodes/lowtower/lxc/150/status/start UPID:lowtower:0035154F:068C3DB0:65E99E9D:vzstart:150:root@pam:
So what I'm offering is converting that one to
func (c *Container) Stop(ctx context.Context) (task *Task, err error) { var upid UPID if err := c.client.Post(ctx, fmt.Sprintf("/nodes/%s/lxc/%d/status/stop", c.Node, c.VMID), nil, &upid); err != nil { return nil, err } return NewTask(upid, c.client), nil }
There was a problem hiding this comment.
i agree to that change, let's do that. UPID and Tasks didnt exist when those funcs were initially wrote so let's go that route.
There was a problem hiding this comment.
Cool! I've created #129 so if you agree we can continue from there.
| return NewTask(upid, c.client), nil | ||
| } | ||
|
|
||
| func (c *Container) RRDData(ctx context.Context, timeframe Timeframe, consolidationFunction ConsolidationFunction) (rrddata []*RRDData, err error) { |
There was a problem hiding this comment.
any chance you learned anything writing the container RRDData func to help fix the virtualmachine func for this guy #112 ?
There was a problem hiding this comment.
Well, I didn't realize the issue before so I just followed the same method for Containers as we do on VirtualMachines. If you think that can cause issues we can remove or I can try to follow up on the issue and if I find anything I can come back to this one.
There was a problem hiding this comment.
i have no context, i thought it worked tbh and havent looked into it
|
@alperencelik this pretty much what most people need to get going with containers. Closes #118 imho If you think we can drop that type and/or know of a fix for the rrddata func for vms then feel free to drop another PR. going to get this merged and cut a release tonight as this is a ton of new funcs added that people may way to use |
|
Thank you so much for reviewing @luthermonson. I would like to hear your thoughts as well so for /lxc/status I'm offering to convert the strings to tasks since the /status returns a UPID but for /rrdData I guess I need some time to test it. |
This PR aims to enhance LXC support for Proxmox. Here is the documentation of LXC(Containers) API.
https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/lxc