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
snap: do exit 0 on install/remove if that snap is already installed or already removed #2292
snap: do exit 0 on install/remove if that snap is already installed or already removed #2292
Conversation
…d again LP: #1622782
ac06fdd
to
6d28db7
Compare
This PR does not address root cause of the problem in LP: #1622782 It will allow for another way to detect "snap already installed" failure (using exit code rather than string) but calls like this will still cause traceback: The least surprising behaviour is that of apt - if package is already installed let user know and exit with code 0 |
@jacekn Silly me, the title of the PR was wrong. The goal is to have subprocess.check_call(["snap", "install", "already-installed-snap"]) not fail, the code should return cleanly (exit 0). |
f94448a
to
68e4d5a
Compare
…f-already-installed
…f-already-installed
…f-already-installed
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 much welcome improvement. I've added a few comments but I don't think any of them block this.
@@ -971,6 +971,40 @@ func (inst *snapInstruction) dispatch() snapActionFunc { | |||
return snapInstructionDispTable[inst.Action] | |||
} | |||
|
|||
func (inst *snapInstruction) errToResponse(err error) Response { | |||
if _, ok := err.(*snap.AlreadyInstalledError); ok { |
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 switch on the type would feel nicer here but you don't have to change it now.
} | ||
|
||
func (e AlreadyInstalledError) Error() string { | ||
return fmt.Sprintf("snap %q is already installed", e.Snap) |
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.
Perhaps a place to add i18n (later)
} | ||
|
||
type NotInstalledError struct { | ||
Snap string |
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.
SnapRef ;-)
Rev Revision | ||
} | ||
|
||
func (e NotInstalledError) Error() string { |
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 :)
LP: #1622782