-
Notifications
You must be signed in to change notification settings - Fork 265
Feat: HaResource delete #464
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds functionality to delete High Availability (HA) resources and removes unused rawDigest fields from HA rule structures. The implementation includes proper error handling for cases where the HA resource doesn't exist.
- Adds
DeleteHaResourcemethod forGuestIDwith comprehensive error handling - Removes unused
rawDigestfield fromHaNodeAffinityRuleandHaResourceAffinityRulestructs - Updates corresponding test cases to reflect the removal of
rawDigest
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| proxmox/error.go | Adds new error types for HA resource deletion scenarios |
| proxmox/config__guest.go | Implements the main HA resource deletion functionality |
| proxmox/client__new.go | Adds interface method for HA resource deletion |
| proxmox/client__new__mock.go | Implements mock for HA resource deletion |
| proxmox/client__api.go | Adds low-level API interface and implementation |
| proxmox/client__api__mock.go | Adds mock implementation for API interface |
| proxmox/config__ha__rules.go | Removes unused rawDigest fields from HA rule structs |
| proxmox/config__ha__rules_test.go | Updates tests to remove rawDigest references |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func (id GuestID) deleteHaResource(ctx context.Context, c clientApiInterface) error { | ||
| err := c.deleteHaResource(ctx, id) | ||
| if err != nil { | ||
| if strings.HasPrefix(err.Error(), "500 cannot delete service") { |
Copilot
AI
Sep 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using string prefix matching on error messages is fragile and could break if the API error format changes. Consider checking for specific HTTP status codes or using structured error types instead.
| if strings.HasPrefix(err.Error(), "500 cannot delete service") { | |
| // Try to check for HTTP status code 500 and error message "cannot delete service" | |
| type httpError interface { | |
| StatusCode() int | |
| Error() string | |
| } | |
| if herr, ok := err.(httpError); ok { | |
| if herr.StatusCode() == 500 && strings.Contains(herr.Error(), "cannot delete service") { | |
| return Error.haResourceDoesNotExist(id) | |
| } | |
| } else if strings.HasPrefix(err.Error(), "500 cannot delete service") { | |
| // Fallback to string matching if error type does not expose status code |
Adds functionality to delete a HA Resource.
Removes unused
rawDigestfrom the HA rules.Related to Telmate/terraform-provider-proxmox#1394