refactor: env-var overrides, error handling, and cleanup for testability#130
Closed
amitb0ra wants to merge 1 commit intoRocketChat:masterfrom
Closed
refactor: env-var overrides, error handling, and cleanup for testability#130amitb0ra wants to merge 1 commit intoRocketChat:masterfrom
amitb0ra wants to merge 1 commit intoRocketChat:masterfrom
Conversation
- app.js: env-var path overrides (LEADERBOARD_CONFIG_PATH, LEADERBOARD_DATA_PATH, LEADERBOARD_LOG_PATH, LEADERBOARD_ADMINDATA_PATH, LEADERBOARD_CONFIG_BACKUP_PATH, LEADERBOARD_PORT), LEADERBOARD_SKIP_REFRESH to disable child process, pass res to Util.post calls, fix hardcoded ./admindata.json in login handler, export server - Util.js: post(req, res, callback) — respond 400 on malformed JSON instead of silently hanging, move findContributor from app.js - API.js: remove process.exit() on bad credentials — log error and return instead, support LEADERBOARD_CONFIG_PATH env var - Remove dead .dockerignore (Docker support removed previously) - Remove stale tests/access.sh and tests/api-call-tests.sh (brute-force scripts hitting dead external URLs)
Sing-Li
requested changes
Mar 26, 2026
Member
Sing-Li
left a comment
There was a problem hiding this comment.
It is a pity to add ENV var JUST TO SUPPORT TESTING.
Please look into the npm .dotenv library and make this PR a support for .env files during deployment. (in addition to just support testing)
I am totally against combining code generator suggested hallucinations into one PR. For example, the fix in the callback() JSON parse is simply nonsense.
Please remove all other fixes or mods that has nothing to do with supporting dotenv, then test to make sure .env files are actually supported. Thanks.
Member
|
closing this since you didn't reuse it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extracts server-side bug fixes, cleanup, and env-var overrides into a standalone PR
Changes
app.jsLEADERBOARD_CONFIG_PATH,LEADERBOARD_DATA_PATH,LEADERBOARD_LOG_PATH,LEADERBOARD_ADMINDATA_PATH,LEADERBOARD_CONFIG_BACKUP_PATH,LEADERBOARD_PORTLEADERBOARD_SKIP_REFRESH=1— skip spawningrefresh.jschild process (useful for testing and dry-run setups)restoUtil.post()calls — enables proper error responses on malformed JSON (see Util.js below)./admindata.jsonin login handler → use theadmindataPathvariable (was already defined but not used in one spot)findContributorfrom inline function toUtil.findContributor(deduplication)module.exports = { server }— export the HTTP server for programmatic useUtil.jspost(req, res, callback)— respond with400 { message: 'Invalid JSON body' }on malformed POST body instead of silently hanging (same issue identified in fix: add log / output message notifying operator when Github token has expired #126 by @srijnabhargav)findContributor()— moved here fromapp.jsAPI.jsprocess.exit()on bad GitHub credentials — log the error and return instead (prevents the entire server from crashing on a single bad token check)LEADERBOARD_CONFIG_PATHenv var for config resolutionCleanup
.dockerignore(Docker support was removed in a previous PR)tests/access.shandtests/api-call-tests.sh(brute-force stress scripts hitting a defunct external URL)Merge order