Add Group to new client #534
Conversation
Tinyblargon
added
type/enhancement
There was a problem hiding this comment.
Pull request overview
This PR re-implements group handling functionality in the new client interface, introducing a modern GroupInterface with methods for CRUD operations and member management. The implementation follows the existing pattern used for User and ApiToken interfaces.
Key Changes:
- Introduces
GroupInterfacewith complete CRUD operations (Create, Read, Update, Delete, List, Exists, Set) and member management (AddMembers, RemoveMembers) - Migrates from deprecated methods to new client interface pattern with validation and "NoCheck" variants
- Updates
ConfigGroup.Commentfromstringto*stringfor consistent nil handling
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| proxmox/config__group.go | Core implementation of GroupInterface with CRUD operations and member management logic |
| proxmox/config__group_test.go | Comprehensive unit tests for all group operations using mock server |
| proxmox/config__user.go | Adds helper methods for user-group membership management (addGroups, removeGroups, setGroups) |
| proxmox/config__user_test.go | Updates tests to use pointer-based UserID and removes deprecated helper tests |
| proxmox/client.go | Integrates GroupInterface into ClientNew structure |
| proxmox/client__new.go | Adds Group field to ClientNew |
| proxmox/client__api.go | Adds updateUser helper method for user updates |
| internal/array/array.go | Adds utility functions: Combine, CSV, RemoveItem, RemoveItems for array operations |
| internal/array/array_test.go | Tests for new array utility functions |
| internal/mockServer/requests.go | Adds RequestsPutHandler for flexible PUT request testing |
| test/api/Group/*.go | Integration tests for all group operations (create, read, update, delete, list, exists, add/remove/set members) |
| test/data/test_data_cli/cli_group.go | Updates test data to use pointer-based Comment field |
| cli/command//.go | Updates CLI commands to use new GroupInterface methods |
| proxmox/config__apiToken.go | Refactors to use pointer-based name field for consistency |
| proxmox/config__apiToken_test.go | Updates tests for pointer-based name field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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. | ||
| // proxmox does not seem to enforce any limit on the length of a group name. When going over thousands of characters the ui kinda breaks. |
There was a problem hiding this comment.
The error message has a typo: "characters" is misspelled as "charters". This should be corrected to improve clarity.
| // Check if a groupname is valid. | ||
| func (group GroupName) String() string { return string(group) } // For fmt.Stringer interface. | ||
|
|
||
| // Check if a groupName is valid. |
There was a problem hiding this comment.
The comment has a grammatical error: "groupname" should be two words "group name" for better readability and consistency with the rest of the codebase.
| // Check if a groupName is valid. | |
| // Check if a group name is valid. |
| require.NoError(t, err) | ||
| require.True(t, exists) | ||
| }}, | ||
| {name: `Verify doesn't group exist`, |
There was a problem hiding this comment.
The test name has a grammatical error. The apostrophe in "doesn't" should be removed to maintain consistency with other test names that use the phrase "not exist" rather than "doesn't exist".
| {name: `Verify doesn't group exist`, | |
| {name: `Verify group does not exist`, |
1 participant
Re implements the group handling functionality in the New client interface.
Related to #483