-
Notifications
You must be signed in to change notification settings - Fork 78
[MOSIP-43631] updated helm charts #781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
WalkthroughUpdates Helm chart configuration for graceful pod shutdown with lifecycle hooks and termination grace period. Upgrades admin-UI container images and refactors uitest-admin Docker build to use Alpine Linux base with non-root user and updated package management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Signed-off-by: bhumi46 <thisisbn46@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
helm/admin-ui/values.yaml (1)
228-228: Minor: Typo in comment.Line 228 reads "Termination grace perios" but should be "Termination grace period".
Apply this diff to fix the typo:
-## Termination grace perios : the maximum amount of time (in seconds) Kubernetes will wait for a container to gracefully shut down +## Termination grace period: the maximum amount of time (in seconds) Kubernetes will wait for a container to gracefully shut downuitest-admin/Dockerfile (1)
47-49: Inconsistent COPY --chown syntax between commands.Line 47 uses numeric UID:GID (1001:1001) while lines 48–49 use username:group notation. While functionally equivalent, standardize for clarity and consistency.
Apply this diff to standardize on numeric UID:GID notation:
-COPY --chown=${container_user}:${container_user_group} ./src/main/resources/ ${work_dir}/resources/ -COPY --chown=${container_user_uid}:${container_user_gid} ./target/*.jar ${work_dir} +COPY --chown=${container_user_uid}:${container_user_gid} ./src/main/resources/ ${work_dir}/resources/ +COPY --chown=${container_user_uid}:${container_user_gid} ./target/*.jar ${work_dir}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
helm/admin-ui/.gitignore(1 hunks)helm/admin-ui/templates/deployment.yaml(1 hunks)helm/admin-ui/values.yaml(2 hunks)uitest-admin/Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: uitest-admin / build-dockers
- GitHub Check: build-admin-ui / npm-build
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (7)
helm/admin-ui/.gitignore (1)
2-2: Helm lock file exclusion follows best practices.Ignoring Chart.lock prevents unnecessary VCS noise from auto-generated lock files and reduces merge conflicts.
helm/admin-ui/values.yaml (2)
220-229: Graceful shutdown lifecycle is well-structured.The preStop sleep (30s) + terminationGracePeriodSeconds (60s) pattern provides orderly connection draining before force termination. This aligns with best practices for stateful service shutdown.
328-329: I need to verify the image existence using alternative methods since the API endpoint returned authentication error.Let me search for more specific information about the mosipid organization:
Let me search for more context about the MOSIP project and whether "mosipid" is a legitimate source:
Let me search for MOSIP project documentation to understand the context better:
Verify volumePermissions image availability before deploying.
The image reference has been changed to
mosipid/os-shell:12-debian-12-r46. While the mosipid Docker Hub organization exists, the specific imagemosipid/os-shell:12-debian-12-r46could not be verified to be publicly available. The Docker Hub API returned authentication errors, and this image does not appear in public documentation. Ensure:
- This image exists in your registry or is publicly accessible
- It is compatible with the previous
bitnami/bitnami-shell:10usage- If this is a private or internal image, proper pull credentials are configured in the Kubernetes deployment
helm/admin-ui/templates/deployment.yaml (1)
36-36: Termination grace period properly parameterized.The template correctly sources terminationGracePeriodSeconds from values with a sensible default, aligning with the lifecycle preStop hook defined in values.yaml. Pod spec placement is correct.
uitest-admin/Dockerfile (3)
5-23: Alpine migration is clean; kubectl fetch is robust.Switching to Alpine with apk and dynamic kubectl stable fetch is a solid optimization. Package list looks complete for testing (chromium, xvfb, fonts). Cleanup of temp kubectl version file is good.
34-45: Non-root user creation follows Alpine best practices.The addgroup/adduser flow with explicit UID/GID is the correct approach for Alpine and improves security posture.
51-53: The code is correctly implemented; no issues found.The verification confirms that the original review comment's concerns are unfounded:
- entrypoint.sh exists: Found at
uitest-admin/entrypoint.shin the build context.- Proper COPY: Line 47 explicitly copies
./entrypoint.shto${work_dir}/entrypoint.shwith--chmod=771, ensuring it is executable.- Path resolution: WORKDIR is set to
/home/${container_user}/(line 51) and the ENTRYPOINT invokes./entrypoint.sh(line 53), which correctly resolves to the copied file location.The entrypoint is present, executable, and will be found at runtime. No action needed.
Likely an incorrect or invalid review comment.
Summary by CodeRabbit
Chores
Infrastructure Updates