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

rss-bridge: add config option #223148

Merged
merged 4 commits into from
Apr 3, 2024
Merged

Conversation

Mynacol
Copy link
Contributor

@Mynacol Mynacol commented Mar 25, 2023

Description of changes

This allows managing rss-bridge's config with nix. Config options are listed at
https://rss-bridge.github.io/rss-bridge/For_Hosts/Custom_Configuration.html

This change should be backwards compatible, only setting the /var/lib/rss-bridge/config.ini.php symlink if actually using the new config option (similar to the whitelist option).

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

(first contribution, open for improvements)

@Mynacol
Copy link
Contributor Author

Mynacol commented Mar 26, 2023

I forgot to ping @dawidsowa

@Mynacol
Copy link
Contributor Author

Mynacol commented Jun 30, 2023

@dawidsowa Could you have a look at this? The whitelist.txt was recently deprecated, so supporting the configuration options is slightly more important now.

An alternative would be to use environment variables, but the en- and decoding might cause errors.

h7x4
h7x4 previously requested changes Sep 18, 2023
nixos/modules/services/web-apps/rss-bridge.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/rss-bridge.nix Outdated Show resolved Hide resolved
@Mynacol
Copy link
Contributor Author

Mynacol commented Sep 19, 2023

Another idea: Should I move the symlink somewhere into /etc? Would be cleaner, separates config from data.

@quantenzitrone
Copy link
Contributor

Having the config file in /etc would be nice, but i don't see it as necessary, I'd rather have this merged soon, since the current way of whitelisting doesn't work anymore.

@Mynacol
Copy link
Contributor Author

Mynacol commented Feb 6, 2024

After communicating with @dvikan about this, I completely re-wrote this PR to support existing upstream methods. Now we use environment variables, injected through the nginx config. I've now tested this change quite thoroughly and it works nicely so far. The whitelist option is properly removed and aliased, the cache directory continues to be set at ${dataDir}/cache by default.
(If one were to use a literal " in a config name or value that would break the nginx config, so it would be catched by that check as well, anything else should be properly escaped and formatted).

After some time we might consider dropping the pkg patches altogether (that'd break for people that manually created a config.ini.php file).

Maybe @SuperSandro2000 you'd like to look at this as well?

@Mynacol Mynacol requested a review from h7x4 February 6, 2024 23:04
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

the other parts are a bit to complicated for me right now, maybe I look at this again at a later point

nixos/modules/services/web-apps/rss-bridge.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/rss-bridge.nix Outdated Show resolved Hide resolved
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I've mostly added some formatting tips to make this easier to understand for me. Can you squash the review commits into one or two meaningful ones? Then I want to cherry-pick and test this on my instance and if that continues to work, we can merge this.

nixos/modules/services/web-apps/rss-bridge.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/rss-bridge.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/rss-bridge.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/rss-bridge.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/rss-bridge.nix Outdated Show resolved Hide resolved
Mynacol and others added 4 commits March 2, 2024 22:26
This allows managing rss-bridge's config with nix.
It leverages the environment variable way of setting the config options,
introduced quite [some time ago](RSS-Bridge/rss-bridge#2100)
It is the only existing way to set config options independent of the
document root, and upstream is [hesitant](RSS-Bridge/rss-bridge#3842)
to change the config loading methods.

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Preserve the default value for the filecache path, but also allow
modifying it, adapting the tmpfiles rule to create the directory with
the right permissions.

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Prefer setting the whitelisted bridges through the generic configuration
method. Removes the need for a whitelist.txt file.

Preserves backwards compatibility by taking the same values and
essentially just renaming the config option.
@Mynacol
Copy link
Contributor Author

Mynacol commented Mar 2, 2024

Thanks @SuperSandro2000! I rebased all the commits to four quite meaningful ones. The filecache path commit could be additionally squashed.
Please test thoroughly as well to ensure compatibility with existing and new configs 💙

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

my config continues to work like before but it is also pretty simple and just consists of a whitelist

@SuperSandro2000 SuperSandro2000 merged commit 8042af0 into NixOS:master Apr 3, 2024
24 checks passed
@Mynacol Mynacol deleted the rss-bridge-config branch April 4, 2024 09:37
Mynacol added a commit to Mynacol/nixpkgs that referenced this pull request Oct 4, 2024
The rss-bridge service changes introduced in f220178
resp. NixOS#223148 removes the need for
the package patch. This commit removes the patch to ease updating and
maintenance.
Relevant service functionality was also removed (e.g. the setting of
RSSBRIDGE_DATA).

The explicit definition of FileCache.path so users can easily see its
default value and change it, requires to use a freeformType to let users
freely add potentially upcoming config options. This type is restricted
to ini types (although we coerce them to environment variables).
This however makes the list of enabled_bridges impossible. That was
fixed by explicitly introducing this option with a type allowing lists.
The default value however should be unset, which is expressed as `null`,
which further spurred a change in the environment variable generation to
ignore null values (instead of coercing them to an empty string).

A breaking change note was added to highlight this change. A check that
warns users of the not-application of their existing config file is
not easily possible, as people could have only added or changed the
config.ini.php file on the file system without changing a nix variable.
wrbbz pushed a commit to wrbbz/nixpkgs that referenced this pull request Oct 9, 2024
The rss-bridge service changes introduced in f220178
resp. NixOS#223148 removes the need for
the package patch. This commit removes the patch to ease updating and
maintenance.
Relevant service functionality was also removed (e.g. the setting of
RSSBRIDGE_DATA).

The explicit definition of FileCache.path so users can easily see its
default value and change it, requires to use a freeformType to let users
freely add potentially upcoming config options. This type is restricted
to ini types (although we coerce them to environment variables).
This however makes the list of enabled_bridges impossible. That was
fixed by explicitly introducing this option with a type allowing lists.
The default value however should be unset, which is expressed as `null`,
which further spurred a change in the environment variable generation to
ignore null values (instead of coercing them to an empty string).

A breaking change note was added to highlight this change. A check that
warns users of the not-application of their existing config file is
not easily possible, as people could have only added or changed the
config.ini.php file on the file system without changing a nix variable.
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.

5 participants