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

Allow specifying configuration params in env vars #83

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

djmitche
Copy link
Collaborator

@djmitche djmitche commented Feb 1, 2025

Fixes #73.

This updates the docker-compose to use the new format as well.

@djmitche djmitche requested a review from ryneeverett February 1, 2025 22:09
Copy link

@ryneeverett ryneeverett left a comment

Choose a reason for hiding this comment

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

Since we already broke cli backwards compatibility in #81, could we consider removing --allow-client-id as a command line argument? If we're treating it as a credential then we shouldn't encourage passing it this way.

Comment on lines +270 to +271
assert_eq!(*matches.get_one::<i64>("snapshot-days").unwrap(), 13i64);
assert_eq!(*matches.get_one::<u32>("snapshot-versions").unwrap(), 20u32);

Choose a reason for hiding this comment

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

Seems odd that these end up with such different types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember if I had a reason for that, but I don't think it's too important.

README.md Outdated

By default, the server allows all client IDs. To limit the accepted client IDs,
such as when running a personal server, use `--allow-client-id <client-id>`.
This value can be specified in the environment variable `CLIENT_ID`, as a
comma-separated list of client IDs.

Choose a reason for hiding this comment

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

Might be nice to mention that the client id should be treated as a credential since it may not be obvious that the client id is the only authentication.

@djmitche
Copy link
Collaborator Author

djmitche commented Feb 3, 2025

Since we already broke cli backwards compatibility in #81, could we consider removing --allow-client-id as a command line argument? If we're treating it as a credential then we shouldn't encourage passing it this way.

I mentioned it in the README. But, I don't think this is a critical distinction in a lot of deployments, which are running on a VPS or some other sort of single-user system.

@djmitche djmitche merged commit 7f51d2f into GothenburgBitFactory:main Feb 3, 2025
5 checks passed
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.

Allow specifying config via a env vars
2 participants