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

Workaround for Admins list in Environment Variables by specifying singular admin #4228

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

Inumedia
Copy link
Contributor

Description

This allows working around an issue with Viper where it doesn't properly unmarshal GetIntSlice from environment variables. Seems like this is a known issue and is quite old. I'm assuming there isn't plans to fix it on the Viper side.

spf13/viper#339

Tests

I'll be brutally honest, I don't know how to run Go, but this seemed like a pretty quick low hanging fix.

@CLAassistant
Copy link

CLAassistant commented Nov 28, 2024

CLA assistant check
All committers have signed the CLA.

@ua741
Copy link
Member

ua741 commented Nov 28, 2024

Hello,
This would break the behaviour for people who have multiple admins. Is there a specific reason why can't use the config file to list the admin ids?

@Inumedia
Copy link
Contributor Author

It doesn't alter the admins list functionality, and instead uses a different property admin vs admins.

It's purely intended as a work-around when using environment variables instead of a config file.

For context, in my environment, I prefer to primarily use environment variables and not have config files laying around. Most of the environment variables work except for this one.

Maybe the pull request should be renamed to clarify

@Inumedia Inumedia changed the title Add support for specifying a singular admin id instead of a list Workaround for Admins list in Environment Variables by specifying singular admin Nov 28, 2024
Copy link
Member

@mnvr mnvr left a comment

Choose a reason for hiding this comment

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

Not very keen on bloating up the configuration with one-off workarounds, esp for cases which could be worked around in other ways (e.g. by using the first-user-as-admin functionality). Since you've already put in the effort, will merge this, but going forward please keep feature requests to discussions instead of direct code changes.

(will add the documentation for this new option in a separate PR)

@mnvr mnvr merged commit 4d761f9 into ente-io:main Nov 28, 2024
1 of 2 checks passed
mnvr added a commit that referenced this pull request Nov 28, 2024
@Inumedia Inumedia deleted the patch-1 branch November 28, 2024 10:21
mnvr added a commit that referenced this pull request Nov 28, 2024
Fix code that didn't even compile, + document
#4228
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