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

nixos/fresh-rss: init module #77354

Draft
wants to merge 2 commits into
base: master
from
Draft

nixos/fresh-rss: init module #77354

wants to merge 2 commits into from

Conversation

@aanderse
Copy link
Contributor

aanderse commented Jan 9, 2020

Motivation for this change

#77246

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @schmittlauch

Copy link
Contributor

schmittlauch left a comment

@aanderse First of all thank you for taking care of this so quickly.
These are my remarks after reading through your code without trying out the module so far.
Please also note that I'm relatively new to writing NixOS modules, so take my comments with a grain of salt.

type = types.bool;
default = true;
description = ''
Create the database and database user locally.

This comment has been minimized.

Copy link
@schmittlauch

schmittlauch Jan 9, 2020

Contributor

To me this description doesn't make completely clear what this option is about. Together with the assertion below I assume that this is used to create a mysql db on the same machine with socket authentication?

This comment has been minimized.

Copy link
@aanderse

aanderse Jan 9, 2020

Author Contributor

Thanks for this feedback. You are correct. Any suggestions?

This comment has been minimized.

Copy link
@schmittlauch

schmittlauch Jan 9, 2020

Contributor
localDB = mkOption {
  type = types.bool;
  default = true;
  description = ''
    Create and use a database on the same machine. This allows autheticating to the db without additional credentials.
    This currently only applies if database type "mysql" is selected.
  '';
};
};

services.httpd = {
enable = true;

This comment has been minimized.

Copy link
@schmittlauch

schmittlauch Jan 9, 2020

Contributor

So it looks like you always activate Apache httpd and use fresh-rss behind it. While this is definitely convenient for quickly getting a full RSS reader service running, it takes away the flexibility.
Other modules like gitea or tt-rss either don't care about the HTTP webserver in front of them at all or do supply a virtual host config for a web server of their choice (in the case of tt-rss it's nginx), but don't enable it by default.
This makes it possible for users to supply an alternative web server frontend.

This comment has been minimized.

Copy link
@aanderse

aanderse Jan 9, 2020

Author Contributor

Some modules only provide nginx configuration, some only provide `httpd. Some provide neither. I find this situation regrettable and hope for a day where sysadmins can easily choose and modules support either option.


phpConfig = writeText "constants.local.php" ''
<?php
safe_define('DATA_PATH', '/var/lib/fresh-rss');

This comment has been minimized.

Copy link
@schmittlauch

schmittlauch Jan 9, 2020

Contributor

It'd be nice to tie this together with the stateDir variable defined in the module. Or is there a need to change the directories independently from each other in the package and module?

This comment has been minimized.

Copy link
@Mic92

Mic92 Jan 9, 2020

Contributor

It is not really configurable in the module either so this should be fine.

This comment has been minimized.

Copy link
@aanderse

aanderse Jan 9, 2020

Author Contributor

My intention is to leave this hardcoded. Whether a module has a configurable state directory or not is left up to the discretion of the module author. It is more work to make the state directory configurable, so I went with not.

This comment has been minimized.

Copy link
@schmittlauch

schmittlauch Jan 9, 2020

Contributor

It's just always a potential source of issues if you define the same constant at multiple places and then forget to change both at some point.
But yeah, this is just a minor point.

@aanderse

This comment has been minimized.

Copy link
Contributor Author

aanderse commented Jan 9, 2020

@schmittlauch people who aren't as familiar with a technology often times provide the most valuable feedback as they have fresh eyes and are able to see shortcomings others can't 😄

All feedback welcome. Thanks.

Note this is still a work in progress, though. I've done very minimal testing and I'm not convinced this works reliably yet.

More to come.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.