daemon, overlord/snapstate: set instance name when installing from snap file #5670
Conversation
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
…ed for local installs Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
…aded snaps Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
…ieces Update the test to enable the experimental flag and account for the missing bits that prevent snap from running at the moment. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Codecov Report
@@ Coverage Diff @@
## master #5670 +/- ##
==========================================
- Coverage 78.95% 78.95% -0.01%
==========================================
Files 522 522
Lines 39941 39965 +24
==========================================
+ Hits 31537 31553 +16
- Misses 5839 5843 +4
- Partials 2565 2569 +4
Continue to review full report at Codecov.
|
| @@ -1477,6 +1477,16 @@ out: | |||
| origPath = form.Value["snap-path"][0] | |||
| } | |||
|
|
|||
| var instanceName string | |||
|
|
|||
| if len(form.Value["name"]) > 0 { | |||
There was a problem hiding this comment.
Is "name" only reflecting what's been already agreed and implemented in the client code? No super strong opinion on this, but looking at snap-path above, and the semantics of this one, it feels like a slightly more elaborate attribute name would be more appropriate - e.g instance-name?
There was a problem hiding this comment.
Yeah the client part is already merged and was introduced in #5426. That's also where name was discussed.
daemon/api_test.go
Outdated
| @@ -2335,10 +2335,14 @@ func (s *apiSuite) sideloadCheck(c *check.C, content string, head map[string]str | |||
| ensureStateSoonImpl(st) | |||
| } | |||
|
|
|||
| mockedName := "local" | |||
| if expectedInstanceName == "" { | |||
| expectedInstanceName = mockedName | |||
There was a problem hiding this comment.
IMHO it's better to explicitely pass "local" to all invocations of sideloadCheck (there are just a few of them) rather than this special-casing. But maybe I missed something, is there a specific reason to do it this way?
| c.Assert(rsp.Type, check.Equals, ResponseTypeError) | ||
| c.Check(rsp.Result.(*errorResult).Message, check.Equals, `instance name "foo_instance" does not match snap name "bar"`) | ||
| } | ||
|
|
overlord/snapstate/snapstate_test.go
Outdated
| _, err := snapstate.InstallPath(s.state, si, mockSnap, "some-snap_foo", "", snapstate.Flags{}) | ||
| c.Assert(err, ErrorMatches, "experimental feature disabled - test it by setting 'experimental.parallel-instances' to true") | ||
|
|
||
| // enable layouts |
There was a problem hiding this comment.
Ha, got you! copy-paste ;)
| @@ -0,0 +1,43 @@ | |||
| summary: Checks for parallel installation of sideloaded snaps | |||
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
The changes enable setting of instance name when installing from snap file by issuing a command
snap install --name foo_bar foo.snap. The client part of the change is already in. This PR adds the backend part.