Skip to content

Conversation

@Tinyblargon
Copy link
Collaborator

Add support for the new HA rules introduced in PVE 9.0

Add tests for all pure functions.

@Tinyblargon Tinyblargon requested a review from Copilot September 24, 2025 20:17
@Tinyblargon Tinyblargon self-assigned this Sep 24, 2025
@Tinyblargon Tinyblargon added type/feature Completely new functionality. modifies/go Pull requests that update Go code size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files test/done This PR has been tested and the result was succesfull labels Sep 24, 2025
Copy link
Contributor

Copilot AI left a 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 support for HA (High Availability) rules introduced in Proxmox VE 9.0, including comprehensive test coverage for all pure functions. The implementation provides full CRUD operations for both node affinity and resource affinity rules with proper validation and API integration.

Key changes:

  • Complete HA rules implementation with version checking for PVE 9.0+
  • Comprehensive test suite covering all scenarios and edge cases
  • New test data utilities for HA rule ID validation

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
proxmox/config__ha__rules.go Core HA rules implementation with types, validation, and API methods
proxmox/config__ha__rules_test.go Comprehensive test suite covering all HA rule functionality
test/data/test_data_ha/type_HaRuleID.go Test data utilities for HA rule ID validation scenarios
proxmox/client__new.go Extended client interface with HA rule methods
proxmox/client__api.go API client implementation for HA rule endpoints
proxmox/client.go Added version constant for PVE 9.0
internal/array/array.go New utility package for array operations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

if err := haVersionCheck(ctx, c); err != nil {
return err
}
if err := ha.validateUpdate(); err != nil {
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

Missing return statement after error check. The error should be returned if validation fails.

Suggested change
if err := ha.validateUpdate(); err != nil {
if err := ha.validateUpdate(); err != nil {
return err

Copilot uses AI. Check for mistakes.
HaNodeAffinityRule_Error_GuestsEmpty = "guests must not be empty"
HaNodeAffinityRule_Error_GuestsRequired = "guests must be specified during creation"
HaNodeAffinityRule_Error_Kind = "rule is not a node affinity rule"
HaNodeAffinityRule_Error_NodesEmpty = "modes must not be empty"
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

Typo in error message: 'modes' should be 'nodes'.

Suggested change
HaNodeAffinityRule_Error_NodesEmpty = "modes must not be empty"
HaNodeAffinityRule_Error_NodesEmpty = "nodes must not be empty"

Copilot uses AI. Check for mistakes.
Comment on lines 758 to 767
resoures := make([]string, len(guests))
for i := range guests {
switch (guests)[i].vmType {
case GuestQemu:
resoures[i] = haGuestPrefixVm + (guests)[i].vmId.String()
case GuestLxc:
resoures[i] = haGuestPrefixCt + (guests)[i].vmId.String()
}
}
params[haRuleApiKeyResources] = resoures
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

Variable name 'resoures' should be 'resources'.

Suggested change
resoures := make([]string, len(guests))
for i := range guests {
switch (guests)[i].vmType {
case GuestQemu:
resoures[i] = haGuestPrefixVm + (guests)[i].vmId.String()
case GuestLxc:
resoures[i] = haGuestPrefixCt + (guests)[i].vmId.String()
}
}
params[haRuleApiKeyResources] = resoures
resources := make([]string, len(guests))
for i := range guests {
switch (guests)[i].vmType {
case GuestQemu:
resources[i] = haGuestPrefixVm + (guests)[i].vmId.String()
case GuestLxc:
resources[i] = haGuestPrefixCt + (guests)[i].vmId.String()
}
}
params[haRuleApiKeyResources] = resources

Copilot uses AI. Check for mistakes.
var haRuleIdRegex = regexp.MustCompile(`^[a-zA-Z0-9\-_]+$`)

const (
HaRuleID_Error_MinLength = `ha rule ID must atleast be 2 characters`
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

Spelling error: 'atleast' should be 'at least'.

Suggested change
HaRuleID_Error_MinLength = `ha rule ID must atleast be 2 characters`
HaRuleID_Error_MinLength = `ha rule ID must at least be 2 characters`

Copilot uses AI. Check for mistakes.
HaRuleID_Error_MinLength = `ha rule ID must atleast be 2 characters`
HaRuleID_Error_MaxLength = `ha rule ID has a maximum of 128 characters`
HaRuleID_Error_Invalid = `ha rule ID did not match the following regex '^[a-zA-Z][a-zA-Z0-9\-_]{2,127}$'`
HaRuleID_Error_Start = `ha rule ID can only with a lower or upper case letter`
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

Grammar error in message: should be 'ha rule ID can only start with a lower or upper case letter'.

Suggested change
HaRuleID_Error_Start = `ha rule ID can only with a lower or upper case letter`
HaRuleID_Error_Start = `ha rule ID can only start with a lower or upper case letter`

Copilot uses AI. Check for mistakes.
@Tinyblargon Tinyblargon requested a review from Copilot September 24, 2025 20:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

return nil
}

// HaRuleID has a minimim of 2 characters and max of 128 characters.
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

There's a typo in the comment: 'minimim' should be 'minimum'

Suggested change
// HaRuleID has a minimim of 2 characters and max of 128 characters.
// HaRuleID has a minimum of 2 characters and max of 128 characters.

Copilot uses AI. Check for mistakes.
const (
HaRuleID_Error_MinLength = `ha rule ID must at least be 2 characters`
HaRuleID_Error_MaxLength = `ha rule ID has a maximum of 128 characters`
HaRuleID_Error_Invalid = `ha rule ID did not match the following regex '^[a-zA-Z][a-zA-Z0-9\-_]{2,127}$'`
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

The error message references a regex pattern that doesn't match the actual regex used in the code. The actual regex is ^[a-zA-Z0-9\-_]+$ but the error message shows ^[a-zA-Z][a-zA-Z0-9\-_]{2,127}$

Suggested change
HaRuleID_Error_Invalid = `ha rule ID did not match the following regex '^[a-zA-Z][a-zA-Z0-9\-_]{2,127}$'`
HaRuleID_Error_Invalid = `ha rule ID did not match the following regex '^[a-zA-Z0-9\-_]+$'`

Copilot uses AI. Check for mistakes.
Comment on lines 776 to 778
builder.WriteString(string(nodes[i].Node.String()))
} else {
builder.WriteString(string(nodes[i].Node.String()))
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

Redundant string conversion. nodes[i].Node.String() already returns a string, so the outer string() conversion is unnecessary and should be removed.

Suggested change
builder.WriteString(string(nodes[i].Node.String()))
} else {
builder.WriteString(string(nodes[i].Node.String()))
builder.WriteString(nodes[i].Node.String())
} else {
builder.WriteString(nodes[i].Node.String())

Copilot uses AI. Check for mistakes.
Comment on lines 776 to 778
builder.WriteString(string(nodes[i].Node.String()))
} else {
builder.WriteString(string(nodes[i].Node.String()))
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

Redundant string conversion. nodes[i].Node.String() already returns a string, so the outer string() conversion is unnecessary and should be removed.

Suggested change
builder.WriteString(string(nodes[i].Node.String()))
} else {
builder.WriteString(string(nodes[i].Node.String()))
builder.WriteString(nodes[i].Node.String())
} else {
builder.WriteString(nodes[i].Node.String())

Copilot uses AI. Check for mistakes.
@Tinyblargon Tinyblargon requested a review from Copilot September 24, 2025 20:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

// HaRuleID has a minimum of 2 characters and max of 128 characters.
type HaRuleID string

var haRuleIdRegex = regexp.MustCompile(`^[a-zA-Z0-9\-_]+$`)
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

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

The regex pattern allows underscores but the error message on line 834 suggests underscores are not allowed. The error message should be updated to match the actual regex pattern or the regex should be corrected to exclude underscores if they're not intended to be valid.

Suggested change
var haRuleIdRegex = regexp.MustCompile(`^[a-zA-Z0-9\-_]+$`)
var haRuleIdRegex = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9\-_]{1,127}$`)

Copilot uses AI. Check for mistakes.
@Tinyblargon Tinyblargon added test/needen This PR has to be tested and removed test/done This PR has been tested and the result was succesfull labels Sep 24, 2025
@Tinyblargon Tinyblargon removed the test/needen This PR has to be tested label Sep 25, 2025
@Tinyblargon Tinyblargon added the test/done This PR has been tested and the result was succesfull label Sep 25, 2025
@Tinyblargon Tinyblargon merged commit fe036ac into Telmate:master Sep 25, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

modifies/go Pull requests that update Go code size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files test/done This PR has been tested and the result was succesfull type/feature Completely new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant