[MOSIP-44613]Resolved code rabbit comments#192
Merged
bhumi46 merged 3 commits intomosip:developfrom Mar 30, 2026
Merged
Conversation
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughUpdates GitHub Actions profile auto-detection/quoting and dispatch MODE propagation; tightens DSF filename matching; applies Helm/YAML env-variable substitutions and path fixes; hardens hook scripts (sed/jq/patches, timeouts, decoding); and adds Terraform ActiveMQ NFS allowed-hosts variable, validations and wiring across modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
Helmsman/hooks/esignet-1.7.1/esignet-preinstall.sh (1)
52-54: Consistent pattern applied across all copy operations.All five namespace substitutions in this file now use the anchored regex pattern.
Note: Per the relevant code snippets, other scripts in this directory (
softhsm-esignet-postinstall.sh,redis-setup.sh) and the shared utilitycopy_cm_func.shstill use the older unanchored pattern. Consider updating those for consistency in a follow-up PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Helmsman/hooks/esignet-1.7.1/esignet-preinstall.sh` around lines 52 - 54, Update all five namespace copy operations in esignet-preinstall.sh to use the anchored sed regex pattern; specifically, in each kubectl ... get configmap ... | sed ... | kubectl ... pipeline replace the older unanchored substitution with the anchored form used in the diff (use s|^\(\s*namespace:\) $POSTGRES_NS$|\1 $ESIGNET_NS| style), making sure to reference the variables POSTGRES_NS and ESIGNET_NS and preserve quoting and escaping in the sed expression so the namespace key is matched only at the start of the line.terraform/modules/aws/activemq-setup/activemq-setup.sh (1)
120-126: Consider moving this manifest check to a per-run path.
/tmp/activemq-storageclass.yamlis shared runner state, while the rest of this script already uses a unique$WORK_DIR. Writing and checking the manifest under$WORK_DIRwould avoid stale-file/concurrent-apply collisions and make this guard prove that the current run produced the artifact. That would need the same path wired throughactivemq-setup.ymlandmain.tf.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/modules/aws/activemq-setup/activemq-setup.sh` around lines 120 - 126, The storageClass manifest check currently reads /tmp/activemq-storageclass.yaml which can collide across concurrent runs; update the check to use the run-specific path under $WORK_DIR (the same path used elsewhere in this script) so the guard validates the artifact produced by this run. Modify the script location that writes/reads the manifest and update the callers/configs that reference it (activemq-setup.yml and the Terraform module invocation in main.tf) to pass the new $WORK_DIR-based path instead of the hardcoded /tmp path, ensuring all places consistently use the unique per-run filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/helmsman_external.yml:
- Around line 69-70: The PROFILE variable is being set to a newline-separated
list (PROFILE="$changed_profiles"), which breaks downstream use in the helmsman
invocation that expects a single profile; change the assignment so PROFILE
contains only a single profile (e.g., extract the first entry from
changed_profiles) or instead iterate over changed_profiles and invoke/dispatch
the downstream workflow once per profile; update the code that calls helmsman
(the command using $WORKDIR/dsf/${PROFILE}/mosip-dsf.yaml) to run for each
profile when iterating or to use the single extracted PROFILE value.
In `@Helmsman/dsf/mosip-platform-java11/prereq-dsf.yaml`:
- Around line 45-46: The two helm values grafana.global.cattle.clusterId and
global.cattle.clusterId reference an unset $CLUSTER_ID; define CLUSTER_ID in the
environment or provide a safe default before enabling rancher-monitoring. Fix by
either exporting CLUSTER_ID in your pipeline/CI env (or the chart values
provider) or replace the literal references with a fallback expression (e.g., a
default cluster id) so grafana.global.cattle.clusterId and
global.cattle.clusterId never resolve to an empty/undefined value when
rancher-monitoring is enabled.
- Around line 36-37: The Helmsman prereq uses ${ISTIO_VERSION}
(postInstall/postUpgrade in prereq-dsf.yaml) but ISTIO_VERSION isn't exported in
the workflow so it expands empty and the hook install-istio-and-httpbin.sh falls
back to "develop"; export ISTIO_VERSION (e.g., ISTIO_VERSION=1.22.0) to
$GITHUB_ENV in the "Setup kubectl, istioctl and kubeconfig" step of
helmsman_external.yml so Helmsman gets the value, and also export CLUSTER_ID to
$GITHUB_ENV in the "Initiate helmsman to apply the DSF configurations" step so
${CLUSTER_ID} is populated at runtime. Ensure the exported names match exactly
(ISTIO_VERSION, CLUSTER_ID) so prereq-dsf.yaml postInstall/postUpgrade and any
references to CLUSTER_ID receive the intended values.
In `@Helmsman/dsf/mosip-platform-java21/prereq-dsf.yaml`:
- Around line 45-46: The two entries grafana.global.cattle.clusterId and
global.cattle.clusterId are using an undefined literal "$CLUSTER_ID"; replace
those literal references with a proper templated value and ensure it's declared
in your chart values (for example change "$CLUSTER_ID" to a Helm template like
"{{ .Values.clusterId }}" or to the same variable used in the java11 fix), and
add clusterId to values.yaml (or export the environment variable where you do
string substitution) so the value is defined at deploy time.
- Around line 36-37: The postInstall/postUpgrade commands in prereq-dsf.yaml
reference an undefined ${ISTIO_VERSION} variable (see the
postInstall/postUpgrade entries), so update the GitHub Actions workflow that
runs this prereq to export ISTIO_VERSION into the environment by writing
ISTIO_VERSION=<value> to $GITHUB_ENV before the job/step that applies this YAML;
ensure the workflow sets the same variable name (ISTIO_VERSION) so the
manifest's ${ISTIO_VERSION} resolves at runtime.
In `@Helmsman/hooks/esignet-1.7.1/esignet-init-db.sh`:
- Around line 28-30: Update esignet-1.7.1/redis-setup.sh to use the same
anchored, POSIX-compatible sed pattern as esignet-init-db.sh: replace the older
global replacement sed invocation with one that anchors to the "namespace:" line
and uses [[:space:]]* (e.g., match "^\([[:space:]]*namespace:\) <old_ns>$" and
substitute the new namespace), preserving the variable usage for current and
target namespaces and piping/kubectl logic exactly as in esignet-init-db.sh so
the change is consistent and safe across shell environments.
In `@Helmsman/hooks/esignet-1.7.1/redis-setup.sh`:
- Around line 23-24: The current kubectl wait command for Redis pods (kubectl -n
"$REDIS_NS" wait --for=condition=ready pod -l app.kubernetes.io/name=redis
--timeout=300s || echo "WARNING: Redis pods not ready after timeout,
continuing") silently continues on timeout which can hide deployment failures
for services in $ESIGNET_NS; change this to fail-fast or at least surface to
stderr and match Helmsman timing: either remove the "|| echo ..." and let the
script exit non‑zero on failure, or replace the echo with a stderr log (echo
"... " >&2) and an explicit exit 1; alternatively align the timeout with
Helmsman (480s) or implement a simple retry/backoff loop around kubectl wait to
give Redis more time before exiting.
In `@terraform/implementations/aws/infra/profiles/esignet/aws.tfvars`:
- Line 80: subdomain_public currently includes "minio" which exposes MinIO
publicly while the consumer esignet-resident-oidc-partner-onboarder is disabled;
either remove "minio" from subdomain_public to keep MinIO internal-only, or
enable partner-onboarder in esignet-dsf.yaml and ensure secure S3 creds (replace
s3-user-key: "admin" with proper secret management); also update the profile
description to mention MinIO is deployed via external-dsf.yaml so documentation
reflects reality.
In `@terraform/implementations/aws/infra/variables.tf`:
- Around line 257-260: The current terraform validation for
var.activemq_mount_point only ensures it's non-empty and starts with "/", but it
must also forbid the root path "/" to prevent mounting/exporting the host root;
update the validation block (the condition for var.activemq_mount_point) to
require more than one character or explicitly check that
var.activemq_mount_point != "/" (while keeping the startswith check) so
activemq-setup.yml never receives "/" as the mount target/export path.
- Around line 247-250: The validation for var.activemq_storage_device currently
allows the bare string "/dev/"; update the validation condition in the
validation block for var.activemq_storage_device so it requires at least one
path segment after the "/dev/" prefix (e.g., require
length(var.activemq_storage_device) > length("/dev/") or use a regex that
matches "^/dev/[^/].+"). Ensure the error_message remains descriptive and that
the check references var.activemq_storage_device so malformed tfvars fail at
plan time.
In `@terraform/modules/aws/activemq-setup/activemq-setup.yml`:
- Around line 99-105: The playbook uses activemq_nfs_allowed_hosts but that
variable isn't exposed or passed through the module; add a module input variable
named activemq_nfs_allowed_hosts (with a sensible default like '*' or empty to
force explicit set) to the Terraform module interface, add the corresponding
variable in terraform/implementations/aws/infra/variables.tf, pass it through
wherever activemq-setup.sh is invoked (so the script can inject it into the
Ansible extra-vars), and ensure activemq-setup.sh forwards the value into the
Ansible run so activemq-setup.yml can consume activemq_nfs_allowed_hosts; update
any docs or defaults so the export allowlist is actually configurable.
---
Nitpick comments:
In `@Helmsman/hooks/esignet-1.7.1/esignet-preinstall.sh`:
- Around line 52-54: Update all five namespace copy operations in
esignet-preinstall.sh to use the anchored sed regex pattern; specifically, in
each kubectl ... get configmap ... | sed ... | kubectl ... pipeline replace the
older unanchored substitution with the anchored form used in the diff (use
s|^\(\s*namespace:\) $POSTGRES_NS$|\1 $ESIGNET_NS| style), making sure to
reference the variables POSTGRES_NS and ESIGNET_NS and preserve quoting and
escaping in the sed expression so the namespace key is matched only at the start
of the line.
In `@terraform/modules/aws/activemq-setup/activemq-setup.sh`:
- Around line 120-126: The storageClass manifest check currently reads
/tmp/activemq-storageclass.yaml which can collide across concurrent runs; update
the check to use the run-specific path under $WORK_DIR (the same path used
elsewhere in this script) so the guard validates the artifact produced by this
run. Modify the script location that writes/reads the manifest and update the
callers/configs that reference it (activemq-setup.yml and the Terraform module
invocation in main.tf) to pass the new $WORK_DIR-based path instead of the
hardcoded /tmp path, ensuring all places consistently use the unique per-run
filename.
🪄 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: CHILL
Plan: Pro
Run ID: 0f74cac6-5ecc-4997-9d7c-9f2f59aa554b
📒 Files selected for processing (19)
.github/workflows/helmsman_esignet.yml.github/workflows/helmsman_external.yml.github/workflows/helmsman_mosip.yml.github/workflows/helmsman_testrigs.ymlHelmsman/dsf/mosip-platform-java11/prereq-dsf.yamlHelmsman/dsf/mosip-platform-java21/esignet-dsf.yamlHelmsman/dsf/mosip-platform-java21/prereq-dsf.yamlHelmsman/dsf/mosip-platform-java21/testrigs-dsf.yamlHelmsman/hooks/esignet-1.7.1/captcha-postinstall.shHelmsman/hooks/esignet-1.7.1/esignet-init-db.shHelmsman/hooks/esignet-1.7.1/esignet-preinstall.shHelmsman/hooks/esignet-1.7.1/keycloak-postinstall.shHelmsman/hooks/esignet-1.7.1/redis-setup.shHelmsman/hooks/esignet-1.7.1/softhsm-mock-identity-system-postinstall.shterraform/implementations/aws/infra/profiles/esignet/aws.tfvarsterraform/implementations/aws/infra/variables.tfterraform/modules/aws/activemq-setup/activemq-setup.shterraform/modules/aws/activemq-setup/activemq-setup.ymlterraform/modules/aws/activemq-setup/main.tf
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/helmsman_external.yml (1)
276-282: Payload construction is correct; consider quoting$GITHUB_REPOin URL.The MODE propagation with
${MODE:-apply}default is properly implemented. Static analysis (SC2086) flags unquoted$GITHUB_REPOon line 281. While repository names don't contain spaces in practice, quoting it improves robustness.🔧 Optional fix for shellcheck SC2086
curl -X POST \ -H "Accept: application/vnd.github+json" \ -H "Authorization: Bearer $GITHUB_TOKEN" \ -H "X-GitHub-Api-Version: 2022-11-28" \ - https://api.github.com/repos/$GITHUB_REPO/actions/workflows/helmsman_mosip.yml/dispatches \ + "https://api.github.com/repos/$GITHUB_REPO/actions/workflows/helmsman_mosip.yml/dispatches" \ -d '{"ref":"'"$BRANCH"'","inputs":{"mode":"'"${MODE:-apply}"'","profile":"'"${{ needs.set-matrix.outputs.profile }}"'"}}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helmsman_external.yml around lines 276 - 282, The curl invocation is using an unquoted $GITHUB_REPO in the dispatch URL which triggers shellcheck SC2086; update the run block so the URL uses a quoted repository variable (replace $GITHUB_REPO with "${GITHUB_REPO}") in the https://api.github.com/repos/.../actions/workflows/helmsman_mosip.yml/dispatches call to prevent word-splitting and globbing while leaving the rest (including "${BRANCH}" and the JSON payload) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terraform/implementations/aws/infra/profiles/mosip/aws.tfvars`:
- Line 111: The activemq_nfs_allowed_hosts variable is set to "*" which is
overly permissive; replace the wildcard with a restricted CIDR or host list
(e.g., the cluster CIDR such as "10.0.0.0/8" or the specific application subnet)
and remove the "*" default; update the activemq_nfs_allowed_hosts assignment to
a concrete CIDR or variable reference (e.g., use a cluster_cidr variable) so
only trusted IP ranges can mount the NFS export and adjust the inline comment to
reflect the new restricted value.
In `@terraform/implementations/aws/infra/variables.tf`:
- Around line 265-269: The variable activemq_nfs_allowed_hosts currently accepts
any string; add a Terraform validation block on variable
"activemq_nfs_allowed_hosts" that enforces allowed formats (accept "*" or a
single IP, CIDR, hostname, or a comma-separated list of those) using a regex
condition and a clear error message; implement this by adding a validation {
condition =
can(regex("^(\\*|([0-9]{1,3}(\\.[0-9]{1,3}){3}(\\/\\d{1,2})?|[A-Za-z0-9.-]+)(,([0-9]{1,3}(\\.[0-9]{1,3}){3}(\\/\\d{1,2})?|[A-Za-z0-9.-]+))*)$",
var.activemq_nfs_allowed_hosts)) error = "activemq_nfs_allowed_hosts must be '*'
or an IP/CIDR/hostname or a comma-separated list of those." } inside the
variable block so malformed values cannot be applied.
---
Nitpick comments:
In @.github/workflows/helmsman_external.yml:
- Around line 276-282: The curl invocation is using an unquoted $GITHUB_REPO in
the dispatch URL which triggers shellcheck SC2086; update the run block so the
URL uses a quoted repository variable (replace $GITHUB_REPO with
"${GITHUB_REPO}") in the
https://api.github.com/repos/.../actions/workflows/helmsman_mosip.yml/dispatches
call to prevent word-splitting and globbing while leaving the rest (including
"${BRANCH}" and the JSON payload) unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 5df0f94a-670b-41e7-b98d-20038ef3e1bb
📒 Files selected for processing (12)
.github/workflows/helmsman_external.ymlHelmsman/hooks/esignet-1.7.1/redis-setup.shterraform/implementations/aws/infra/main.tfterraform/implementations/aws/infra/profiles/esignet/aws.tfvarsterraform/implementations/aws/infra/profiles/mosip/aws.tfvarsterraform/implementations/aws/infra/variables.tfterraform/infra/aws/main.tfterraform/infra/aws/variables.tfterraform/infra/main.tfterraform/infra/variables.tfterraform/modules/aws/activemq-setup/activemq-setup.shterraform/modules/aws/activemq-setup/main.tf
🚧 Files skipped from review as they are similar to previous changes (3)
- Helmsman/hooks/esignet-1.7.1/redis-setup.sh
- terraform/modules/aws/activemq-setup/main.tf
- terraform/implementations/aws/infra/profiles/esignet/aws.tfvars
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
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
1 participant
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 by CodeRabbit
Bug Fixes
Chores