[MOSIP-44684] Fixed the issue related to pushing reports to MinIO#177
Open
abhishek8shankar wants to merge 2 commits into
Open
[MOSIP-44684] Fixed the issue related to pushing reports to MinIO#177abhishek8shankar wants to merge 2 commits into
abhishek8shankar wants to merge 2 commits into
Conversation
Signed-off-by: Abhi <abhishek.shankarcs@gmail.com>
WalkthroughUpdates Dockerfile to verify MinIO ChangesDockerfile Container Image Build
S3 Onboarding Script and Configuration
Deployment values update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
deploy/install.sh (2)
33-33: ⚡ Quick winUse
read -rto prevent backslash interpretation.The
readcommands at lines 33, 53, 65, 72, and 107 should use the-rflag to prevent backslash sequences from being interpreted. Without-r, input containing backslashes will be mangled. While unlikely for the current use cases, usingread -ris a shell scripting best practice.🐚 Proposed fix
- read -p "Is values.yaml for onboarder chart set correctly as part of Pre-requisites?(Y/n) " yn; + read -r -p "Is values.yaml for onboarder chart set correctly as part of Pre-requisites?(Y/n) " yn; if [ "$yn" = "Y" ]; then echo Istio label kubectl label ns $NS istio-injection=disabled --overwrite helm repo update echo Copy configmaps kubectl -n $NS --ignore-not-found=true delete cm s3 kubectl -n $NS --ignore-not-found=true delete cm onboarder-namespace sed -i 's/\r$//' copy_cm.sh ./copy_cm.sh echo Copy secrets sed -i 's/\r$//' copy_secrets.sh ./copy_secrets.sh - read -p "Provide onboarder bucket name : " s3_bucket + read -r -p "Provide onboarder bucket name : " s3_bucket if [[ -z "$s3_bucket" ]]; then echo "s3_bucket not provided; EXITING;" exit 1 fi if [[ $s3_bucket == *[' !@#$%^&*()+']* ]]; then echo "s3_bucket should not contain spaces / any special character; EXITING" exit 1 fi - read -p "Provide onboarder s3 bucket region : " s3_region + read -r -p "Provide onboarder s3 bucket region : " s3_region if [[ $s3_region == *[' !@#$%^&*()+']* ]]; then echo "s3_region should not contain spaces / any special character; EXITING" exit 1 fi - read -p "Provide S3 URL : " s3_url + read -r -p "Provide S3 URL : " s3_url if [[ -z "$s3_url" ]]; then echo "s3_url not provided; EXITING;" exit 1 fi ... echo -e "\e[1mHave you completed the changes mentioned in the onboarding document? (y/n)\e[0m" - read answer + read -r answerAs per static analysis hints, Shellcheck flags these as SC2162.
Also applies to: 53-53, 65-65, 72-72, 107-107
🤖 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/install.sh` at line 33, The read prompts in deploy/install.sh are missing the -r flag, so backslashes in user input could be interpreted; update each interactive read invocation (the prompt lines using read like the one asking "Is values.yaml for onboarder chart set correctly..." and the other read calls at the same locations) to use read -r so input is read raw (e.g., change read "..." to read -r "...") for all occurrences flagged (the read calls at the lines corresponding to the prompts around the values.yaml check and the other prompts referenced).
60-63: ⚡ Quick winConsider more appropriate validation patterns for S3 parameters.
The current character validation pattern
*[' !@#$%^&*()+']*has a few concerns:
- For
s3_bucket: AWS S3 bucket names have specific rules (lowercase letters, numbers, hyphens, and periods only; 3-63 characters). The current pattern allows many characters that would fail S3 bucket creation.- For
s3_region: AWS regions follow a specific format (e.g.,us-east-1). The current pattern rejects hyphens, which are required.- Pattern inconsistency: The error message says "any special character" but only checks a subset.
💡 Proposed fix with more accurate validation
if [[ -z "$s3_bucket" ]]; then echo "s3_bucket not provided; EXITING;" exit 1 fi - if [[ $s3_bucket == *[' !@#$%^&*()+']* ]]; then - echo "s3_bucket should not contain spaces / any special character; EXITING" + if [[ ! "$s3_bucket" =~ ^[a-z0-9][a-z0-9.-]{1,61}[a-z0-9]$ ]]; then + echo "s3_bucket must be 3-63 characters, lowercase letters, numbers, hyphens, and periods only; EXITING" exit 1 fi read -p "Provide onboarder s3 bucket region : " s3_region - if [[ $s3_region == *[' !@#$%^&*()+']* ]]; then - echo "s3_region should not contain spaces / any special character; EXITING" + if [[ -n "$s3_region" && ! "$s3_region" =~ ^[a-z0-9-]+$ ]]; then + echo "s3_region should contain only lowercase letters, numbers, and hyphens; EXITING" exit 1 fiNote: If using MinIO or other S3-compatible storage with different naming rules, adjust the regex accordingly.
Also applies to: 67-70
🤖 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/install.sh` around lines 60 - 63, Replace the brittle glob check with proper regex validation for S3 names: validate s3_bucket using a regex enforcing S3 rules (3–63 chars, lowercase letters, numbers, hyphens and periods, cannot start or end with a hyphen/period) and check length separately (refer to variable s3_bucket), and validate s3_region with a region pattern like ^[a-z]{2}(-[a-z]+)*-\d+$ (refer to variable s3_region) so hyphens are allowed; also update the echo messages to state the exact validation rule that failed; apply the same corrected validation logic where s3_region/s3_bucket are currently checked (the blocks around the existing checks).
🤖 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 `@deploy/install.sh`:
- Around line 65-70: The s3_region validation currently only checks for special
characters but not emptiness; update the install.sh validation for the variable
s3_region to include an empty-string check consistent with s3_bucket and s3_url
validations (or add a clear comment if empty is intentionally allowed). Locate
the prompt/read for s3_region and the following conditional that uses the
pattern check, then add the same empty-check logic used for s3_bucket/s3_url (or
annotate intent) so that s3_region cannot be blank unless explicitly permitted.
- Around line 79-81: The script fetches s3_user_secret and s3_user_key via
kubectl but does not validate them; update the install.sh after the kubectl
commands to check that both s3_user_secret and s3_user_key are non-empty (and
not null/unset) and if either is empty print a clear error mentioning the
missing variable and exit non-zero before running Helm, referencing the
variables s3_user_secret and s3_user_key and the kubectl retrieval commands so
the validation runs immediately after those lines.
- Around line 119-126: Move the error-handling flags (set -e, set -o errexit,
set -o nounset, set -o errtrace, set -o pipefail) to immediately after the
script shebang so they apply to the entire script execution, and remove the
duplicate block currently placed just before the call to installing_onboarder;
ensure the flags are placed before any namespace creation, prompts or
environment setup so functions like installing_onboarder and any earlier logic
run under the strict error semantics.
- Line 90: The unquoted use of the shell variable ENABLE_INSECURE can cause word
splitting/globbing (SC2086); update the invocation that currently uses
$ENABLE_INSECURE so the flag is safely quoted as "$ENABLE_INSECURE" or,
preferably, change the pattern to conditionally append the flag only when
ENABLE_INSECURE is non-empty (e.g., test -n "$ENABLE_INSECURE" and then include
the flag) so that the code that references ENABLE_INSECURE is robust against
multi-word values.
In `@Dockerfile`:
- Around line 16-18: Pin the MinIO mc binary to a specific release and verify
its SHA256 checksum before making it executable: replace the current curl of the
floating URL that downloads /bin/mc with two downloads (the specific RELEASE.*
mc binary and its corresponding .sha256sum), run sha256sum -c against the
downloaded checksum, then chmod +x /bin/mc and run mc --version; reference the
mc binary download and the checksum verification step so the Dockerfile uses a
fixed release and validates integrity.
---
Nitpick comments:
In `@deploy/install.sh`:
- Line 33: The read prompts in deploy/install.sh are missing the -r flag, so
backslashes in user input could be interpreted; update each interactive read
invocation (the prompt lines using read like the one asking "Is values.yaml for
onboarder chart set correctly..." and the other read calls at the same
locations) to use read -r so input is read raw (e.g., change read "..." to read
-r "...") for all occurrences flagged (the read calls at the lines corresponding
to the prompts around the values.yaml check and the other prompts referenced).
- Around line 60-63: Replace the brittle glob check with proper regex validation
for S3 names: validate s3_bucket using a regex enforcing S3 rules (3–63 chars,
lowercase letters, numbers, hyphens and periods, cannot start or end with a
hyphen/period) and check length separately (refer to variable s3_bucket), and
validate s3_region with a region pattern like ^[a-z]{2}(-[a-z]+)*-\d+$ (refer to
variable s3_region) so hyphens are allowed; also update the echo messages to
state the exact validation rule that failed; apply the same corrected validation
logic where s3_region/s3_bucket are currently checked (the blocks around the
existing checks).
🪄 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: 0eb0260d-21aa-4124-853a-b51f76111521
📒 Files selected for processing (3)
Dockerfiledeploy/install.shhelm/partner-onboarder/values.yaml
Mahesh-Binayak
approved these changes
May 14, 2026
Signed-off-by: Abhishek S <127825992+abhishek8shankar@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@deploy/values.yaml`:
- Around line 11-26: Remove or guard all references to the deleted modules in
install/default.sh: delete the certificate upload block that targets "mobileid"
and remove the client ID extraction that looks for
"mpartner-default-demo-oidc-clientID" (or replace it with the current valid
client name if applicable); ensure any variables, conditional checks or function
calls tied to "mobileid" or "demo-oidc" are also removed or wrapped behind a
feature flag check so the script no longer assumes those modules exist during
onboarding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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.
Summary by CodeRabbit