-
Notifications
You must be signed in to change notification settings - Fork 574
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
overlord/snapstate: avoid creating command aliases for daemons #3323
overlord/snapstate: avoid creating command aliases for daemons #3323
Conversation
…hing about daemon/services as well but later)
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, one question/suggestion inline for easier readability.
overlord/snapstate/aliasesv2.go
Outdated
@@ -225,8 +230,8 @@ func refreshAliases(st *state.State, info *snap.Info, curAliases map[string]*Ali | |||
if curTarget.Manual == "" { | |||
continue | |||
} | |||
if info.Apps[curTarget.Manual] == nil { | |||
// not an existing app | |||
if !nonDaemon(info, curTarget.Manual) { |
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.
Is there a way to avoid the double negative here (!nonDaemon()
). Maybe something like: if info.Apps[curTarget.Manual] != nil || isDaemon(info, curTarget.Manual)
(slightly longer but I find it easier to read, but maybe thats just a personal preference).
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.
I was trying not too repeat .Apps all over the place but maybe readability is more important
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
overlord/snapstate/aliasesv2.go
Outdated
@@ -201,6 +201,11 @@ func autoAliasesDelta(st *state.State, names []string) (changed map[string][]str | |||
return changed, dropped, firstErr | |||
} | |||
|
|||
func nonDaemon(info *snap.Info, appName string) bool { | |||
app := info.Apps[appName] | |||
return app != nil && app.Daemon == "" |
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.
Will this place need changes when we land dbus-activated services?
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.
interesting question, I don't see changes to
https://github.com/snapcore/snapd/blob/master/wrappers/binaries.go#L37
in #2592
@mvo?
if we need to not have binaires for those maybe we can move to have a helper wrappers.HasBinary(app) , sounds like something to do in #2592 though
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.
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.
\o/
We have plans to do something about daemons/services as well but for now avoid creating confusion, spurious symlinks.