o/snapstate: deduplicate snap names in remove/install/update #10910
Conversation
MiguelPires
force-pushed
the
dup-op-fails
branch
from
99c97f7
to
d7d590b
Compare
overlord/snapstate/snapstate.go
Outdated
| // * is duplicated in the list | ||
| func validateSnapNames(op string, names []string) error { | ||
| if invalidNames := findInvalidSnapNames(names); len(invalidNames) > 0 { | ||
| return fmt.Errorf("cannot %s due to invalid snap names: %v", op, strings.Join(invalidNames, ", ")) |
There was a problem hiding this comment.
Why aren't you just passing invalidNames to fmt.Errorf? It should be doing the string concatenation itself.
There was a problem hiding this comment.
Thought it looked simpler without the brackets around the list. I can change it though, it's not a strong opinion
There was a problem hiding this comment.
Mine is not strong either, I was mostly curious :-) Let's see what others say.
Codecov Report
@@ Coverage Diff @@
## master #10910 +/- ##
=======================================
Coverage 78.18% 78.18%
=======================================
Files 910 910
Lines 102783 102781 -2
=======================================
+ Hits 80360 80363 +3
+ Misses 17404 17398 -6
- Partials 5019 5020 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
overlord/snapstate/snapstate.go
Outdated
| // (only one condition needs to be met): | ||
| // * differs from the expected format | ||
| // * is duplicated in the list | ||
| func validateSnapNames(op string, names []string) error { |
There was a problem hiding this comment.
since we're changing it, this should probably be named like validateSnapNamesForOp(...) since a generic validation would probably be placed in snap/naming, and this one here is operation specific and returns an contextualized error.
overlord/snapstate/snapstate.go
Outdated
| @@ -1195,6 +1199,10 @@ func UpdateMany(ctx context.Context, st *state.State, names []string, userID int | |||
| type updateFilter func(*snap.Info, *SnapState) bool | |||
|
|
|||
| func updateManyFiltered(ctx context.Context, st *state.State, names []string, userID int, filter updateFilter, flags *Flags, fromChange string) ([]string, []*state.TaskSet, error) { | |||
| if err := validateSnapNames("refresh", names); err != nil { | |||
There was a problem hiding this comment.
Also, for refresh, passing duplicates worked before. I wonder if changing this now can break people. It's probably useful to get a second opinion from @pedronis
There was a problem hiding this comment.
my thinking is if refresh worked so far, to just make duplicate names in install also work by coalescing them, so:
"snap install|refresh foo bar foo baz" is simply the same as "snap install|refresh foo bar baz"
scripts using env variables might be scenarios where this kind of thing happens
pedronis
added
Needs Samuele review
MiguelPires
force-pushed
the
dup-op-fails
branch
from
d7d590b
to
0e16c10
Compare
|
I forced-pushed since the new code doesn't have much in common with the previous one. The new version deduplicates the snap names pass into *Many functions. I'll update the PR title to reflect this |
MiguelPires
changed the title
|
|
||
| installed, ts, err := snapstate.InstallMany(s.state, []string{"some-snap", "some-base", "some-snap", "some-base"}, s.user.ID) | ||
| c.Assert(err, IsNil) | ||
| c.Check(installed, testutil.DeepUnsortedMatches, []string{"some-snap", "some-base"}) |
There was a problem hiding this comment.
you can probably use DeepEquals as the order is preserved
| c.Check(installed, testutil.DeepUnsortedMatches, []string{"some-snap", "some-base"}) | |
| c.Check(installed, DeepEquals, []string{"some-snap", "some-base"}) |
There was a problem hiding this comment.
It's true for install but for UpdateMany we actually invert the order
strutil/strutil_test.go
Outdated
| }{ | ||
| {input: []string{"a", "b", "c"}, output: []string{"a", "b", "c"}}, | ||
| {input: []string{"a", "b", "a"}, output: []string{"a", "b"}}, | ||
| {input: []string{"a", "a", "a", "a", "b", "b", "a"}, output: []string{"a", "b"}}, |
There was a problem hiding this comment.
| {input: []string{"a", "a", "a", "a", "b", "b", "a"}, output: []string{"a", "b"}}, | |
| // order of of first occurrence is preserved | |
| {input: []string{"a", "a", "a", "c", "a", "b", "b", "a"}, output: []string{"a", "c", "b"}}, |
Calling snap remove, install or refresh with the same snap name twice was causes the operation to succeed once and fail with strange errors.
This PR extends the validation of snapstate operations to prevent calling these operations with duplicate snap names. It also adds validation of the snap name format to some operations that didn't have it previously.This PR deduplicates the snap names passed into remove, install and update.https://bugs.launchpad.net/snapd/+bug/1843725