-
Notifications
You must be signed in to change notification settings - Fork 599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
o/snapstate: move auxinfo functions to snapstate backend with metadata helpers #15051
Merged
bboozzoo
merged 14 commits into
canonical:master
from
olivercalder:backend-metadata-management
Feb 14, 2025
Merged
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
2aaf366
o/snapstate: move auxinfo functions to snapstate backend with metadat…
olivercalder ea63622
o/snapstate: only discard store metadata if no other instances present
olivercalder c21b939
o/snapstate: remove unnecessary check for non-empty snapID before ins…
olivercalder f7fefa1
o/snapstate: make InstallStoreMetadata return an undo callback
olivercalder 06c89d8
o/snapstate/backend: disambiguate metadata management test cases
olivercalder 025b681
o/snapstate: revise comments around handling of empty snap id in inst…
olivercalder 2e56a77
o/snapstate: use LinkContext in InstallStoreMetadata instead of indiv…
olivercalder 92b4513
o/snapstate: pass aux store info by value, not pointer
olivercalder e590adc
o/snapstate: move store metadata calls to within backend methods
olivercalder 5f12e92
Revert "o/snapstate: move store metadata calls to within backend meth…
olivercalder 066392d
o/snapstate: pass link context into DiscardStoreMetadata
olivercalder 37e95bf
o/snapstate{,/backend}: split metadata removal helper into uninstall …
olivercalder bc79190
o/snapstate: remove outdated comment
olivercalder b16c644
o/snapstate/backend: DiscardStoreMetadata directly removes metadata
olivercalder File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
// -*- Mode: Go; indent-tabs-mode: t -*- | ||
|
||
/* | ||
* Copyright (C) 2025 Canonical Ltd | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU General Public License version 3 as | ||
* published by the Free Software Foundation. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
* | ||
*/ | ||
|
||
package backend | ||
|
||
// InstallStoreMetadata saves revision-agnostic metadata to disk for the snap | ||
// with the given snap ID. At the moment, this metadata includes auxiliary | ||
// store information. | ||
func InstallStoreMetadata(snapID string, aux *AuxStoreInfo) error { | ||
if snapID == "" { | ||
return nil | ||
} | ||
if err := keepAuxStoreInfo(snapID, aux); err != nil { | ||
return err | ||
} | ||
// TODO: install other types of revision-agnostic metadata | ||
return nil | ||
} | ||
|
||
// DiscardStoreMetadata removes revision-agnostic metadata to disk for the snap | ||
// with the given snap ID. At the moment, this metadata includes auxiliary | ||
// store information. If hasOtherInstances is true, does nothing. | ||
func DiscardStoreMetadata(snapID string, hasOtherInstances bool) error { | ||
if hasOtherInstances || snapID == "" { | ||
return nil | ||
} | ||
if err := discardAuxStoreInfo(snapID); err != nil { | ||
return err | ||
} | ||
// TODO: discard other types of revision-agnostic metadata | ||
return nil | ||
} | ||
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
// -*- Mode: Go; indent-tabs-mode: t -*- | ||
|
||
/* | ||
* Copyright (C) 2025 Canonical Ltd | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU General Public License version 3 as | ||
* published by the Free Software Foundation. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
* | ||
*/ | ||
|
||
package backend_test | ||
|
||
import ( | ||
. "gopkg.in/check.v1" | ||
|
||
"github.com/snapcore/snapd/dirs" | ||
"github.com/snapcore/snapd/overlord/snapstate/backend" | ||
"github.com/snapcore/snapd/snap" | ||
"github.com/snapcore/snapd/testutil" | ||
) | ||
|
||
type metadataSuite struct{} | ||
|
||
var _ = Suite(&metadataSuite{}) | ||
|
||
func (s *metadataSuite) SetUpTest(c *C) { | ||
dirs.SetRootDir(c.MkDir()) | ||
} | ||
|
||
func (s *metadataSuite) TestStoreMetadataRoundTrip(c *C) { | ||
const snapID = "my-snap-id" | ||
const hasOtherInstances = false | ||
c.Assert(backend.AuxStoreInfoFilename(snapID), testutil.FileAbsent) | ||
aux := &backend.AuxStoreInfo{ | ||
Media: snap.MediaInfos{ | ||
snap.MediaInfo{ | ||
Type: "icon", | ||
URL: "http://images.com/my-icon", | ||
Width: 128, | ||
Height: 128, | ||
}, | ||
snap.MediaInfo{ | ||
Type: "website", | ||
URL: "http://another.com", | ||
}, | ||
}, | ||
StoreURL: "https://snapcraft.io/example-snap", | ||
Website: "http://example.com", | ||
} | ||
c.Check(backend.InstallStoreMetadata(snapID, aux), IsNil) | ||
c.Check(backend.AuxStoreInfoFilename(snapID), testutil.FilePresent) | ||
|
||
var info snap.Info | ||
info.SnapID = snapID | ||
c.Check(backend.RetrieveAuxStoreInfo(&info), IsNil) | ||
c.Check(info.Media, DeepEquals, aux.Media) | ||
c.Check(info.LegacyWebsite, Equals, aux.Website) | ||
c.Check(info.StoreURL, Equals, aux.StoreURL) | ||
|
||
c.Check(backend.DiscardStoreMetadata(snapID, hasOtherInstances), IsNil) | ||
c.Check(backend.AuxStoreInfoFilename(snapID), testutil.FileAbsent) | ||
} | ||
|
||
func (s *metadataSuite) TestStoreMetadataEmptySnapID(c *C) { | ||
const snapID = "" | ||
const hasOtherInstances = false | ||
var aux *backend.AuxStoreInfo | ||
// check that empty snapID does not return an error | ||
c.Check(backend.InstallStoreMetadata(snapID, aux), IsNil) | ||
c.Check(backend.DiscardStoreMetadata(snapID, hasOtherInstances), IsNil) | ||
} | ||
|
||
func (s *metadataSuite) TestDiscardStoreMetadataHasOtherInstances(c *C) { | ||
const snapID = "my-snap-id" | ||
c.Assert(backend.AuxStoreInfoFilename(snapID), testutil.FileAbsent) | ||
aux := &backend.AuxStoreInfo{ | ||
StoreURL: "https://snapcraft.io/example-snap", | ||
} | ||
c.Check(backend.InstallStoreMetadata(snapID, aux), IsNil) | ||
c.Check(backend.AuxStoreInfoFilename(snapID), testutil.FilePresent) | ||
|
||
// Check that it does not discard if hasOtherInstances is true | ||
hasOtherInstances := true | ||
c.Check(backend.DiscardStoreMetadata(snapID, hasOtherInstances), IsNil) | ||
c.Assert(backend.AuxStoreInfoFilename(snapID), testutil.FilePresent) | ||
|
||
hasOtherInstances = false | ||
// Check that it is discarded if hasOtherInstances is false | ||
c.Check(backend.DiscardStoreMetadata(snapID, hasOtherInstances), IsNil) | ||
c.Assert(backend.AuxStoreInfoFilename(snapID), testutil.FileAbsent) | ||
} |
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if these could be called directly from within other backend methods (e.g.
backend.LinkSnap
andbackend.UnlinkSnap
), as that would mean nearly/all of the complexity around saving and reverting auxinfo could be avoided. However, that would require those methods to be able to computehasOtherInstances
, which is not possible at the moment, unless we want to start passing snapstate into the backend.Notably, the error handling for
doLinkSnap
afterbackend.LinkSnap(newInfo, ...)
has succeeded already does exactly what we want: if there's another revision, simply callbackend.LinkSnap(oldInfo, ...)
, and if there's no other revision, callbackend.UnlinkSnap
. IfInstallStoreMetadata
andDiscardStoreMetadata
were called from within these, respectively, the result would be as desired.There are lots of other backend functions which are called from the handlers though, so I'm not sure if it would be consistent to place calls to these helpers within
backend.LinkSnap
andbackend.UnlinkSnap
, for example, else why are so many backend helpers called from withinThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to hear your thoughts on this next week @bboozzoo and @pedronis