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

miniflux: add service #56719

Merged
merged 1 commit into from Apr 12, 2019
Merged

miniflux: add service #56719

merged 1 commit into from Apr 12, 2019

Conversation

bricewge
Copy link
Contributor

@bricewge bricewge commented Mar 2, 2019

Motivation for this change

This is an update for the PR #52638 written by @nornagon to add a service for miniflux. It amend the initial PR with the remarks made by @infinisil and two config options to specify the initial admin account.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

nixos/modules/services/web-apps/miniflux.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/miniflux.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/miniflux.nix Outdated Show resolved Hide resolved
@bricewge
Copy link
Contributor Author

bricewge commented Mar 9, 2019

Can I get a new review since I think I addressed all the comments?

nixos/modules/services/web-apps/miniflux.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/miniflux.nix Outdated Show resolved Hide resolved
@bricewge bricewge force-pushed the miniflux-service branch 2 times, most recently from fa1591a to cd0397c Compare March 12, 2019 10:19
@bricewge bricewge force-pushed the miniflux-service branch 2 times, most recently from c3317f9 to 22bfbf3 Compare March 12, 2019 13:54
@andir
Copy link
Member

andir commented Mar 24, 2019

Overall this looks very good.

Could we get a NixOS VM test as well? :-)

@bricewge
Copy link
Contributor Author

I have added the VM test.

However it currently fail from what I think is a bug in systemd; for the test to be working you need to comment out DynamicUser = true;.

Let's follow the manual crumbs:

If DynamicUser= is enabled, RemoveIPC=, PrivateTmp= are implied.
---systemd.exec(5)

So with DynamicUser= enabled we also have PrivateTmp=.

If the executable path is prefixed with "+" then the process is executed with
full privileges. In this mode privilege restrictions configured with User=,
Group=, CapabilityBoundingSet= or the various file system namespacing options
(such as PrivateDevices=, PrivateTmp=) are not applied to the invoked command
line (but still affect any other ExecStart=, ExecStop=, … lines).
---systemd.service(5)

But since we prefixed the path for ExecStartPre= the pre-start script should not be affected by PrivateTmp=, unfortunately it looks like it's not the case. Here is the log of the failing test and here the log of the not failing one (without DynamicUser=).

Do you think it is a systemd bug? Could this PR be merged by using a specific user/group?

@flokli
Copy link
Contributor

flokli commented Mar 31, 2019

@bricewge would #57677 help (as you don't need to access postgresql through the socket in /tmp), or declaring databases and users through what's suggested in #56720 ?

@bricewge
Copy link
Contributor Author

bricewge commented Apr 1, 2019

@flokli Rebasing on master seems to have fixed it, so maybe #57677 helped.
The test pass on my machine.

nixos/modules/services/web-apps/miniflux.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/miniflux.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/miniflux.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/miniflux.nix Show resolved Hide resolved
@bricewge
Copy link
Contributor Author

bricewge commented Apr 6, 2019

I have updated the PR to stay more in line with NixOS/rfcs#42, options have been removed and you can now fully configure miniflux.

Note that the default credentials will be in the store, it doesn't looks like a great deal since it is public knowledge.

Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

LGTM. @andir ?

@joachifm
Copy link
Contributor

@GrahamcOfBorg test miniflux

@joachifm joachifm merged commit 5dafbb2 into NixOS:master Apr 12, 2019
@joachifm joachifm mentioned this pull request Apr 12, 2019
10 tasks
@nornagon
Copy link
Contributor

Thanks for pushing this through to completion @bricewge!

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

Successfully merging this pull request may close these issues.

None yet

7 participants