feat: return agent state as enum #521
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tinyblargon
added
type/enhancement
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a GuestAgentState enum to replace error-based parsing for agent states, providing a cleaner API for downstream consumers to determine guest agent status without parsing error strings.
Key Changes:
- Introduces a
GuestAgentStateenum with four states: Unknown, Running, NotRunning, and VmNotRunning - Updates function signatures across the codebase to return the state enum alongside data and errors
- Modifies the deprecated
GetVmAgentNetworkInterfacesfunction to use the new state enum for backward compatibility
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| proxmox/data__qemu__agent.go | Defines the GuestAgentState enum and implements state-aware logic in getAgentInformation() |
| proxmox/client__api.go | Updates API layer to set state based on specific error messages from Proxmox |
| proxmox/client__api__mock.go | Updates mock implementation signatures to match new state parameter |
| proxmox/client__new.go | Updates interface definition to include state in return signature |
| proxmox/client__new__mock.go | Updates mock client signatures to match new state parameter |
| proxmox/client.go | Modifies deprecated function to use state enum and convert to appropriate errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
proxmox/data__qemu__agent.go
Outdated
| return nil, nil | ||
| var state GuestAgentState | ||
| params, err := c.api.getGuestQemuAgent(ctx, vmr, &state) | ||
| if state == GuestAgentRunning || state == GuestAgentVmNotRunning { |
There was a problem hiding this comment.
Logic error: This condition returns early with nil data when state == GuestAgentRunning, but the agent being running means data should be available and fetched. Only GuestAgentNotRunning and GuestAgentVmNotRunning should trigger an early return. This condition should be:
if state == GuestAgentNotRunning || state == GuestAgentVmNotRunning {
Suggested change
| if state == GuestAgentRunning || state == GuestAgentVmNotRunning { | |
| if state == GuestAgentNotRunning || state == GuestAgentVmNotRunning { |
proxmox/client__api.go
Outdated
| getGuestConfig(ctx context.Context, vmr *VmRef) (map[string]any, error) | ||
| getGuestPendingChanges(ctx context.Context, vmr *VmRef) ([]any, error) | ||
| getGuestQemuAgent(ctx context.Context, vmr *VmRef, isRunning *bool) (map[string]any, error) | ||
| getGuestQemuAgent(ctx context.Context, vmr *VmRef, isRunning *GuestAgentState) (map[string]any, error) |
There was a problem hiding this comment.
Parameter name isRunning is inconsistent with the actual type *GuestAgentState. It should be renamed to state to match the implementation and other files (e.g., client__api__mock.go already uses state).
proxmox/data__qemu__agent.go
Outdated
Comment on lines
28
to
40
| func (vmr *VmRef) getAgentInformation(ctx context.Context, c *clientNew) (*rawAgentNetworkInterfaces, GuestAgentState, error) { | ||
| if err := c.oldClient.CheckVmRef(ctx, vmr); err != nil { | ||
| return nil, err | ||
| return nil, GuestAgentUnknown, err | ||
| } | ||
| var isRunning bool | ||
| params, err := c.api.getGuestQemuAgent(ctx, vmr, &isRunning) | ||
| if !isRunning { | ||
| return nil, nil | ||
| var state GuestAgentState | ||
| params, err := c.api.getGuestQemuAgent(ctx, vmr, &state) | ||
| if state == GuestAgentRunning || state == GuestAgentVmNotRunning { | ||
| return nil, state, nil | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, GuestAgentUnknown, err | ||
| } | ||
| return &rawAgentNetworkInterfaces{a: params}, nil | ||
| return &rawAgentNetworkInterfaces{a: params}, GuestAgentRunning, nil |
There was a problem hiding this comment.
The new state handling logic (particularly the state checks and conversions on lines 34-40) lacks test coverage. Consider adding tests that verify:
- When the agent is not running, the function returns
(nil, GuestAgentNotRunning, nil) - When the VM is not running, the function returns
(nil, GuestAgentVmNotRunning, nil) - When the agent is running and returns data successfully, the function returns
(data, GuestAgentRunning, nil) - When an unexpected error occurs, the function returns
(nil, GuestAgentUnknown, error)
Tinyblargon
added
test/done
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before we returned the agent state as errors downstream projects would have to parse.
Now we return an enum, removing the need for downstream error parsing for agent related errors.