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

Basic backup logic #133

Merged
merged 8 commits into from
Mar 17, 2024
Merged

Basic backup logic #133

merged 8 commits into from
Mar 17, 2024

Conversation

Pivnoy
Copy link
Contributor

@Pivnoy Pivnoy commented Mar 13, 2024

Vzdump API call for backups

func (n *Node) Vzdump(ctx context.Context, params *VirtualMachineBackupOptions) (task *Task, err error) {
var upid UPID

if params == nil {
Copy link
Owner

Choose a reason for hiding this comment

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

API docs read like all params are optional https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/vzdump

you sure you dont want to allow nil params? I haven't tested this but it reads like empty post data should still create a backup with some form of defaults. maybe confirm and let's drop the check?

types.go Outdated
Comment on lines 1247 to 1248
ModeType = string
CompressType = string
Copy link
Owner

Choose a reason for hiding this comment

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

these are already types and "mode" and "compress" are pretty generic and these are specific for the vzdump params. maybe better if you do VirtualMachineBackupMode and VirtualMachineBackupCompress

Mode ModeType `json:"mode,omitempty"`
Compress CompressType `json:"compress,omitempty"`
Notes string `json:"notes,omitempty"`
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not really one to talk here as I've definitely not finished param types before but if you could finish the params for this one I would appreciate it as it's an important feature and we should probably do it the first time. https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/vzdump note

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will finish it

@Pivnoy
Copy link
Contributor Author

Pivnoy commented Mar 15, 2024

Add all params for vzdump API call and change enum types.
Remove return error for nil check params.

@luthermonson
Copy link
Owner

This is great! Thanks for the contribution

Copy link

codecov bot commented Mar 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 26.96%. Comparing base (fdb95a1) to head (a904ff2).
Report is 14 commits behind head on main.

Files Patch % Lines
nodes.go 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #133      +/-   ##
==========================================
+ Coverage   25.95%   26.96%   +1.00%     
==========================================
  Files          15       15              
  Lines        1753     1962     +209     
==========================================
+ Hits          455      529      +74     
- Misses       1278     1399     +121     
- Partials       20       34      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@luthermonson luthermonson merged commit 893e647 into luthermonson:main Mar 17, 2024
2 of 3 checks passed
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