Conversation
vladsha-dev
requested review from
Innavin369,
VadimKovalenkoSNF,
mazursasha1990 and
roman-stolar
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 27.74%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple workflow files and scripts to improve the management of OpenSearch and Logstash services during deployment. Key changes include the renaming of steps for clarity, the addition of a new job to start Logstash after clearing OpenSearch indexes, and enhanced handling of environment variables. Input options for deployment environments have been expanded, and descriptions have been updated for better specificity. Additionally, scripts have been modified to improve functionality, including the deletion of custom OpenSearch templates and enhanced SSH handling. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)doc/release/RELEASE-NOTES.md (1)
The fix for clearing OpenSearch indexes looks good, but let's verify the implementation to ensure proper cleanup during deployment. Run the following script to verify the template cleanup implementation: ✅ Verification successfulOpenSearch index template cleanup implementation is verified The implementation in
The fix aligns with the release notes which mention switching from legacy 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify that both indexes and templates are properly deleted during cleanup.
# Test: Search for the clear_opensearch.sh.tpl script and verify it includes template deletion.
rg -A 5 "clear_opensearch.sh.tpl"
# Test: Search for any OpenSearch template deletion commands.
rg -A 5 "_template|_index_template"
Length of output: 23646 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
deployment/clear_opensearch/run.sh.tpl (1)
Line range hint
1-11: Add error handling and environment variable validationWhile the SSH setup is secure, the script needs additional error handling and validation:
#!/bin/bash + +# Exit on error +set -e + +# Validate required environment variables +if [ -z "$BASTION_IP" ]; then + echo "Error: BASTION_IP environment variable is not set" + exit 1 +fi chmod 700 /root/.ssh chmod 600 /root/.ssh/id_rsa chmod 600 /root/.ssh/config chown -R root:root /root/.ssh chmod +x /script/clear_opensearch.sh + +# Check if SSH setup was successful +if [ ! -f "/root/.ssh/id_rsa" ] || [ ! -f "/root/.ssh/config" ]; then + echo "Error: SSH setup failed - missing required files" + exit 1 +fi + echo -e "\nCopy script to Bastion host\n" -scp /script/clear_opensearch.sh ec2-user@$BASTION_IP: +scp /script/clear_opensearch.sh ec2-user@$BASTION_IP: || { echo "Error: Failed to copy script to Bastion host"; exit 1; } echo -e "\nRun cleaning OpenSearch indexes\n" -ssh ec2-user@$BASTION_IP ./clear_opensearch.sh +ssh ec2-user@$BASTION_IP ./clear_opensearch.sh || { echo "Error: Failed to execute script on Bastion host"; exit 1; }deployment/clear_opensearch/clear_opensearch.sh.tpl (1)
Line range hint
22-26: Add proper cleanup for EFS mount pointEnsure EFS is properly unmounted even if the script fails.
+# Create mount point +MOUNT_POINT="/mnt" + +# Cleanup function +cleanup() { + if mountpoint -q "$MOUNT_POINT"; then + echo "Unmounting EFS..." + sudo umount "$MOUNT_POINT" || echo "Warning: Failed to unmount EFS" + fi +} + +# Set cleanup to run on script exit +trap cleanup EXIT + echo -e "\nRemove the JDBC input lock files from the EFS storage connected to Logstash\n" -sudo mount -t efs -o tls,accesspoint=$EFS_AP_ID $EFS_ID:/ /mnt -sudo rm /mnt/production_locations_jdbc_last_run -sudo rm /mnt/moderation_events_jdbc_last_run -sudo umount /mnt + +# Validate EFS variables +if [ -z "$EFS_AP_ID" ] || [ -z "$EFS_ID" ]; then + echo "Error: Required EFS variables not set" + exit 1 +fi + +# Mount EFS +sudo mount -t efs -o tls,accesspoint=$EFS_AP_ID $EFS_ID:/ "$MOUNT_POINT" || { echo "Error: Failed to mount EFS"; exit 1; } + +# Remove lock files +for lock_file in "production_locations_jdbc_last_run" "moderation_events_jdbc_last_run"; do + if [ -f "$MOUNT_POINT/$lock_file" ]; then + sudo rm "$MOUNT_POINT/$lock_file" || echo "Warning: Failed to remove $lock_file" + fi +done.github/workflows/db_apply_anonimized.yml (3)
Line range hint
119-133: Add timeout and error handling for OpenSearch operationsThe step that clears OpenSearch indexes and templates should have a timeout and retry mechanism.
- name: Clear the custom OpenSearch indexes and templates for ${{ vars.ENV_NAME }} + timeout-minutes: 10 + continue-on-error: false + retries: 2 env: AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} OPENSEARCH_DOMAIN: ${{ steps.export_variables.outputs.OPENSEARCH_DOMAIN }} EFS_AP_ID: ${{ steps.export_variables.outputs.EFS_AP_ID }} EFS_ID: ${{ steps.export_variables.outputs.EFS_ID }} BASTION_IP: ${{ vars.BASTION_IP }} run: | + # Validate required outputs + if [ -z "$OPENSEARCH_DOMAIN" ] || [ -z "$EFS_AP_ID" ] || [ -z "$EFS_ID" ]; then + echo "Error: Required variables not exported from previous step" + exit 1 + fi
Line range hint
134-146: Improve SSH key handling securityThe SSH key handling could be more secure.
- mkdir -p script - mkdir -p ssh - echo -e "Host *\n\tStrictHostKeyChecking no\n\n" > ssh/config - printf "%s\n" "${{ secrets.SSH_PRIVATE_KEY }}" > ssh/id_rsa - echo "" >> ssh/id_rsa + # Create directories with restricted permissions + mkdir -m 700 -p script ssh + + # Create SSH config with specific host + cat > ssh/config <<EOF + Host ${{ vars.BASTION_IP }} + StrictHostKeyChecking no + UserKnownHostsFile /dev/null + IdentityFile /root/.ssh/id_rsa + EOF + + # Write SSH key with proper permissions + install -m 600 /dev/null ssh/id_rsa + printf "%s\n" "${{ secrets.SSH_PRIVATE_KEY }}" > ssh/id_rsa
Line range hint
147-157: Add health check after starting LogstashThe workflow should verify that Logstash is healthy after starting.
- name: Start Logstash for ${{ vars.ENV_NAME }} + timeout-minutes: 5 env: AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} AWS_DEFAULT_REGION: "eu-west-1" run: | - aws \ + # Start Logstash + aws --output json \ ecs update-service --desired-count 1 --cluster=ecsOpenSupplyHub${{vars.ENV_NAME}}Cluster \ --service=OpenSupplyHub${{vars.ENV_NAME}}AppLogstash + + # Wait for service to stabilize + aws ecs wait services-stable \ + --cluster ecsOpenSupplyHub${{vars.ENV_NAME}}Cluster \ + --services OpenSupplyHub${{vars.ENV_NAME}}AppLogstash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.github/workflows/db_apply_anonimized.yml(1 hunks).github/workflows/deploy_to_aws.yml(2 hunks).github/workflows/release-deploy.yml(1 hunks)deployment/clear_opensearch/clear_opensearch.sh.tpl(1 hunks)deployment/clear_opensearch/run.sh.tpl(1 hunks)doc/release/RELEASE-NOTES.md(2 hunks)doc/release/RELEASE-PROTOCOL.md(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/release-deploy.yml
🧰 Additional context used
🪛 LanguageTool
doc/release/RELEASE-PROTOCOL.md
[grammar] ~162-~162: “Person” is a singular noun. It appears that the verb form is incorrect.
Context: ... environment. If the responsible person need to clear custom OpenSearch indexes and ...
(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
[uncategorized] ~178-~178: A comma is probably missing here.
Context: ...r preparing the fix, merge it back. For production you have to make active the `disable_li...
(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
🔇 Additional comments (6)
deployment/clear_opensearch/clear_opensearch.sh.tpl (1)
3-13: Documentation is clear and comprehensive
The comment block effectively explains the script's purpose and flexibility.
doc/release/RELEASE-PROTOCOL.md (2)
115-120: LGTM: Clear instructions for OpenSearch clearing during deployment
The added instructions clearly explain the process of clearing custom OpenSearch indexes and templates during deployment using the new checkbox option.
Line range hint 162-178: LGTM: Comprehensive hotfix deployment instructions
The instructions for hotfix deployments have been updated to include the OpenSearch clearing process, maintaining consistency with the regular deployment process.
🧰 Tools
🪛 LanguageTool
[typographical] ~159-~159: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...g days before its scheduled date and in 1-2 hours to prevent any actions on the env...
(HYPHEN_TO_EN)
[grammar] ~160-~160: The verb form ‘have’ does not appear to fit in this context.
Context: ...ment is carried out. 2. The responsible person have to take db snapshot manually via Amazon...
(SINGULAR_NOUN_VERB_AGREEMENT)
[grammar] ~162-~162: “Person” is a singular noun. It appears that the verb form is incorrect.
Context: ... environment. If the responsible person need to clear custom OpenSearch indexes and ...
(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
🪛 Markdownlint (0.35.0)
161-161: null
Link fragments should be valid
(MD051, link-fragments)
.github/workflows/deploy_to_aws.yml (2)
33-33: LGTM: Clear description for OpenSearch clearing input
The input description has been updated to accurately reflect that both custom OpenSearch indexes and templates will be cleared.
416-416: LGTM: Accurate step name for OpenSearch clearing
The step name has been updated to accurately reflect that both custom OpenSearch indexes and templates will be cleared.
doc/release/RELEASE-NOTES.md (1)
52-53: LGTM: Clear documentation of the OpenSearch clearing bugfix
The release notes clearly explain:
- The root cause: templates weren't being cleared along with indexes
- The impact: conflicts preventing updates from being applied
- The solution: adding template deletion to the clearing process
VadimKovalenkoSNF
left a comment
VadimKovalenkoSNF
left a comment
There was a problem hiding this comment.
Good work! Approved.
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
vladsha-dev
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
|
3 participants
It was found that clearing OpenSearch indexes didn’t work properly because the templates weren’t cleared. After updating the index mappings within the index template files, the index template remained unchanged because only the indexes were deleted during deployment, not both the indexes and their templates. This caused conflicts and prevented developers' updates from being applied to the OpenSearch indexes.
This issue has been fixed by adding additional requests to delete the appropriate index templates to the
clear_opensearch.sh.tplscript, which is triggered when clearing OpenSearch during deployment to any environment.