zancarius / archlinux-pkgbuilds

Various odds and ends but mostly just custom PKGBUILDs.
8 stars 4 forks source link

Sentry package should use sysusers.d? #11

Closed zancarius closed 7 years ago

zancarius commented 7 years ago

I'd like to investigate using sysusers.d for Sentry's user/group management since the current method of including it in the sentry.install upgrade/install/uninstall sections is a bit hackish (it's appropriate for Arch but I want to migrate away from this eventually). The AUR package for Mattermost uses it but I have no idea how this would affect package installation/uninstallation.

mitchhentges commented 7 years ago

IIRC, you need to restart systemd-sysusers after inserting into sysusers.d for the changes to take effect. Fortunately, this would easily be backwards-compatible, due to the u flag only adding a user if it doesn't already exist - not that it's a big deal, but convenient. (slightly) interstingly, if you remove the user from sysusers.d, the user isn't removed from /etc/passwd, and would have to be manually removed

zancarius commented 7 years ago

Hey, thanks. I forgot about this actually (not surprising, I know).

Might look into implementing this at some point soon. Probably by the next minor version bump from upstream?

mitchhentges commented 7 years ago

That would be interesting! I'm setting up continuous deliver of some of my small projects to my linux server, and keep coming back to your Sentry config files for inspiration and direction. I'm just glad you found sysusers.d, I was definitely just going to slap 'em in /etc/passwd, :chicken:

zancarius commented 7 years ago

Hah, thanks. At least it's of use to someone!

Truth be told, I've been meaning to implement this for a while (apparently since November of last year--sorry about that!). I want to say it's because I'm lazy (also true), but in actuality I just ended up forgetting. Maybe it's some mix of both.

Now that you've reminded me, I'll get this situated tonight or tomorrow and then push it through whenever Sentry's next update comes down the pipeline. Expect to see something appear in the next 24-48 hours as I'll reference the commit here in case you wish to test it out and offer up some changes/corrections--or at least for an extra set of eyes to make sure nothing's missing.

zancarius commented 7 years ago

It's available here for now (plus a couple other commits since I'm an idiot).

Problem is, I don't really like restarting services from within the post-install scripts, but it's the only way to pick up the changes for chown. I guess there's the possibility of using alpm-hooks(5) but that seems an awful lot like overkill, particularly since the intent behind hooks is to run helper tools that may be included (or needed) by the PKGBUILD. post-install should be OK for the time being.

zancarius commented 7 years ago

I should note that instead of restarting systemd-sysusers, it'll call /usr/bin/systemd-sysusers directly since that's how the script is intended to run (more or less) and that's all the service file does anyway.

mitchhentges commented 7 years ago

Err, I think that your link might be to the wrong location @zancarius :wink:

zancarius commented 7 years ago

Shit.

Firefox hates copy/paste because I abuse it with about 800+ tabs (actually "only" 458 at the moment). Thanks for catching that. It's fixed.

I love it when I write a comment, tab over or forget about it for a while, re-read it for obvious errors/omissions/mistakes, and then post it without actually thinking about the content, assuming instead that it's correct.

I need to note that there's a series of commits in that branch, and that I initially restarted the service before I realized that systemd-sysusers is intended to be used directly. It still feels hackish, but that's just the nature of things.

Also, I added a note on the post-remove as you mentioned earlier. There was never any indication that the Sentry user would be left over following an uninstall. It's probably the polite thing to do.

mitchhentges commented 7 years ago

Hmm, I haven't (ever) worked on an AUR package before, so I'm not super knowledge-ful here. However, here you install sentry-sysusers.conf, as well as a bunch of other archlinux-pkgbuild/sentry/ files. However, sentry-sysusers.conf doesn't exist yet. Perhaps that's deliberate? I'm probably missing something obvious :chicken:

zancarius commented 7 years ago

That's actually an omission on my part. I neglected to push that particular changeset to GitHub.

It turns out that my PKGBUILDs dir is somewhat convoluted. Each PKGBUILD has its own .git repo pointing to the Arch upstream, so I have to be cautious which directory I'm in when I add/commit files (and sometimes miss things). Hence why it's good to do this on a branch before doing something stupid, like I did in this case.

Should be addressed in b0c0de2. You're not going crazy, I just neglected to push the appropriate file, because I accidentally committed it in the AUR repo's directory, not the one that was getting pushed to GitHub, etc. (hence thinking it was already in here).

mitchhentges commented 7 years ago

lgtm to me :+1: Thanks for looking into this!