nixos/fresh-rss: init module #77354

aanderse · 2020-01-09T02:44:35Z

Motivation for this change

Things done

Notify maintainers

schmittlauch

@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.

schmittlauch · 2020-01-09T11:21:02Z

nixos/modules/services/web-apps/fresh-rss.nix

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


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?

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

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

schmittlauch · 2020-01-09T11:28:46Z

nixos/modules/services/web-apps/fresh-rss.nix

+    };
+
+    services.httpd = {
+      enable = true;


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.

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.

schmittlauch · 2020-01-09T11:32:51Z

pkgs/servers/web-apps/fresh-rss/default.nix

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


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?

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

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.

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 · 2020-01-09T12:06:28Z

@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.

makefu · 2020-03-10T22:13:15Z

Hey! I'd love to see this module being included into nixpkgs. Right now we "only" have tt-rss which has a couple of shortcomings and freshrss has a lot of potential.
Cheers!

aanderse · 2020-03-10T22:42:25Z

@makefu any interest in picking this up? I don't use fresh-rss so this PR is lower on my priorty list.

makefu · 2020-03-11T07:03:07Z

@aanderse low prio is fine for me, i will use ttrss until the time comes :) but thanks for even starting the PR!

stale · 2020-09-07T07:39:18Z

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

ambroisie · 2021-02-05T16:34:31Z

I am also interested in this MR, can I know what's left to be done?

aanderse · 2021-02-05T16:37:32Z

I don't think many people use httpd so someone should probably port it to nginx. Aside from that... probably just testing?

I don't use this software and have very little interest in finishing this off anymore. Competing priorities, etc... Are you interested in taking this over?

ambroisie · 2021-02-05T16:42:52Z

@aanderse I'm quite new to Nix, and have never packaged anything for nixpkgs.

I could probably port it to nginx, but I'm trying to understand why the service itself should be taking care of this, instead of being left to the user ?

I assume testing is mostly making sure the derivation builds, and probably also running correctly.

aanderse · 2021-02-05T16:45:52Z

The package is probably good to go. The module does a fair bit of work for the user. We could just ship a package without a module...

aanderse · 2021-03-19T19:43:06Z

If anyone wants to finish this off please feel free to take over.

aanderse added 2 commits January 8, 2020 21:41

fresh-rss: init at 1.15.3

9af8085

nixos/fresh-rss: init module

577919b

ofborg bot added 6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jan 9, 2020

schmittlauch reviewed Jan 9, 2020

View reviewed changes

stale bot added the 2.status: stale label Sep 7, 2020

ryantm added 2.status: merge conflict and removed 2.status: merge conflict labels Oct 3, 2020

stale bot removed the 2.status: stale label Oct 3, 2020

ryantm added the 2.status: merge conflict label Oct 3, 2020

aanderse closed this Mar 19, 2021

aanderse deleted the fresh-rss branch March 19, 2021 19:43

ajs124 mentioned this pull request Aug 9, 2021

freshrss: init at 1.18.1, tests and module #133035

Closed

11 tasks

Stunkymonkey mentioned this pull request Sep 7, 2022

freshrss: init at 1.20.0, tests and module #190172

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/fresh-rss: init module #77354

nixos/fresh-rss: init module #77354

aanderse commented Jan 9, 2020

schmittlauch left a comment

schmittlauch Jan 9, 2020

aanderse Jan 9, 2020

schmittlauch Jan 9, 2020 •

edited

Loading

schmittlauch Jan 9, 2020

aanderse Jan 9, 2020

schmittlauch Jan 9, 2020

Mic92 Jan 9, 2020

aanderse Jan 9, 2020

schmittlauch Jan 9, 2020 •

edited

Loading

aanderse commented Jan 9, 2020

makefu commented Mar 10, 2020

aanderse commented Mar 10, 2020

makefu commented Mar 11, 2020

stale bot commented Sep 7, 2020

ambroisie commented Feb 5, 2021

aanderse commented Feb 5, 2021

ambroisie commented Feb 5, 2021

aanderse commented Feb 5, 2021

aanderse commented Mar 19, 2021

nixos/fresh-rss: init module #77354

nixos/fresh-rss: init module #77354

Conversation

aanderse commented Jan 9, 2020

Motivation for this change

Things done

Notify maintainers

schmittlauch left a comment

Choose a reason for hiding this comment

schmittlauch Jan 9, 2020

Choose a reason for hiding this comment

aanderse Jan 9, 2020

Choose a reason for hiding this comment

schmittlauch Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

schmittlauch Jan 9, 2020

Choose a reason for hiding this comment

aanderse Jan 9, 2020

Choose a reason for hiding this comment

schmittlauch Jan 9, 2020

Choose a reason for hiding this comment

Mic92 Jan 9, 2020

Choose a reason for hiding this comment

aanderse Jan 9, 2020

Choose a reason for hiding this comment

schmittlauch Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

aanderse commented Jan 9, 2020

makefu commented Mar 10, 2020

aanderse commented Mar 10, 2020

makefu commented Mar 11, 2020

stale bot commented Sep 7, 2020

ambroisie commented Feb 5, 2021

aanderse commented Feb 5, 2021

ambroisie commented Feb 5, 2021

aanderse commented Feb 5, 2021

aanderse commented Mar 19, 2021

schmittlauch Jan 9, 2020 •

edited

Loading

schmittlauch Jan 9, 2020 •

edited

Loading