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

[BridgeFactory.php] Add RSSBRIDGE_WHITELIST environment variable support #2058

Closed
wants to merge 1 commit into from

Conversation

gileri
Copy link
Contributor

@gileri gileri commented Apr 8, 2021

Enabling/disabling bridges via the whitelist.txt file can be cumbersome in some setups, for example in containers. This PR introduces a RSSBRIDGE_WHITELIST environment variable that accepts a wildcard (*) or bridges names separated by a comma instead of newlines.

After merging, the Whitelisting and Docker will need to be updated.

Edit : I'm of course open to modifications regarding the environment variable name or other changes.

@em92 em92 mentioned this pull request May 3, 2021
9 tasks
@Bockiii
Copy link
Contributor

Bockiii commented Jun 28, 2021

This is integrated in #2100

@gileri
Copy link
Contributor Author

gileri commented Jun 29, 2021

Your PR looks more complete, I'll decline mine if yours it gets merged.

@dvikan
Copy link
Contributor

dvikan commented Mar 24, 2022

Fixed by #2100

@dvikan dvikan closed this Mar 24, 2022
@ikajdan
Copy link

ikajdan commented Jun 12, 2022

Fixed by #2100

Was it really? Seems like the whitelist config got eventually removed from that PR by this commit: 99d6203.

@ikajdan
Copy link

ikajdan commented Jul 10, 2022

@gileri, can we get this reopened?

@dvikan dvikan reopened this Jul 10, 2022
@dvikan
Copy link
Contributor

dvikan commented Jul 10, 2022

I can see the advantage in this pr. But the container/environment still must be restarted for new env vars to take effect.

An alternative:

diff --git a/config.default.ini.php b/config.default.ini.php
index c1627aac..ae96d108 100644
--- a/config.default.ini.php
+++ b/config.default.ini.php
@@ -12,6 +12,8 @@
 ; timezone = "UTC" (default)
 timezone = "UTC"
 
+enabled_bridges = Gettr Vimeo
+

@Bockiii
Copy link
Contributor

Bockiii commented Jul 11, 2022

If we add it to the config, wouldn't that make it more confusing as to where to actually configure the whitelisted bridges? because then we would have the whitelist.txt and also the config.

One of the reasons for doing it through the environment variable was also to make it easier to deploy to heroku et al.

I dont have a strong feeling either way, but we should come up with a strategy first before we add it to the config file.

@dvikan
Copy link
Contributor

dvikan commented Jul 31, 2022

I think we should deprecate the usage of whitelist.txt and move it into a config value enabled_bridges and also en env value RSSBRIDGE_ENABLED_BRIDGES or perhaps RSSBRIDGE_ENABLEDBRIDGES.

@dvikan
Copy link
Contributor

dvikan commented May 4, 2023

After having learnt about arrays in ini files it should be done like this:

enabled_bridges[] = TwitterBridge                                                                                       
enabled_bridges[] = YoutubeBridge                                                                                          
enabled_bridges[] = FooBridge

For the env var, they cant be arrays. They are strings. Not sure how to handle that. Maybe space separated:

RSSBRIDGE_ENABLED_BRIDGES="TwitterBridge YoutubeBridge FooBridge"

@Bockiii
Copy link
Contributor

Bockiii commented May 4, 2023

So do you want 3 options?

whitelist.txt
enabled_bridges config array in config.ini
RSSBRIDGE_WHITELIST env variable?

Do you want them to all add up?

@dvikan
Copy link
Contributor

dvikan commented May 5, 2023

Yeah I think all config should be in .ini file. And whitelist.txt and env overshadows for convenience.

@Bockiii
Copy link
Contributor

Bockiii commented May 5, 2023

overshadow meaning replace? or just add.

So if I have a whitelist with BridgeA, an ini selection of BridgeB and BridgeC and an env variable of BridgeA and BridgeD, should A B C and D be selected?

@dvikan
Copy link
Contributor

dvikan commented May 6, 2023

Had not thought about that. I mean replace yes.

@Bockiii
Copy link
Contributor

Bockiii commented May 6, 2023

So in my example, only A and D would be selected since B and C are only in the .ini and you want that replaced?

I think thats very complicated. A simple addition would make more sense for the users I think.

Its also how most other things work. Access rights are usually additive and in this case I would see it as "bridges a user has access to".

@dvikan
Copy link
Contributor

dvikan commented May 7, 2023

What you think @gileri ?

@dvikan
Copy link
Contributor

dvikan commented Jul 2, 2023

fixed in #3428

@gileri

@dvikan dvikan closed this Jul 2, 2023
@gileri
Copy link
Contributor Author

gileri commented Jul 2, 2023

Sorry for the delayed answer. Had a look over #3428, seems perfect. Thank you @dvikan, and @Bockiii for the review !

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

Successfully merging this pull request may close these issues.

4 participants