Skip to content

Commit

Permalink
Merge pull request #374 from Tinyblargon/optimize1
Browse files Browse the repository at this point in the history 
Optimize
  • Loading branch information
@Tinyblargon
Tinyblargon authored Nov 9, 2024
2 parents 5b6e96a + 6d0a520 commit 0146e30
Show file tree
Hide file tree
Showing 8 changed files with 116 additions and 241 deletions.
20 changes: 18 additions & 2 deletions docs/style-guide/sdk.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,22 @@ Public functions should follow this guide as much as possible, to realize a homo
It is important to note that every public function becomes a critical point of reliance for some software.
As such, it is imperative to uphold backward compatibility when making changes. See [Versioning](#versioning) for more information.

## Types

In the SDK, we use public and private types. This section will discuss the naming conventions for these types.

### Public Types

For now we don't have any specific naming conventions for public types. The only thing that is important that it conveys it's purpose, scope and usage.

### Private Types

For private types we use the following naming conventions:

#### Enumerations

For an internal enum type, the name must be suffixed with `Enum`.

## Standardized Interfaces

Standardized interfaces are key in software development for consistent functionality exposure. They enhance code understanding, usage, and system interoperability.
Expand Down Expand Up @@ -414,11 +430,11 @@ func Test_UserID_Validate(t *testing.T) {
input UserID
output error
}{
{name: "Valid ID",
{name: `Valid ID`,
input: UserID(1),
output: nil,
},
{name: "Invalid ID",
{name: `Invalid ID`,
input: UserID(0),
output: errors.New(UserID_Error_Invalid),
},
Expand Down
44 changes: 17 additions & 27 deletions proxmox/config_group.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type ConfigGroup struct {
}

// Creates the specified group
func (config *ConfigGroup) Create(client *Client) error {
func (config ConfigGroup) Create(client *Client) error {
config.Validate(true)
params := config.mapToApiValues(true)
err := client.Post(params, "/access/groups")
Expand All @@ -31,7 +31,7 @@ func (config *ConfigGroup) Create(client *Client) error {
}

// Maps the struct to the API values proxmox understands
func (config *ConfigGroup) mapToApiValues(create bool) (params map[string]interface{}) {
func (config ConfigGroup) mapToApiValues(create bool) (params map[string]interface{}) {
params = map[string]interface{}{
"comment": config.Comment,
}
Expand All @@ -54,20 +54,8 @@ func (config ConfigGroup) mapToStruct(params map[string]interface{}) *ConfigGrou
return &config
}

// Custom error for when the *ConfigGroup is nil
func (config *ConfigGroup) nilCheck() error {
if config == nil {
return errors.New("pointer for (ConfigGroup) is nil")
}
return nil
}

// Created or updates the specified group
func (config *ConfigGroup) Set(client *Client) (err error) {
err = config.nilCheck()
if err != nil {
return
}
func (config ConfigGroup) Set(client *Client) (err error) {
existence, err := config.Name.CheckExistence(client)
if err != nil {
return
Expand All @@ -79,7 +67,7 @@ func (config *ConfigGroup) Set(client *Client) (err error) {
}

// Updates the specified group
func (config *ConfigGroup) Update(client *Client) error {
func (config ConfigGroup) Update(client *Client) error {
config.Validate(false)
params := config.mapToApiValues(true)
err := client.Put(params, "/access/groups/"+string(config.Name))
Expand All @@ -94,14 +82,10 @@ func (config *ConfigGroup) Update(client *Client) error {
}

// Validates all items and sub items of the ConfigGroup
func (config *ConfigGroup) Validate(create bool) (err error) {
err = config.nilCheck()
if err != nil {
func (config ConfigGroup) Validate(create bool) (err error) {
if err = config.Name.Validate(); err != nil {
return
}
if create {
err = config.Name.Validate()
}
if config.Members != nil {
for _, e := range *config.Members {
err = e.Validate()
Expand All @@ -116,6 +100,12 @@ func (config *ConfigGroup) Validate(create bool) (err error) {
// GroupName may only contain the following characters: abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890-_
type GroupName string

const (
GroupName_Error_Invalid string = "variable of type (GroupName) may only contain the following characters: -_abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890"
GroupName_Error_Empty string = "variable of type (GroupName) may not be empty"
GroupName_Error_MaxLength string = "variable of type (GroupName) may not be more tha 1000 characters long"
)

// Add users to the specified group
func (group GroupName) AddUsersToGroup(members *[]UserID, client *Client) error {
users, err := listUsersFull(client)
Expand Down Expand Up @@ -368,17 +358,17 @@ func (group GroupName) usersToRemoveFromGroup(allUsers []interface{}, members *[
// Check if a groupname is valid.
func (group GroupName) Validate() error {
if group == "" {
return errors.New("variable of type (GroupName) may not be empty")
return errors.New(GroupName_Error_Empty)
}
// proxmox does not seem to enforce any limit on the length of a group name. When going over thousands of charters the ui kinda breaks.
if len([]rune(group)) > 1000 {
return errors.New("variable of type (GroupName) may not be more tha 1000 characters long")
return errors.New(GroupName_Error_MaxLength)
}
regex, _ := regexp.Compile(`^([a-z]|[A-Z]|[0-9]|_|-)*$`)
if regex.Match([]byte(group)) {
return nil
if !regex.Match([]byte(group)) {
return errors.New(GroupName_Error_Invalid)
}
return errors.New("")
return nil
}

// Returns a list of all existing groups
Expand Down
66 changes: 24 additions & 42 deletions proxmox/config_group_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package proxmox

import (
"errors"
"testing"

"github.com/Telmate/proxmox-api-go/test/data/test_data_group"
Expand Down Expand Up @@ -114,72 +115,49 @@ func Test_ConfigGroup_mapToStruct(t *testing.T) {
}
}

func Test_ConfigGroup_nilCheck(t *testing.T) {
testData := []struct {
input *ConfigGroup
err bool
}{
{input: &ConfigGroup{}},
{err: true},
}
for _, e := range testData {
if e.err {
require.Error(t, e.input.nilCheck())
} else {
require.NoError(t, e.input.nilCheck())
}
}
}

// TODO improve when Name and Realm have their own types
func Test_ConfigGroup_Validate(t *testing.T) {
validGroupName := GroupName("groupName")
False := 0
TrueAndFalse := 1
True := 2
testData := []struct {
input *ConfigGroup
err bool
input ConfigGroup
hasError bool
err error

create int
}{
// GroupName
{
err: true,
create: TrueAndFalse,
},
input: ConfigGroup{},
err: errors.New(GroupName_Error_Empty),
create: TrueAndFalse},
{
input: &ConfigGroup{},
err: true,
create: True,
},
{input: &ConfigGroup{}},
input: ConfigGroup{Name: GroupName(test_data_group.GroupName_Max_Legal())},
create: TrueAndFalse},
{
input: &ConfigGroup{Name: GroupName(test_data_group.GroupName_Max_Legal())},
create: True,
},
{
input: &ConfigGroup{Name: GroupName(test_data_group.GroupName_Max_Illegal())},
err: true,
create: True,
},
input: ConfigGroup{Name: GroupName(test_data_group.GroupName_Max_Illegal())},
err: errors.New(GroupName_Error_MaxLength),
create: TrueAndFalse},
// GroupMembers
{
input: &ConfigGroup{
input: ConfigGroup{
Name: validGroupName,
Members: &[]UserID{
{Name: "user1"},
}},
err: true,
create: TrueAndFalse,
hasError: true,
create: TrueAndFalse,
},
{
input: &ConfigGroup{
input: ConfigGroup{
Name: validGroupName,
Members: &[]UserID{{Name: "user1", Realm: "pam"}}},
create: TrueAndFalse,
},
{
input: &ConfigGroup{
input: ConfigGroup{
Name: validGroupName,
Members: &[]UserID{
{Name: "user1", Realm: "pam"},
Expand All @@ -191,14 +169,18 @@ func Test_ConfigGroup_Validate(t *testing.T) {
}
for _, e := range testData {
if e.create < True {
if e.err {
if e.err != nil {
require.Equal(t, e.err, e.input.Validate(false))
} else if e.hasError {
require.Error(t, e.input.Validate(false))
} else {
require.NoError(t, e.input.Validate(false))
}
}
if e.create > False {
if e.err {
if e.err != nil {
require.Equal(t, e.err, e.input.Validate(false))
} else if e.hasError {
require.Error(t, e.input.Validate(true))
} else {
require.NoError(t, e.input.Validate(true))
Expand Down
43 changes: 0 additions & 43 deletions proxmox/config_qemu.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,11 @@ import (
// Currently ZFS local, LVM, Ceph RBD, CephFS, Directory and virtio-scsi-pci are considered.
// Other formats are not verified, but could be added if they're needed.
// const rxStorageTypes = `(zfspool|lvm|rbd|cephfs|dir|virtio-scsi-pci)`
const machineModels = `(pc|q35|pc-i440fx)`

type (
QemuDevices map[int]map[string]interface{}
QemuDevice map[string]interface{}
QemuDeviceParam []string
IpconfigMap map[int]interface{}
)

// ConfigQemu - Proxmox API QEMU options
Expand Down Expand Up @@ -1046,15 +1044,6 @@ func FormatDiskParam(disk QemuDevice) string {
return strings.Join(diskConfParam, ",")
}

// Given a QemuDevice (representing a usb), return a param string to give to ProxMox
func FormatUsbParam(usb QemuDevice) string {
usbConfParam := QemuDeviceParam{}

usbConfParam = usbConfParam.createDeviceParam(usb, []string{})

return strings.Join(usbConfParam, ",")
}

// Create RNG parameter.
func (c ConfigQemu) CreateQemuRngParams(params map[string]interface{}) {
rngParam := QemuDeviceParam{}
Expand Down Expand Up @@ -1098,24 +1087,6 @@ func (c ConfigQemu) CreateQemuEfiParams(params map[string]interface{}) {
}
}

// Create parameters for each disk.
func (c ConfigQemu) CreateQemuDisksParams(params map[string]interface{}, cloned bool) {
// For new style with multi disk device.
for diskID, diskConfMap := range c.QemuDisks {
// skip the first disk for clones (may not always be right, but a template probably has at least 1 disk)
if diskID == 0 && cloned {
continue
}

// Device name.
deviceType := diskConfMap["type"].(string)
qemuDiskName := deviceType + strconv.Itoa(diskID)

// Add back to Qemu prams.
params[qemuDiskName] = FormatDiskParam(diskConfMap)
}
}

// Create parameters for each PCI Device
func (c ConfigQemu) CreateQemuPCIsParams(params map[string]interface{}) {
// For new style with multi pci device.
Expand All @@ -1134,20 +1105,6 @@ func (c ConfigQemu) CreateQemuPCIsParams(params map[string]interface{}) {
}
}

// Create parameters for serial interface
func (c ConfigQemu) CreateQemuMachineParam(
params map[string]interface{},
) error {
if c.Machine == "" {
return nil
}
if matched, _ := regexp.MatchString(machineModels, c.Machine); matched {
params["machine"] = c.Machine
return nil
}
return fmt.Errorf("unsupported machine type, fall back to default")
}

func (p QemuDeviceParam) createDeviceParam(
deviceConfMap QemuDevice,
ignoredKeys []string,
Expand Down
2 changes: 1 addition & 1 deletion proxmox/config_qemu_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8472,7 +8472,7 @@ func Test_ConfigQemu_Validate(t *testing.T) {
QemuUsbID0: QemuUSB{Mapping: &QemuUsbMapping{}}}}),
current: &ConfigQemu{USBs: QemuUSBs{
QemuUsbID0: QemuUSB{Port: &QemuUsbPort{}}}},
err: errors.New(QemuUSB_Error_MappedID)},
err: errors.New(QemuUSB_Error_MappingID)},
{name: `errors.New(QemuUSB_Error_PortID)`,
input: baseConfig(ConfigQemu{USBs: QemuUSBs{
QemuUsbID0: QemuUSB{Port: &QemuUsbPort{}}}}),
Expand Down
Loading

0 comments on commit 0146e30

Please sign in to comment.