/snapd

interfaces: add thumbnailer interface #1775

Closed
wants to merge 29 commits into
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@fkaleo @zyga @morphis @niemeyer @jhenstridge @artmello
@fkaleo

fkaleo commented Aug 29, 2016

No description provided.

@fkaleo fkaleo changed the title from Add thumbnailer interface to interfaces: add thumbnailer interface Aug 29, 2016

interfaces/builtin/thumbnailer_test.go
+ iface: &builtin.BluezInterface{},
+ slot: &interfaces.Slot{
+ SlotInfo: &snap.SlotInfo{
+ Snap: &snap.Info{SuggestedName: "bluez"},
@zyga

zyga Aug 29, 2016

Contributor

This is copy pasted, it would be nice to at least replace those strings.

@zyga
Contributor

zyga commented Aug 29, 2016

What is the indented use case of this interface? It seems to be used on classic (because of the peer=unconfined label. If that is the case did you consider adding this interface to the list of implicit slots added on classic (see snap/implicit.go)
@zyga
Contributor

zyga commented Aug 29, 2016

Please check the README.md for some hints on how to run unit tests. You should also re-format the files you changed with go fmt so that they match the coding standard used by snapd.
@fkaleo

fkaleo commented Aug 29, 2016

Sorry about that: most of my work was missing from the PR due to the missed commit.
About the snap/implicit.go, thank you I was not aware of that. Is there some documentation of what it means?
interfaces/builtin/thumbnailer.go
+type ThumbnailerInterface struct{}
+
+func (iface *ThumbnailerInterface) Name() string {
+ return "thumbnailer"
@morphis

morphis Aug 31, 2016 •
Edited 1 time

Contributor

Can you document this interface in https://github.com/snapcore/snapd/blob/master/docs/interfaces.md?

Also a manual test case to verify the interface is working in https://github.com/snapcore/snapd/blob/master/integration-tests/manual-tests.md or a spread test is required.

@fkaleo

fkaleo Sep 1, 2016 •
Edited 1 time

I added a spread test that queries the thumbnailer service over dbus.

interfaces/builtin/thumbnailer.go
+
+var thumbnailerServiceConnectedPlugAppArmor = []byte(`
+# Description: Allow accessing the thumbnailer service.
+# Usage: reserved
@morphis

morphis Aug 31, 2016

Contributor

Why is the plug reserved as well?

@fkaleo

fkaleo Sep 1, 2016

Removed 'reserved'

interfaces/builtin/thumbnailer.go
+<policy context="default">
+ <deny send_destination="com.canonical.Thumbnailer"/>
+</policy>
+`)
@jhenstridge

jhenstridge Sep 7, 2016

Contributor

I'm a snappy novice, but this looks like the kind of configuration you'd use to expose a service on the system bus.

Thumbnailer is a user service that runs on the session bus and serve requests from processes with the same UID. It definitely isn't intended to be a system service running as root exposed on the system bus.

@morphis

morphis Sep 7, 2016

Contributor

It is. @fkaleo can you clarify what kind of service the thumbnailer is?

@fkaleo

fkaleo Sep 8, 2016

Indeed it sounds like I did it all wrong. Despair.
@jhenstridge are we supposed to have a single instance of the service per user session or is it ok to bundle the thumbnailer service as part of the app and spawn one per app?

@morphis
Contributor

morphis commented Sep 7, 2016

@fkaleo: Can you look at the failing tests?
@fkaleo

fkaleo commented Sep 8, 2016

Just a note: once this interface is good to go, we will be missing:
@niemeyer
Contributor

niemeyer commented Sep 19, 2016

What's the status here?
@niemeyer
Contributor

niemeyer commented Sep 29, 2016

I see activity, but a question 10 days ago that went unanswered, missing files, conflicts, failing tests.

It doesn't look like this should be in review yet. Closing, please reopen when it's golden.

@niemeyer niemeyer closed this Sep 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment