many: remove *snap.Info field from snapstate.target #16308
Conversation
…freshCandidates with goal filter
…ce usage with snapstate.target
…oreUpdatePlanCore This enables us to eliminate double validation in refreshHintsFromUpdatePlan.
…rate on a SnapSetup
This can be used to resume a refresh. This works well with how refresh-candidates works today, and it will also be used to implement a more general-use staged refresh.
github-actions
bot
added
Needs Documentation -auto-
There was a problem hiding this comment.
Pull request overview
This PR refactors the snapstate package by removing the *snap.Info field from snapstate.target and moving related functionality into the goal-based implementation. This enables implementing goals from data that might not have a *snap.Info, allowing the refresh-candidates code to use the goal-based public API instead of internal functions.
Key changes:
- Replaced
*snap.Infoin targets withSnapSetupand derived helper methods - Modified
InstallOne,InstallPath, and related functions to returnSnapSetupinstead of*snap.Info - Introduced
SetupUpdateGoalfor pre-built SnapSetup data andStoreUpdateGoalWithFilterfor filtered updates
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| overlord/snapstate/target.go | Core refactoring removing *snap.Info from target, adding helper methods and new goal types |
| overlord/snapstate/target_test.go | Updated tests to check SnapSetup instead of *snap.Info |
| overlord/snapstate/storehelpers.go | Modified to use new target structure and filter function parameter |
| overlord/snapstate/snapstate_update_test.go | Updated test signatures for ValidateRefreshes to use SnapSetup |
| overlord/snapstate/snapstate_install_test.go | Updated test expectations for SnapSetup return values |
| overlord/snapstate/snapstate.go | Updated public API signatures and internal implementations |
| overlord/snapstate/refreshhints.go | Simplified to work with targets directly instead of *snap.Info |
| overlord/snapstate/export_test.go | Updated test exports for new Target type |
| overlord/snapstate/component.go | Fixed typo in comment |
| overlord/snapstate/check_snap.go | Added Is method to SnapNeedsClassicError |
| overlord/devicestate/*.go | Updated mock signatures throughout to use SnapSetup |
| overlord/assertstate/assertstate.go | Updated ValidateRefreshes signature to use SnapSetup |
| daemon/*.go | Updated all daemon mocks and tests for new signatures |
Comments suppressed due to low confidence (2)
overlord/snapstate/target.go:1
- The comment on lines 626-627 explains why
SideInfo.Channelshould be empty, butsetup.Channelis now checked on line 630. Add a comment explaining thatsetup.Channelcontains the tracked channel fromRevOpts, distinguishing it fromSideInfo.Channelwhich reflects where the snap was downloaded from.
// -*- Mode: Go; indent-tabs-mode: t -*-
overlord/snapstate/target.go:1
- The comment on lines 703-705 explains the distinction between effective channel and tracked channel, but the new code structure (checking both
SideInfo.Channelandsetup.Channel) makes this less clear. Update the comment to explain thatSideInfo.Channelis the effective channel (where the snap came from) andsetup.Channelis the tracked channel.
// -*- Mode: Go; indent-tabs-mode: t -*-
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Tue Nov 25 21:34:35 UTC 2025 No spread failures reported |
|
@andrewphelpsj thank you so much for cover-letter style introduction to the patch set. |
we need to discuss this, while you latter point is partly true, SnapSetup is too full of snapstate task specific bits to feel entirely clean to pass it around liberally |
| // local revision and sideloading, or full metadata in which case it | ||
| // the snap will appear as installed from the store. | ||
| func InstallPath(st *state.State, si *snap.SideInfo, path, instanceName, channel string, flags Flags, prqt PrereqTracker) (*state.TaskSet, *snap.Info, error) { | ||
| func InstallPath(st *state.State, si *snap.SideInfo, path, instanceName, channel string, flags Flags, prqt PrereqTracker) (*state.TaskSet, SnapSetup, error) { |
There was a problem hiding this comment.
this in particular is a odd change I think
3 participants
This is a pretty big refactor, and some bits are still a little janky.
Main goal: get rid of the
*snap.Infothat is carried by asnapstate.target.This will enable implementing goals from data that might not have a
*snap.Info. Thus, we can swap the code that handles refresh-candidates over to using the goal-based public API, rather than relying on internal functions. Currently,autoRefreshPhase2calls functions that skip over the plan-creation phase of refreshing. Enabling the creation of a goal with a pre-existing plan feels natural.As a nice bonus, a lot of functionality that should have been owned by the goals is moved there. For example, the store goals now handle filtering down auto-refreshes based on gating and validation sets. Previously, this conditionally happened after the
updatePlanwas created. Thus, it wasn't clear if it should apply to goals that orchestrate file-based installations.Note that the
refresh-candidatescase is very similar to how we might implement a more general-use feature of staging a refresh for later. It would share the new goal-based implementation. That is the main motivation for this refactor.Most likely controversial change: Some things that used to operate on a
*snap.Infonow operate on asnapstate.SnapSetup. Logically, I think this makes sense, since that is the data structure that represents an installation/update.Draft for now, I have some cleanup to do still.