Skip to content
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

Add system configuration for the Kolibri dbus daemon #1

Merged
merged 1 commit into from
Jan 22, 2021

Conversation

dylanmccall
Copy link
Contributor

@dylanmccall dylanmccall commented Jan 7, 2021

This depends on changes to Kolibri pending review in flathub/org.learningequality.Kolibri#17, but I believe it is safe to merge at any time.

https://phabricator.endlessm.com/T31182

@dylanmccall
Copy link
Contributor Author

dylanmccall commented Jan 12, 2021

Things to test here:

  • A "kolibri" system user is created. This is both for upgrading Endless OS, and for new installs. (That isn't really changed in this branch, but we never actually had this stuff in a release so it's worth making sure).
  • The newest (unreleased) version of the Kolibri flatpak app works, with the desktop application talking to a Kolibri instance whose home directory is located at /var/lib/kolibri/data.
  • Using the old (released) version of the Kolibri flatpak will cause the provided dbus service to be non-functional, but will otherwise not break anything.

@erikos erikos requested a review from danigm January 18, 2021 17:41
@dylanmccall
Copy link
Contributor Author

dylanmccall commented Jan 18, 2021

To test this, we need an early version of the Kolibri flatpak from flathub/org.learningequality.Kolibri#17. You can install it from the Flathub testing repository:

flatpak install --system https://dl.flathub.org/build-repo/35974/org.learningequality.Kolibri.flatpakref

@dylanmccall dylanmccall marked this pull request as ready for review January 19, 2021 03:49
Copy link

@jprvita jprvita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole D-Bus + systemd approach looks correct and sensible to me. The only question I had is about the KOLIBRI_USER management, but it looks like we already have a user named kolibri in the system, so we are good there. Otherwise I would suggest for us to use systemd's DynamicUser= setting instead.

I don't know Kolibri too deeply, so take my CR+ with a grain of salt.

Copy link
Contributor

@manuq manuq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me as well!

@dylanmccall
Copy link
Contributor Author

Hm, now you mention it, I do wonder why we aren't using DynamicUser. I vaguely recall discussing that when we did eos-kolibri originally, but can't find an email about it so I might be imagining. I'll try that and see what happens, but perhaps in a separate branch :)

@jprvita
Copy link

jprvita commented Jan 22, 2021

Hm, now you mention it, I do wonder why we aren't using DynamicUser. I vaguely recall discussing that when we did eos-kolibri originally, but can't find an email about it so I might be imagining. I'll try that and see what happens, but perhaps in a separate branch :)

If we were to switch it now, we'd have to drop the created kolibri user by reverting endlessm/base-passwd@c12a433. The only problem I can think from the top of my head is that if there are any files owned by that user, they would need to be dealt with (since they would have the old uid/gid). I don't know from the top of my head how DynamicUser deals with file ownership.

@dylanmccall
Copy link
Contributor Author

dylanmccall commented Jan 22, 2021

Ah, happily DynamicUser has migrations like that in mind and will behave as usual if there's an existing kolibri user. Alas, there is one other (rather serious) issue, but I made a draft pull request to explore the idea: #2.

@dylanmccall dylanmccall merged commit 1053e60 into master Jan 22, 2021
@jprvita jprvita deleted the T31182-dbus-system-service branch February 4, 2021 23:18
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants