-
Notifications
You must be signed in to change notification settings - Fork 252
Allow configuration via files specified via _FILE environment variables
#137
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
base: main
Are you sure you want to change the base?
Conversation
ea6f540 to
cdcf441
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR centralizes configuration management into a new config.js module and adds support for Docker-style _FILE environment variables (e.g., DATA_PATH_FILE) for reading configuration from mounted secrets/configs. This is a common pattern in containerized environments.
Key Changes:
- New
config.jsmodule withreadEnv()function that checks_FILEvariants first before falling back to regular environment variables - Refactored all modules to use centralized configuration instead of reading
process.envdirectly - Added
VOLUMEdirective to Dockerfile for the data path
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| standalone/src/config.js | New centralized configuration module with _FILE environment variable support |
| standalone/src/siteverify.js | Updated to use centralized config; fixed typo in error message |
| standalone/src/ratelimit.js | Updated to use centralized config for rate limiting settings |
| standalone/src/index.js | Updated to use centralized config for server settings |
| standalone/src/db.js | Simplified to use centralized config for database configuration |
| standalone/src/auth.js | Updated to use centralized config for admin key |
| standalone/src/cap.js | Updated to use centralized config; optimized challenge data parsing |
| standalone/src/assets.js | Updated to use centralized config for asset server settings |
| standalone/Dockerfile | Added VOLUME declaration and quoted environment variable references |
Comments suppressed due to low confidence (2)
standalone/src/assets.js:11
- Operands config. ... latest" and config. ... latest" are identical.
(config.assetsServer.versions.widget === "latest" ||
config.assetsServer.versions.widget === "latest")
standalone/src/config.js:21
- Avoid automated semicolon insertion (93% of all statements in the enclosing script have an explicit semicolon).
const dbUrl = await readEnv("DB_URL") || `sqlite://${path.join(dataPath, "db.sqlite")}`
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cdcf441 to
a15f6a9
Compare
Using Docker with Configs and Secrets (or even Kubernetes), it's quite common to mount them as files into the container. This requires that the application is able to also deal with files as configuration source, which is commonly solved using the same environment variable with a
_FILEsuffix (e.g.DATA_DIR_FILE) that points to a file that contains the value.The
_FILEvariant is checked first and if it's not set, the regular variable is read.This PR refactors the configuration of the standalone server to be centralized into a new
config.jsthat reads all config variables - also supporting file variables.Also, a
VOLUMEwas added to the Dockerfile for the data path.This is based based on #135 which is why I marked this as draft.