[MOSIP-43894, MOSIP-43640, MOSIP-44014, DSD-9381] Port release-1.3.x …#141
Open
ckm007 wants to merge 9 commits into
Open
[MOSIP-43894, MOSIP-43640, MOSIP-44014, DSD-9381] Port release-1.3.x …#141ckm007 wants to merge 9 commits into
ckm007 wants to merge 9 commits into
Conversation
…improvements to develop postgres-upgrade: - Dockerfile: add COPY for otp-manager and digital-card-service db_upgrade_scripts - repo-list.txt: fix resident -> resident-services, add digital-card-service and otp-manager - upgrade.csv: append 1.2.x -> 1.3.0 upgrade paths alongside existing 1.1.x -> 1.2.x paths CI workflow: - push-trigger.yml: add MOSIP* branch trigger; add get-db-upgrade-scripts and build-upgrade-dockers jobs to build postgres-upgrade docker image on push helm/postgres-upgrade: - postgres-upgrade-job.yaml: fix indentation and env var quoting (MOSIP-44014) - values.yaml: update image from mosipdev:release-1.2.0.1 to mosipqa:develop deploy: - postgres-install.sh: add PostgreSQL image override, ISTIO_ADDONS_CHART_VERSION, switch istio-addons from local chart to mosip/istio-addons helm repo chart - postgres-istio-addons-values.yaml: new istio config for postgres - masterdata-loader/: new directory replacing flat copy-secrets-masterdata.sh and masterdata-install.sh; install.sh uses CHART_VERSION=0.0.1-develop and mosipDataGithubBranch=develop - postgres/ansible/: add ansible playbooks for VM-based postgres setup and cleanup - Remove deploy/chart/istio-addons/ (replaced by mosip/istio-addons helm repo chart) Skipped: helm/postgres-init (develop uses generic template architecture from MOSIP-37808, incompatible with release-1.3.x per-DB template approach); helm chart versions kept as 0.0.1-develop; get_db_data.sh kept as-is (develop pulls all repos from develop branch via $1 parameter) Signed-off-by: ckm007 <chandrakeshavmishra@gmail.com>
|
Warning Review limit reached
More reviews will be available in 38 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds postgres-upgrade CI artifact/build flow, Ansible-based PostgreSQL provisioning/cleanup tooling and inventory generation, updates Helm/istio install wiring, tweaks masterdata-loader scripts/docs, and includes related templates and Dockerfile adjustments. ChangesPostgreSQL Infrastructure & Deployment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
The elif clause for 'esignet' was missing 'then', causing a bash syntax error. This was a pre-existing bug exposed when the get-db-upgrade-scripts CI job started executing the script. Signed-off-by: ckm007 <chandrakeshavmishra@gmail.com>
When triggered by a pull_request event, github.ref is refs/pull/<N>/merge which strips to 'merge' - a non-existent branch. Use github.head_ref for PR events to get the actual source branch name. Signed-off-by: ckm007 <chandrakeshavmishra@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (6)
deploy/postgres-istio-addons-values.yaml (1)
3-5: ⚡ Quick winMake Istio host/controller values overridable per environment.
istio.hostandingressControllerare hard-coded, anddeploy/postgres-install.shLine 25 consumes this file directly. That couples the installer to one environment and forces manual edits for others.Proposed change
--- a/deploy/postgres-install.sh +++ b/deploy/postgres-install.sh @@ NS=postgres ISTIO_ADDONS_CHART_VERSION=1.0.0 +ISTIO_HOST="${ISTIO_HOST:-postgres.sandbox.xyz.net}" +ISTIO_INGRESS_CONTROLLER="${ISTIO_INGRESS_CONTROLLER:-ingressgateway-internal}" @@ - helm -n $NS install istio-addons mosip/istio-addons --version $ISTIO_ADDONS_CHART_VERSION -f postgres-istio-addons-values.yaml --wait + helm -n $NS install istio-addons mosip/istio-addons \ + --version $ISTIO_ADDONS_CHART_VERSION \ + -f postgres-istio-addons-values.yaml \ + --set istio.host="$ISTIO_HOST" \ + --set istio.ingressController="$ISTIO_INGRESS_CONTROLLER" \ + --wait🤖 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 `@deploy/postgres-istio-addons-values.yaml` around lines 3 - 5, The istio.host and ingressController values are hard-coded in postgres-istio-addons-values.yaml, coupling the installer to one environment; update the YAML to make istio.host and ingressController configurable (e.g., replace fixed literals with overridable placeholders or defaults) and modify the installer script that consumes this file so it can accept environment-specific overrides (via env vars, CLI flags, or passing a different values file) and inject those into istio.host and ingressController at runtime; ensure the symbols istio.host and ingressController remain the keys used so downstream code keeps working.deploy/postgres/ansible/cleanup-postgresql.sh (1)
200-200: ⚡ Quick winAdd
-rflag toreadcommands to prevent backslash interpretation.Without the
-rflag, backslashes in user input will be interpreted as escape sequences, which could cause issues with confirmation strings.🔧 Proposed fix
- read -p "Type 'DELETE EVERYTHING' to confirm device wipe: " device_confirm + read -r -p "Type 'DELETE EVERYTHING' to confirm device wipe: " device_confirm if [[ "$device_confirm" != "DELETE EVERYTHING" ]]; then echo "Device wipe cancelled." exit 1 fi fi - read -p "Are you sure you want to proceed with PostgreSQL cleanup? (y/N): " confirm + read -r -p "Are you sure you want to proceed with PostgreSQL cleanup? (y/N): " confirm if [[ $confirm != [yY] && $confirm != [yY][eE][sS] ]]; thenAlso applies to: 207-207
🤖 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 `@deploy/postgres/ansible/cleanup-postgresql.sh` at line 200, Update the interactive prompts that use read (e.g., the lines setting device_confirm and the similar read around line 207) to add the -r flag so backslashes in user input are not treated as escape sequences; change instances like read -p "Type 'DELETE EVERYTHING' to confirm device wipe: " device_confirm to read -r -p "..." device_confirm (and apply the same -r addition to the other read invocation referenced).deploy/postgres/ansible/postgresql-setup.yml (1)
403-411: ⚖️ Poor tradeoffPGPASSWORD environment variable could be visible in process listings.
While these tasks are marked
no_log: true, thePGPASSWORDenvironment variable is exported within shell commands and could be visible in process listings during execution. This is a known limitation of using environment variables for authentication in subprocess commands.Consider using PostgreSQL's
.pgpassfile mechanism for production deployments, which is more secure than environment variables. However, this would require additional playbook logic to create and manage the.pgpassfile with proper permissions.Also applies to: 413-421
🤖 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 `@deploy/postgres/ansible/postgresql-setup.yml` around lines 403 - 411, The task that exports PGPASSWORD in the shell ("Test password authentication locally first") can leak secrets via process listings; replace the inline PGPASSWORD export with creating and using a temporary .pgpass file instead: write a line using {{ postgres_host|default('localhost') }}:{{ postgres_port }}:*:postgres:{{ postgres_password }} into a file (e.g., /tmp/pgpass_{{ inventory_hostname }}), set strict permissions (chmod 600) before invoking psql, run psql pointing to that file via HOME or PGPASSFILE environment, capture the result into local_password_test as before, and ensure the .pgpass file is removed after the check; apply the same change to the similar task at 413-421 and keep no_log: true for registered variables.deploy/postgres/ansible/postgresql-cleanup.yml (1)
183-188: 💤 Low valueIncomplete task: checks for backup but doesn't restore it.
The task is named "Restore original pg_hba.conf backup" but only performs a
statcheck and doesn't actually restore anything. The registeredoriginal_backupvariable is never used in subsequent tasks.Either remove this task if restoration is not needed, or complete the restoration logic if it was intended.
🗑️ Option 1: Remove unused task
- # Restore original pg_hba.conf backup if exists - - name: Check for original pg_hba.conf backup - stat: - path: "{{ postgres_config_dir }}/pg_hba.conf.*~" - register: original_backup - ignore_errors: yes - # Final verification🤖 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 `@deploy/postgres/ansible/postgresql-cleanup.yml` around lines 183 - 188, The current task named "Restore original pg_hba.conf backup" only runs a stat and registers original_backup but never restores anything; fix by adding a follow-up task that actually restores the backup when original_backup.stat.exists is true: locate the backup using the same glob "{{ postgres_config_dir }}/pg_hba.conf.*~" (or use with_fileglob to capture the actual filename), then move or copy that backup over "{{ postgres_config_dir }}/pg_hba.conf" (use the command/shell mv or the copy module with remote_src: yes) and ensure the task uses when: original_backup.stat.exists and appropriate become: yes and notifies a restart if needed; alternatively, if no restore is intended, simply remove the stat task and the original_backup registration.deploy/postgres/ansible/setup-vm-inventory.sh (1)
70-70: ⚡ Quick winAdd
-rflag to allreadcommands to prevent backslash interpretation.Without the
-rflag,readwill interpret backslashes as escape sequences, which can cause issues when users enter file paths or other values containing backslashes. This is especially important for the SSH key path input where backslashes might be present.🔧 Proposed fix
Replace all
read -pwithread -r -p:- read -p "Do you want to overwrite it? (y/N): " overwrite + read -r -p "Do you want to overwrite it? (y/N): " overwriteApply the same change to all other
read -pcommands on lines 83, 92, 106, 123, 127, 136, 145, 153, 162, 172, and 195.Also applies to: 83-83, 92-92, 106-106, 123-123, 127-127, 136-136, 145-145, 153-153, 162-162, 172-172, 195-195
🤖 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 `@deploy/postgres/ansible/setup-vm-inventory.sh` at line 70, Replace every occurrence of the interactive input commands that use read -p with read -r -p to prevent backslash interpretation (e.g., the SSH key path prompt and all other read -p prompts in the script); update each read invocation to add the -r flag so user-entered backslashes are treated literally.deploy/postgres/ansible/run-postgresql-playbook.sh (1)
204-204: ⚡ Quick winAdd
-rflag toreadcommand.Without the
-rflag, backslashes in user input will be interpreted as escape sequences.🔧 Proposed fix
- read -p "Do you want to proceed with PostgreSQL secure setup? (y/N): " -n 1 -r + read -r -p "Do you want to proceed with PostgreSQL secure setup? (y/N): " -n 1 echoNote: The
-rflag should come before-p, and the existing-rat the end is actually part of theREPLYvariable pattern check, not thereadflags.🤖 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 `@deploy/postgres/ansible/run-postgresql-playbook.sh` at line 204, The read invocation that prompts "Do you want to proceed with PostgreSQL secure setup? (y/N):" is not correctly using the raw-input flag; move the -r flag into the read options (before -p) so backslashes in user input are not treated as escapes and keep the -n 1 option; update the read command invocation (the read prompt line) to include -r among its flags rather than relying on the trailing -r which is not a flag there.
🤖 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 @.github/workflows/push-trigger.yml:
- Line 64: The guard expression using "!contains(github.ref, 'master') ||
!contains(github.ref, 'main')" is always true; change it to use logical AND so
both contains checks must be false: replace with "if: ${{ !contains(github.ref,
'master') && !contains(github.ref, 'main') }}" (or equivalently "if: ${{
!(contains(github.ref, 'master') || contains(github.ref, 'main')) }}") for the
same if lines shown and replicate this fix for the other occurrences referenced.
- Around line 53-63: The BRANCH_NAME derivation using github.ref breaks on
pull_request events because github.ref becomes refs/pull/<id>/merge; update the
branch resolution logic that sets BRANCH_NAME (used when calling get_db_data.sh)
to be event-aware: if github.event_name == "pull_request" use
github.event.pull_request.head.ref, otherwise strip the prefix from github.ref
as currently done. Modify the step that echoes BRANCH_NAME into $GITHUB_ENV so
it selects the appropriate source based on github.event_name before invoking
get_db_data.sh.
- Around line 50-51: The workflow uses mutable action refs (actions/checkout@v3,
actions/upload-artifact@v4, mosip/kattu/.../docker-build.yml@master); replace
those with immutable pinned refs (use the corresponding action commit SHAs
instead of tags/branch names) and update the checkout step (the actions/checkout
usage) to harden credentials by setting persist-credentials to false and
fetch-depth to 0 so the runner doesn't expose unnecessary token access or
shallow clones; ensure the remote docker-build reusable workflow reference is
replaced with a specific commit SHA as well.
In `@deploy/masterdata-loader/install.sh`:
- Line 33: install.sh invokes ./copy_secret.sh which currently fetches a GitHub
"blob" HTML URL (see deploy/masterdata-loader/copy_secret.sh) causing curl to
return text/html and break the secret copy; fix by updating the helper URL to
the raw file endpoint and use a robust curl invocation. Modify the UTIL_URL
value in deploy/masterdata-loader/copy_secret.sh to point to the
raw.githubusercontent.com raw file (or otherwise construct the raw URL from the
repo/branch/path), and ensure the download uses curl -fsSL (or equivalent) and
validates a shell/plaintext Content-Type before executing; keep install.sh
calling ./copy_secret.sh unchanged but rely on the corrected UTIL_URL and curl
flags in copy_secret.sh.
In `@deploy/masterdata-loader/README.md`:
- Around line 7-9: The fenced code block in README.md currently lacks a language
tag which triggers MD040; update the snippet that shows ./install.sh to use a
bash code fence by adding "bash" after the opening backticks so the block
becomes a bash fenced code block (i.e., replace ``` with ```bash) to satisfy
markdownlint.
In `@deploy/postgres-install.sh`:
- Line 12: The script deploy/postgres-install.sh calls "helm repo update" then
installs "mosip/istio-addons" but never ensures the mosip repo exists; update
the script to check for the mosip repo (e.g., run "helm repo list" and grep for
the "mosip" entry) and if missing add it with "helm repo add mosip
https://mosip.github.io/mosip-helm" before running helm repo update and the helm
install of "mosip/istio-addons" so the install call succeeds in a clean
environment.
In `@deploy/postgres/ansible/check-postgresql-status.sh`:
- Line 36: The check-postgresql-status.sh script currently runs a Debian-only
command ("dpkg -l | grep postgresql") which will fail on RHEL/CentOS; update the
remote check to detect the host OS family (via Ansible facts or /etc/os-release)
and run the appropriate package query per host: use rpm -qa | grep postgresql
for RedHat family and dpkg -l | grep postgresql for Debian family (or consult
ansible_facts['os_family']) and log "No PostgreSQL packages found" only if
neither check finds packages; update the ansible shell invocation that currently
contains the dpkg command to branch based on the detected OS family (refer to
the command in check-postgresql-status.sh).
- Around line 102-118: The ansible shell payload is wrapped in single quotes so
remote shell substitutions like $(hostname), $(date) and $(dpkg -l | grep
postgresql | wc -l) are not expanded; change the invocation in the ansible
"$TARGET_GROUP" -i "$INVENTORY_FILE" -m shell -a block to use double quotes for
the outer string and escape all dollar signs (e.g. \$(...)) so the commands are
evaluated on the remote host, or alternatively replace the inline quoted script
with a here-doc/EOF form passed to -a to preserve expansion; update the echoed
lines that reference $(...) and any $vars accordingly.
In `@deploy/postgres/ansible/postgresql-cleanup.yml`:
- Around line 151-158: The task currently named "Wipe storage device (if
requested)" only zeroes the first 100MB (dd bs=1M count=100) which is
misleading; either rename the task to reflect signature-only behavior (e.g.,
change the task name string "Wipe storage device (if requested)" to "Wipe device
signature (first 100MB)" and leave the wipefs + dd count=100 commands using
postgres_device and wipe_device) or implement a true full-device wipe by
removing the dd count so dd writes until EOF (replace dd if=/dev/zero of="{{
postgres_device }}" bs=1M count=100 with dd if=/dev/zero of="{{ postgres_device
}}" bs=1M status=progress oflag=direct to stream until completion and keep
wipefs -a), and ensure the when condition still uses wipe_device |
default(false) | bool; pick one option and update the task name/commands
accordingly.
In `@deploy/postgres/ansible/postgresql-setup.yml`:
- Around line 324-349: The pg_hba.conf task ("Configure pg_hba.conf for access")
currently hardcodes "0.0.0.0/0" which allows connections from any IP; replace
that CIDR with the playbook variable collected earlier (network_cidr) so access
is limited to the intended range, and add a sensible default or validation
(e.g., fail the play if network_cidr is undefined or use a safe default like
localhost CIDR) to avoid accidental wide-open access; update the host line in
the content block to reference network_cidr (and keep owner/group/mode/notify
unchanged).
- Around line 374-382: The current task "Set PostgreSQL postgres user password
only if needed" embeds '{{ postgres_password }}' in a shell heredoc which can
leak via process listings; instead create a temporary .pgpass file owned by the
postgres user with mode 0600 containing the line "*:{{ postgres_port
}}:postgres:{{ postgres_password }}", set PGPASSFILE to that temp file and run
sudo -u postgres psql -p {{ postgres_port }} -c "ALTER USER postgres PASSWORD
'{{ postgres_password }}';" (or simply run the ALTER command without a heredoc),
then securely remove the temp .pgpass; update the task that registers
password_set_result and keep the when: existing_password_test.rc != 0 condition.
In `@deploy/postgres/ansible/README.md`:
- Line 134: The fenced code block that currently ends/starts with the triple
backticks (```) is missing a language identifier, which triggers markdownlint
MD040; update the fenced code block marker from ``` to include a language tag
such as ```text (or ```bash/```md as appropriate) so the block is explicitly
typed and MD040 is satisfied.
- Line 107: The README line "No plaintext passwords in logs or files" is
inaccurate for generated Kubernetes Secret files because
`/tmp/postgresql-secrets/*.yml` contains base64-encoded secret data that is
reversible; update the README text (the heading/line that mentions "No plaintext
passwords in logs or files") to explicitly state that secrets are base64-encoded
and therefore sensitive plaintext-equivalents, instruct users to treat
`/tmp/postgresql-secrets/*.yml` as sensitive, recommend immediate secure
deletion (e.g., rm -f) and restricted permissions after use, and remove or
reword the misleading absolute claim so it accurately reflects that encoded
secrets must be protected.
- Around line 73-85: The README hardcodes the secret filename
(postgres-postgresql.yml) but the playbook outputs {{ postgres_secret_name
}}.yml when --secret-name is used, causing kubectl apply to fail; update the
README text so the apply step mentions the dynamic filename (use the placeholder
{{ postgres_secret_name }}.yml and call out the --secret-name flag), or show the
safe alternatives: apply the specific generated file by name using the
placeholder or apply all files in /tmp/postgresql-secrets/ (e.g., a glob), and
update the list entries for postgres-postgresql.yml, postgres-setup-config.yml,
and DEPLOYMENT_INSTRUCTIONS.md to reflect the variable output naming.
In `@deploy/postgres/ansible/run-postgresql-playbook.sh`:
- Around line 76-80: In run-postgresql-playbook.sh the python3 bcrypt check
currently runs apt-get (lines showing the python3 import check and sudo apt-get
install) which assumes Debian and sudo; change this to only detect and warn
instead of auto-installing: when the python3 -c "import bcrypt" test fails, emit
a clear warning via the warning function that the python3-bcrypt/py-bcrypt
package is missing and list the OS families supported (Debian/RedHat) with
guidance to install via the appropriate package manager or run the Ansible role
that handles dependencies; do not call apt-get or sudo in the script—either exit
with a non-zero status or rely on the playbook to install the package so
CI/restricted environments are not prompted for sudo.
In `@deploy/postgres/ansible/setup-vm-inventory.sh`:
- Line 16: DEFAULT_PG_PORT is set to "5432" but the hosts.ini template and
README examples use "5433"; update the DEFAULT_PG_PORT variable in
setup-vm-inventory.sh to "5433" so it matches hosts.ini and documentation (and
verify no other scripts or references expect 5432), specifically change the
DEFAULT_PG_PORT definition and run a quick grep for DEFAULT_PG_PORT/port
references to ensure consistency.
- Around line 258-264: The success message prints the wrong runner script name;
update the echo lines that currently reference "./ansible-postgresql.sh" to use
the actual runner "./run-postgresql-playbook.sh" (check the echo string around
"You can now run your PostgreSQL automation scripts:"), and scan for any other
occurrences of "ansible-postgresql.sh" in the same file to replace with
"run-postgresql-playbook.sh" so the README and script messages match.
---
Nitpick comments:
In `@deploy/postgres-istio-addons-values.yaml`:
- Around line 3-5: The istio.host and ingressController values are hard-coded in
postgres-istio-addons-values.yaml, coupling the installer to one environment;
update the YAML to make istio.host and ingressController configurable (e.g.,
replace fixed literals with overridable placeholders or defaults) and modify the
installer script that consumes this file so it can accept environment-specific
overrides (via env vars, CLI flags, or passing a different values file) and
inject those into istio.host and ingressController at runtime; ensure the
symbols istio.host and ingressController remain the keys used so downstream code
keeps working.
In `@deploy/postgres/ansible/cleanup-postgresql.sh`:
- Line 200: Update the interactive prompts that use read (e.g., the lines
setting device_confirm and the similar read around line 207) to add the -r flag
so backslashes in user input are not treated as escape sequences; change
instances like read -p "Type 'DELETE EVERYTHING' to confirm device wipe: "
device_confirm to read -r -p "..." device_confirm (and apply the same -r
addition to the other read invocation referenced).
In `@deploy/postgres/ansible/postgresql-cleanup.yml`:
- Around line 183-188: The current task named "Restore original pg_hba.conf
backup" only runs a stat and registers original_backup but never restores
anything; fix by adding a follow-up task that actually restores the backup when
original_backup.stat.exists is true: locate the backup using the same glob "{{
postgres_config_dir }}/pg_hba.conf.*~" (or use with_fileglob to capture the
actual filename), then move or copy that backup over "{{ postgres_config_dir
}}/pg_hba.conf" (use the command/shell mv or the copy module with remote_src:
yes) and ensure the task uses when: original_backup.stat.exists and appropriate
become: yes and notifies a restart if needed; alternatively, if no restore is
intended, simply remove the stat task and the original_backup registration.
In `@deploy/postgres/ansible/postgresql-setup.yml`:
- Around line 403-411: The task that exports PGPASSWORD in the shell ("Test
password authentication locally first") can leak secrets via process listings;
replace the inline PGPASSWORD export with creating and using a temporary .pgpass
file instead: write a line using {{ postgres_host|default('localhost') }}:{{
postgres_port }}:*:postgres:{{ postgres_password }} into a file (e.g.,
/tmp/pgpass_{{ inventory_hostname }}), set strict permissions (chmod 600) before
invoking psql, run psql pointing to that file via HOME or PGPASSFILE
environment, capture the result into local_password_test as before, and ensure
the .pgpass file is removed after the check; apply the same change to the
similar task at 413-421 and keep no_log: true for registered variables.
In `@deploy/postgres/ansible/run-postgresql-playbook.sh`:
- Line 204: The read invocation that prompts "Do you want to proceed with
PostgreSQL secure setup? (y/N):" is not correctly using the raw-input flag; move
the -r flag into the read options (before -p) so backslashes in user input are
not treated as escapes and keep the -n 1 option; update the read command
invocation (the read prompt line) to include -r among its flags rather than
relying on the trailing -r which is not a flag there.
In `@deploy/postgres/ansible/setup-vm-inventory.sh`:
- Line 70: Replace every occurrence of the interactive input commands that use
read -p with read -r -p to prevent backslash interpretation (e.g., the SSH key
path prompt and all other read -p prompts in the script); update each read
invocation to add the -r flag so user-entered backslashes are treated literally.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b5c2fae-838d-4607-bf50-aa2f302864b9
⛔ Files ignored due to path filters (1)
postgres-upgrade/upgrade.csvis excluded by!**/*.csv
📒 Files selected for processing (25)
.github/workflows/push-trigger.ymldeploy/chart/README.mddeploy/chart/istio-addons/.helmignoredeploy/chart/istio-addons/Chart.yamldeploy/chart/istio-addons/templates/gateway.yamldeploy/chart/istio-addons/templates/vs.yamldeploy/chart/istio-addons/values.yamldeploy/masterdata-loader/README.mddeploy/masterdata-loader/copy_secret.shdeploy/masterdata-loader/install.shdeploy/postgres-install.shdeploy/postgres-istio-addons-values.yamldeploy/postgres/ansible/README.mddeploy/postgres/ansible/check-postgresql-status.shdeploy/postgres/ansible/cleanup-postgresql.shdeploy/postgres/ansible/hosts.inideploy/postgres/ansible/postgresql-cleanup.ymldeploy/postgres/ansible/postgresql-setup.ymldeploy/postgres/ansible/run-postgresql-playbook.shdeploy/postgres/ansible/setup-vm-inventory.shhelm/postgres-upgrade/templates/postgres-upgrade-job.yamlhelm/postgres-upgrade/values.yamlpostgres-upgrade/Dockerfilepostgres-upgrade/get_db_data.shpostgres-upgrade/repo-list.txt
💤 Files with no reviewable changes (6)
- deploy/chart/istio-addons/templates/vs.yaml
- deploy/chart/istio-addons/.helmignore
- deploy/chart/istio-addons/templates/gateway.yaml
- deploy/chart/README.md
- deploy/chart/istio-addons/Chart.yaml
- deploy/chart/istio-addons/values.yaml
head_ref returns the PR source branch (e.g. port/release-1.3.x-to-develop) which does not exist in module repos being cloned by get_db_data.sh. base_ref returns the PR target branch (e.g. develop, release-1.3.x) which is the correct branch to clone module repos from. Signed-off-by: ckm007 <chandrakeshavmishra@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/push-trigger.yml (1)
55-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
releaserefs explicitly in branch selection.Line 59 derives
BRANCH_NAMEfromgithub.ref, which becomes a tag onreleaseevents. Line 66 then passes that toget_db_data.sh(git clone -b $1), so release runs can fail when the tag is not present across all repos inrepo-list.txt.Suggested patch
- # For PRs use base_ref (target branch); for push/release strip the ref - if [ "${{ github.event_name }}" == "pull_request" ]; then - echo "BRANCH_NAME=${{ github.base_ref }}" >> $GITHUB_ENV - else - echo "BRANCH_NAME=$(echo ${{ github.ref }} | sed -e 's,.*/\(.*\),\1,')" >> $GITHUB_ENV - fi + # PR -> target branch, release -> target_commitish, others -> ref_name + if [ "${{ github.event_name }}" = "pull_request" ]; then + BRANCH_NAME="${{ github.base_ref }}" + elif [ "${{ github.event_name }}" = "release" ]; then + BRANCH_NAME="${{ github.event.release.target_commitish }}" + else + BRANCH_NAME="${{ github.ref_name }}" + fi + echo "BRANCH_NAME=${BRANCH_NAME}" >> "$GITHUB_ENV" echo "GPG_TTY=$(tty)" >> $GITHUB_ENV @@ - bash ./get_db_data.sh ${{ env.BRANCH_NAME }} + bash ./get_db_data.sh "${{ env.BRANCH_NAME }}"🤖 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 @.github/workflows/push-trigger.yml around lines 55 - 67, The workflow sets BRANCH_NAME from github.base_ref for pull_request and from github.ref for others, but github.ref is a tag on release events which breaks git clone -b in get_db_data.sh; update the branch-selection logic to explicitly handle release events by checking github.event_name == "release" and setting BRANCH_NAME to the release tag (github.event.release.tag_name) before falling back to the existing sed extraction, so get_db_data.sh / git clone -b receives the correct tag/branch name.
🤖 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.
Outside diff comments:
In @.github/workflows/push-trigger.yml:
- Around line 55-67: The workflow sets BRANCH_NAME from github.base_ref for
pull_request and from github.ref for others, but github.ref is a tag on release
events which breaks git clone -b in get_db_data.sh; update the branch-selection
logic to explicitly handle release events by checking github.event_name ==
"release" and setting BRANCH_NAME to the release tag
(github.event.release.tag_name) before falling back to the existing sed
extraction, so get_db_data.sh / git clone -b receives the correct tag/branch
name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0edf87ea-bac3-4ae7-ac78-f865b4609e86
📒 Files selected for processing (1)
.github/workflows/push-trigger.yml
- push-trigger.yml: fix !contains || condition to != && form so Ready/Upload steps correctly skip on master and main branches - copy_secret.sh: fix blob URL to raw.githubusercontent.com so wget fetches the shell script instead of HTML - masterdata-loader/README.md: add bash language tag to fenced code block - setup-vm-inventory.sh: fix script reference from ansible-postgresql.sh to run-postgresql-playbook.sh Signed-off-by: ckm007 <chandrakeshavmishra@gmail.com>
- Replace sed-based ref stripping with github.ref_name (cleaner, no shell command needed) for non-PR events - Quote BRANCH_NAME argument passed to get_db_data.sh to prevent word splitting - Use github.ref_name != 'master/main' instead of github.ref != 'refs/heads/*' (simpler and works correctly for both branch and tag refs) - Add master/main guard to build-upgrade-dockers job so it also skips on master and main branches Signed-off-by: ckm007 <chandrakeshavmishra@gmail.com>
- setup-vm-inventory.sh: align DEFAULT_PG_PORT from 5432 to 5433 to match hosts.ini template and documentation - ansible/README.md: add 'text' language tag to workflow diagram code block Signed-off-by: ckm007 <chandrakeshavmishra@gmail.com>
mosip/istio-addons and bitnami/postgresql installs would fail with 'repo not found' on a clean environment because helm repo add was never called. Add both repo registrations before helm repo update, consistent with deploy/postgres-upgrade.sh. Signed-off-by: ckm007 <chandrakeshavmishra@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/masterdata-loader/copy_secret.sh (1)
6-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin and verify the downloaded utility before execution.
Line 6 currently pulls
copy_cm_func.shfrom a movingmasterref, and Line 10 executes that downloaded content. This creates a supply-chain risk and nondeterministic installs if upstream changes unexpectedly.Suggested hardening
- UTIL_URL=https://raw.githubusercontent.com/mosip/mosip-infra/master/deployment/v3/utils/copy_cm_func.sh + UTIL_URL=https://raw.githubusercontent.com/mosip/mosip-infra/<pinned-commit-sha>/deployment/v3/utils/copy_cm_func.sh + UTIL_SHA256="<expected_sha256>" @@ - wget -q $UTIL_URL -O copy_cm_func.sh && chmod +x copy_cm_func.sh + wget -q "$UTIL_URL" -O copy_cm_func.sh + echo "${UTIL_SHA256} copy_cm_func.sh" | sha256sum -c - + chmod +x copy_cm_func.sh🤖 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 `@deploy/masterdata-loader/copy_secret.sh` around lines 6 - 10, The script downloads and executes copy_cm_func.sh unsafely via UTIL_URL and wget; change UTIL_URL to point to a specific immutable commit SHA (raw URL with commit), download to COPY_UTIL using a robust fetch (e.g., curl/wget with -fSL), verify the file integrity before execution by checking a pinned SHA256 checksum (store expected hash in the script) or verifying a signed release, and only then run chmod +x on COPY_UTIL and execute it; ensure the script exits non‑zero if checksum/signature verification fails so the file is never executed on mismatch.
🤖 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 @.github/workflows/push-trigger.yml:
- Line 68: The branch guard using github.ref_name is incorrect for pull_request
events; update the condition(s) (the three occurrences currently using
github.ref_name) to use github.base_ref when github.event_name == 'pull_request'
and fall back to github.ref_name otherwise — e.g. replace conditions like
"github.ref_name != 'master' && github.ref_name != 'main'" with a combined
expression such as "((github.event_name == 'pull_request' && github.base_ref !=
'master' && github.base_ref != 'main') || (github.event_name != 'pull_request'
&& github.ref_name != 'master' && github.ref_name != 'main'))" so the checks
correctly exclude PRs targeting master/main.
---
Outside diff comments:
In `@deploy/masterdata-loader/copy_secret.sh`:
- Around line 6-10: The script downloads and executes copy_cm_func.sh unsafely
via UTIL_URL and wget; change UTIL_URL to point to a specific immutable commit
SHA (raw URL with commit), download to COPY_UTIL using a robust fetch (e.g.,
curl/wget with -fSL), verify the file integrity before execution by checking a
pinned SHA256 checksum (store expected hash in the script) or verifying a signed
release, and only then run chmod +x on COPY_UTIL and execute it; ensure the
script exits non‑zero if checksum/signature verification fails so the file is
never executed on mismatch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6848e44-5098-4d70-bab0-fcf7d78afd0d
📒 Files selected for processing (5)
.github/workflows/push-trigger.ymldeploy/masterdata-loader/README.mddeploy/masterdata-loader/copy_secret.shdeploy/postgres/ansible/README.mddeploy/postgres/ansible/setup-vm-inventory.sh
✅ Files skipped from review due to trivial changes (2)
- deploy/masterdata-loader/README.md
- deploy/postgres/ansible/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/postgres/ansible/setup-vm-inventory.sh
github.ref_name on a PR run resolves to the merge ref, not the target branch, so the master/main guard was not reliable for PRs. Use base_ref when event is pull_request and fall back to ref_name for push/release. Applied to all three if conditions: Ready scripts, Upload artifact, and build-upgrade-dockers job. Signed-off-by: ckm007 <chandrakeshavmishra@gmail.com>
✅ Action performedReview finished.
|
Prafulrakhade
approved these changes
Jun 5, 2026
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
2 participants
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.
…improvements to develop
postgres-upgrade:
CI workflow:
helm/postgres-upgrade:
deploy:
Skipped: helm/postgres-init (develop uses generic template architecture from MOSIP-37808, incompatible with release-1.3.x per-DB template approach); helm chart versions kept as 0.0.1-develop; get_db_data.sh kept as-is (develop pulls all repos from develop branch via $1 parameter)
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores