Refactor FleetAutoscaler state from map to typed struct#4315
Refactor FleetAutoscaler state from map to typed struct#4315markmandel merged 2 commits intoagones-dev:mainfrom
Conversation
Replace the generic map[string]any state in fasThread with a typed fasState struct containing a wasmPlugin field. This change: - Introduces fasState struct with wasmPlugin *extism.Plugin field - Removes the need for string key constants (wasmStateKey) - Eliminates runtime type assertions when accessing the plugin - Improves type safety and code readability - Updates all related tests to use the new fasState type The refactoring simplifies the code by making the state structure explicit and compile-time checked, reducing potential runtime errors from incorrect type assertions or missing keys.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the FleetAutoscaler state management from a generic map[string]any to a typed fasState struct for improved type safety and code clarity. The changes eliminate runtime type assertions and string key constants, replacing them with compile-time checked struct fields.
Key Changes:
- Introduced
fasStatestruct with awasmPluginfield to replace the generic state map - Removed the
wasmStateKeyconstant and associated string-based map lookups - Simplified cleanup logic in
fasThread.close()by removing nested type assertions
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/fleetautoscalers/controller.go | Introduced fasState struct and updated fasThread to use typed state |
| pkg/fleetautoscalers/fleetautoscalers.go | Updated function signatures to accept fasState and simplified plugin access |
| pkg/fleetautoscalers/controller_test.go | Updated test initialization to use fasState{} |
| pkg/fleetautoscalers/fleetautoscalers_test.go | Updated test calls to use fasState{} |
| pkg/fleetautoscalers/fleetautoscalerwasm_test.go | Updated test initialization to use fasState{} |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return 0, false, errors.Wrapf(err, "failed to create Wasm plugin from %s", u.String()) | ||
| } | ||
| state[wasmStateKey] = plugin // Store the plugin in the state map | ||
| state.wasmPlugin = plugin // Store the plugin in the state map |
There was a problem hiding this comment.
The comment 'Store the plugin in the state map' is outdated since the state is no longer a map. Consider updating to 'Store the plugin in the state'.
| state.wasmPlugin = plugin // Store the plugin in the state map | |
| state.wasmPlugin = plugin // Store the plugin in the state |
|
|
||
| _, ok := state[wasmStateKey] | ||
| if !ok { | ||
| if state.wasmPlugin == nil { |
There was a problem hiding this comment.
The fasState is passed by value, so modifications to state.wasmPlugin inside this function will not persist to the caller. The function should accept *fasState (pointer) instead to allow the plugin to be stored and reused across calls.
There was a problem hiding this comment.
I was debating this. It's a good point, I'll make this change.
|
Build Succeeded 🥳 Build Id: 028aa99d-0cb0-460a-b80f-7ee4bc94e554 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
|
Definitely something flaky with this test. Is it just autopilot though? Seems to be 🤔 /gcbrun |
|
/gcbrun |
lacroixthomas
left a comment
There was a problem hiding this comment.
LGTM !
Looks cleaner this way compared to a map that needs some casting etc. 👌🏼
From your comment on the PR, I'm wondering the issue you found, to move the http client into the state ! Shall see from the next review hehe
|
Build Failed 😭 Build Id: 34d90638-9b47-46c4-9fd8-0f82ce605ce4 Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Failed 😭 Build Id: 07c030e7-dda5-4e93-9b07-ab22f697d6fc Status: FAILURE To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
Build Succeeded 🥳 Build Id: b372d325-ee0c-4b1a-a0f3-9ce967613bbd The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version: |
) * Refactor FleetAutoscaler state from map to typed struct Replace the generic map[string]any state in fasThread with a typed fasState struct containing a wasmPlugin field. This change: - Introduces fasState struct with wasmPlugin *extism.Plugin field - Removes the need for string key constants (wasmStateKey) - Eliminates runtime type assertions when accessing the plugin - Improves type safety and code readability - Updates all related tests to use the new fasState type The refactoring simplifies the code by making the state structure explicit and compile-time checked, reducing potential runtime errors from incorrect type assertions or missing keys. * Review updates
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Replace the generic map[string]any state in fasThread with a typed fasState struct containing a wasmPlugin field. This change:
The refactoring simplifies the code by making the state structure explicit and compile-time checked, reducing potential runtime errors from incorrect type assertions or missing keys.
Which issue(s) this PR fixes:
Cleanup on #4080
Special notes for your reviewer:
Now this is refactored, I'm going to move the http client for the webhook autoscaler into this state, which will simply cert management (and also fix a bug I just found).