[MOSIP-37808] Updated DB attributes of MOSIP esignet#1885
Conversation
Signed-off-by: Abhi <abhishek.shankarcs@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughDatabase initialization and deployment scripts are refactored to accept configurable database names and usernames via environment variables and psql parameters, replacing hardcoded identifiers throughout the pipeline. ChangesDatabase Configuration Parameterization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
db_scripts/mosip_esignet/db.sql (1)
9-11:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHardcoded database name breaks parameterized deployments.
COMMENT ON DATABASEand\cstill targetmosip_esignet, so customMOSIP_DB_NAMEruns will fail or modify the wrong DB.Suggested fix
-COMMENT ON DATABASE mosip_esignet IS 'e-Signet related data is stored in this database'; +COMMENT ON DATABASE :"mosipdbname" IS 'e-Signet related data is stored in this database'; -\c mosip_esignet postgres +\c :mosipdbname postgres🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db_scripts/mosip_esignet/db.sql` around lines 9 - 11, The script currently hardcodes the database name in the COMMENT ON DATABASE and psql connect command (\c) targeting "mosip_esignet"; update both uses to reference the deploy-time variable (MOSIP_DB_NAME) instead of the literal string so parameterized deployments connect to and annotate the correct database (replace the literal "mosip_esignet" in the COMMENT ON DATABASE statement and the \c command with the MOSIP_DB_NAME substitution mechanism your SQL runner uses).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@db_scripts/mosip_esignet/deploy.sh`:
- Around line 34-35: The psql invocations (the PGPASSWORD=... psql ... -f
drop_db.sql -v mosipdbname=$MOSIP_DB_NAME and the drop_role.sql call with -v
dbuname=$DB_UNAME, plus other psql calls using -v dbuserpwd=\'$DBUSER_PWD\' )
use unquoted shell expansions which can cause word-splitting or break arguments
and the literal-quoted password is unsafe; fix by wrapping each variable
expansion in double quotes (e.g. use -v mosipdbname="$MOSIP_DB_NAME", -v
dbuname="$DB_UNAME" and pass PGPASSWORD="$SU_USER_PWD" and -v
dbuserpwd="$DBUSER_PWD") so psql receives each value as a single argument and
passwords with spaces or quotes are handled correctly.
- Around line 34-35: After loading the properties via eval, add a fail-fast
validation block that checks required environment variables (at minimum
MOSIP_DB_NAME and DB_UNAME, and also SU_USER_PWD, SU_USER, DB_SERVERIP, DB_PORT,
DEFAULT_DB_NAME) are set and non-empty; if any are missing, print a clear error
to stderr and exit non‑zero before running any psql DROP or CREATE commands
(these checks should be placed immediately after the properties are sourced to
protect the psql invocations that use MOSIP_DB_NAME/DB_UNAME on lines running
psql -f drop_db.sql, drop_role.sql and the subsequent psql calls).
In `@db_scripts/mosip_esignet/role_dbuser.sql`:
- Around line 1-4: The CREATE ROLE statement uses raw psql variable substitution
(:dbuname and :dbuserpwd) which is unsafe for identifiers and literals; update
the statement in role_dbuser.sql to use psql quoting: use :"dbuname" for the
role identifier and :'dbuserpwd' for the password literal, and apply the same
pattern to other files (grants.sql, drop_role.sql, db.sql) wherever unquoted
:var substitutions appear so identifiers use :"name" and string values use
:'name'.
---
Outside diff comments:
In `@db_scripts/mosip_esignet/db.sql`:
- Around line 9-11: The script currently hardcodes the database name in the
COMMENT ON DATABASE and psql connect command (\c) targeting "mosip_esignet";
update both uses to reference the deploy-time variable (MOSIP_DB_NAME) instead
of the literal string so parameterized deployments connect to and annotate the
correct database (replace the literal "mosip_esignet" in the COMMENT ON DATABASE
statement and the \c command with the MOSIP_DB_NAME substitution mechanism your
SQL runner uses).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72e4c2ff-0943-4b06-8f25-ca463fa0f9e2
📒 Files selected for processing (10)
db_scripts/init_values.yamldb_scripts/mosip_esignet/db.sqldb_scripts/mosip_esignet/ddl.sqldb_scripts/mosip_esignet/deploy.propertiesdb_scripts/mosip_esignet/deploy.shdb_scripts/mosip_esignet/dml.sqldb_scripts/mosip_esignet/drop_db.sqldb_scripts/mosip_esignet/drop_role.sqldb_scripts/mosip_esignet/grants.sqldb_scripts/mosip_esignet/role_dbuser.sql
Signed-off-by: Abhishek S <127825992+abhishek8shankar@users.noreply.github.com>
Summary by CodeRabbit