Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for getting the pending config in a single API call, optimizing the process of fetching both pending changes and current configuration. It introduces a new pendingCurrentConfig method that returns the configuration with pending changes applied.
Key changes:
- Added new
pendingCurrentConfigmethod that applies pending changes to the current configuration - Refactored existing
pendingConfigmethod topendingActiveConfigfor clarity - Extracted duplicate test helper code into a reusable
baseTestmethod
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| proxmox/vmref.go | Added constants for pending change keys, new pendingCurrentConfig method, and renamed existing method for clarity |
| proxmox/vmref_test.go | Added comprehensive test coverage for the new pendingCurrentConfig functionality |
| proxmox/config__qemu_test.go | Refactored duplicate test setup code into a reusable baseTest helper method |
| proxmox/config__qemu.go | Updated method calls to use the renamed pendingActiveConfig and new pendingCurrentConfig methods |
| proxmox/config__lxc__new.go | Updated method call to use the renamed pendingActiveConfig method |
Comments suppressed due to low confidence (1)
proxmox/config__qemu_test.go:1
- [nitpick] The method name
baseTestis ambiguous and doesn't clearly indicate its purpose. Consider renaming it towithDefaultsForTestingorsetTestDefaultsto better communicate that it sets default values for test scenarios.
package proxmox
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| pendingProperty = "key" | ||
| ) | ||
|
|
||
| // pendingActiveConfig returns the active config without pending changes applied. |
There was a problem hiding this comment.
The comment is misleading. Based on the implementation, this method returns the current config values (which could include previously applied pending changes), not necessarily the 'active config without pending changes'. Consider clarifying this as 'returns the current config values from the pending changes API'.
| // pendingActiveConfig returns the active config without pending changes applied. | |
| // pendingActiveConfig returns the current config values as retrieved from the pending changes API. |
Add support for getting the pending config, which is an optimization for getting the pending flag and the normal config in 1 API call.
Reduced duplicate code between tests.