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
cmd: support installing multiple local snaps #11201
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11201 +/- ##
=======================================
Coverage 78.36% 78.36%
=======================================
Files 922 922
Lines 105128 105167 +39
=======================================
+ Hits 82385 82418 +33
- Misses 17611 17615 +4
- Partials 5132 5134 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -488,7 +488,7 @@ func (x *cmdInstall) installOne(nameOrPath, desiredName string, opts *client.Sna | |||
var snapName string | |||
var path string | |||
|
|||
if strings.Contains(nameOrPath, "/") || strings.HasSuffix(nameOrPath, ".snap") || strings.Contains(nameOrPath, ".snap.") { | |||
if isLocalSnap(nameOrPath) { |
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.
Nice, this is much more readable now!
} | ||
|
||
// InstallPathMany sideloads the snaps with the given paths, | ||
// returning the UUID of the background operation upon success. |
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.
A bit weird we called it UUID (I see it's taken from the existing comment) and not just change id...
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.
This looks good! Could you please add a simple spread test in this PR?
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.
Thanks for adding the spread test. Looks fine, one more suggestion.
echo "Install multiple local snaps" | ||
expected="(basic 1.0 installed\stest-snapd-tools 1.0 installed)|(test-snapd-tools 1.0 installed\sbasic 1.0 installed)" | ||
snap install --dangerous test-snapd-tools_1.0_all.snap basic_1.0_all.snap | MATCH -z "$expected" | ||
snap install --devmode test-snapd-tools_1.0_all.snap basic_1.0_all.snap | MATCH -z "$expected" |
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.
Thanks for this test!
client/snap_op.go
Outdated
for _, openFile := range files { | ||
openFile.Close() | ||
} | ||
return "", fmt.Errorf("cannot open: %q", path) |
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.
How about:
return "", fmt.Errorf("cannot open: %q", path) | |
return "", fmt.Errorf("cannot open %q: %v", path, err) |
(then the old code for installing single file could be updated too).
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.
Looks good, I'll change both. We can also just print the err itself since os.PathError's message includes the path that the op failed on
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.
Looks good, +1
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.
LGTM
Adds
cmd
support for installing multiple local snaps.