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

Conversation

Projects
None yet
7 participants
@bricewge
Copy link
Contributor

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.

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 Outdated

@bricewge bricewge force-pushed the bricewge:miniflux-service branch from 1aef1e3 to 2ab021c Mar 9, 2019

@bricewge

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

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

Show resolved Hide resolved nixos/modules/services/web-apps/miniflux.nix Outdated
Show resolved Hide resolved nixos/modules/services/web-apps/miniflux.nix Outdated

@bricewge bricewge force-pushed the bricewge:miniflux-service branch 2 times, most recently from fa1591a to cd0397c Mar 12, 2019

@bricewge bricewge force-pushed the bricewge:miniflux-service branch 2 times, most recently from c3317f9 to 22bfbf3 Mar 12, 2019

@bricewge bricewge force-pushed the bricewge:miniflux-service branch from 22bfbf3 to cd1de42 Mar 24, 2019

@andir

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

Overall this looks very good.

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

@bricewge bricewge force-pushed the bricewge:miniflux-service branch from cd1de42 to f2924ee Mar 28, 2019

@bricewge

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Contributor

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 bricewge force-pushed the bricewge:miniflux-service branch from f2924ee to b8bd49a Apr 1, 2019

@bricewge

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

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

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 Outdated
Show resolved Hide resolved nixos/modules/services/web-apps/miniflux.nix

@bricewge bricewge force-pushed the bricewge:miniflux-service branch from b8bd49a to 5e9570b Apr 5, 2019

@bricewge bricewge force-pushed the bricewge:miniflux-service branch from 5e9570b to e8b68dd Apr 6, 2019

@bricewge

This comment has been minimized.

Copy link
Contributor Author

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.

@flokli

flokli approved these changes Apr 7, 2019

Copy link
Contributor

left a comment

LGTM. @andir ?

@joachifm

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

@GrahamcOfBorg test miniflux

@joachifm joachifm merged commit 5dafbb2 into NixOS:master Apr 12, 2019

13 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
tests.miniflux on aarch64-linux Success
Details
tests.miniflux on x86_64-linux Success
Details

@joachifm joachifm referenced this pull request Apr 12, 2019

Closed

miniflux: add service #52638

3 of 10 tasks complete
@nornagon

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

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
You can’t perform that action at this time.