image: block installation of parallel snap instances #5417
Conversation
Block installation of parallel snap instances either coming from the model assertion or directly form the command line. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
image/image.go
Outdated
| } | ||
|
|
||
| func validateNonLocalSnaps(snaps []string) error { | ||
| nonLocalSnaps := make([]string, len(snaps)) |
There was a problem hiding this comment.
This makes an array of len(snaps) empty strings. This is not what you wanted.
image/image.go
Outdated
| for _, snapName := range snaps { | ||
| _, instanceKey := snap.SplitInstanceName(snapName) | ||
| if instanceKey != "" { | ||
| return fmt.Errorf("cannot use snap %q, parallel snap instances are unsupported", |
There was a problem hiding this comment.
This is a slightly unusual line wrapping. Its more common to put snapName into the same line (we don't care about 80char lines etc).
image/image.go
Outdated
| } | ||
|
|
||
| func validateNonLocalSnaps(snaps []string) error { | ||
| nonLocalSnaps := make([]string, len(snaps)) |
There was a problem hiding this comment.
I think this is meant to be nonLocalSnaps := make([]string, 0, len(snaps).
| _, err = image.DecodeModelAssertion(&image.Options{ | ||
| ModelFile: fn, | ||
| }) | ||
| c.Check(err, ErrorMatches, `cannot use snap "foo_instance", parallel snap instances are unsupported`) |
There was a problem hiding this comment.
Should we test for kernel,gadget,base here as well? That those are not parallel installs?
image/image.go
Outdated
| @@ -183,6 +206,10 @@ func decodeModelAssertion(opts *Options) (*asserts.Model, error) { | |||
| } | |||
| } | |||
|
|
|||
| if err := validateNoParallelSnapInstances(modela.RequiredSnaps()); err != nil { | |||
There was a problem hiding this comment.
Not specific to this PR but why modela and not just model?
image/image.go
Outdated
| _, instanceKey := snap.SplitInstanceName(snapName) | ||
| if instanceKey != "" { | ||
| return fmt.Errorf("cannot use snap %q, parallel snap instances are unsupported", | ||
| snapName) |
There was a problem hiding this comment.
Nitpick, squeeze that one one line.
image/image.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func validateNonLocalSnaps(snaps []string) error { |
There was a problem hiding this comment.
Can you add a comment to this function. I also find it a bit annoying to read since the words together make it harder to distinguish Non from No.
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Codecov Report
@@ Coverage Diff @@
## master #5417 +/- ##
=========================================
+ Coverage 79.05% 79.16% +0.1%
=========================================
Files 504 504
Lines 38081 38326 +245
=========================================
+ Hits 30106 30340 +234
- Misses 5551 5555 +4
- Partials 2424 2431 +7
Continue to review full report at Codecov.
|
Block installation of parallel snap instances either coming from the model
assertion or directly form the command line.