chore: automate ballerine deployment #2871
Conversation
|
WalkthroughThe changes introduced in this pull request enhance the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 6
🧹 Outside diff range and nitpick comments (2)
ballerine_install.sh (2)
21-35: Input validation incheck_http_https()is too restrictive.The function
check_http_https()exits if the input does not contain 'http://' or 'https://'. Users might provide domains without protocols (e.g., 'example.com'), causing the script to exit unexpectedly. Consider allowing inputs without protocols and defaulting to 'http://' or 'https://' as needed.Apply this diff to adjust the input validation:
check_http_https() { local input=$1 echo "checking domain if suitable $input" - if [[ $input == *http://* && $input != *https://* ]]; then - echo "The string contains 'http' but not 'https'." - elif [[ $input == *https://* && $input != *http://* ]]; then - echo "The string contains 'https' but not 'http'." - elif [[ $input == *http://* && $input == *https://* ]]; then - echo "The string contains both 'http' and 'https'." + if [[ $input == *http://* || $input == *https://* ]]; then + echo "The domain contains protocol information." else - echo "The string contains neither 'http' nor 'https'." - exit 1; + echo "No protocol specified. Defaulting to 'https://'." + input="https://$input" fi }
157-157: Variable 'VERBOSE' is set but not used.The
VERBOSEvariable is assigned a value but not utilized anywhere in the script. Consider implementing verbose logging or removing the variable if it's unnecessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ballerine_install.sh(1 hunks)deploy/docker-compose-build.yml(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
ballerine_install.sh
[warning] 156-156: VERBOSE appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (1)
deploy/docker-compose-build.yml (1)
101-101: Adding restart: on-failure to PostgreSQL service enhances reliability.
Including a restart policy ensures that the ballerine-postgres service will automatically restart if it fails, improving the robustness of your application.
ballerine_install.sh
Outdated
| echo "The operating system is not recognized." | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
Script continues after failing to recognize the operating system.
When the operating system is not recognized, the script should exit to prevent unintended behavior.
Apply this diff to exit the script when the OS is unrecognized:
echo "The operating system is not recognized."
+exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "The operating system is not recognized." | |
| ;; | |
| esac | |
| echo "The operating system is not recognized." | |
| exit 1 | |
| ;; | |
| esac |
ballerine_install.sh
Outdated
| echo "The host is running a Linux distribution but not Ubuntu." | ||
| echo "We do not support this Linux distribution" | ||
| fi |
There was a problem hiding this comment.
Script does not exit on unsupported Linux distributions.
After detecting an unsupported Linux distribution, the script continues execution. This may lead to errors later on. It's essential to exit the script when the OS is unsupported.
Apply this diff to exit the script for unsupported Linux distributions:
echo "We do not support this Linux distribution"
+exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "The host is running a Linux distribution but not Ubuntu." | |
| echo "We do not support this Linux distribution" | |
| fi | |
| echo "The host is running a Linux distribution but not Ubuntu." | |
| echo "We do not support this Linux distribution" | |
| exit 1 | |
| fi |
ballerine_install.sh
Outdated
| sed -i '' "s|http://localhost:3000|$VITE_DOMAIN_NAME|g" $i | ||
| done |
There was a problem hiding this comment.
Incompatible sed -i usage across different operating systems.
The sed -i '' command is specific to macOS. On Linux systems like Ubuntu, the syntax should be sed -i without the empty quotes. This difference can cause the script to fail on Ubuntu.
Apply this diff to make sed command compatible across macOS and Linux:
for i in $env_files;
do
echo "Updating env variables of $i"
- sed -i '' "s|http://localhost:3000|$VITE_DOMAIN_NAME|g" $i
+ if [[ "$OSTYPE" == "darwin"* ]]; then
+ sed -i '' "s|http://localhost:3000|$VITE_DOMAIN_NAME|g" $i
+ else
+ sed -i "s|http://localhost:3000|$VITE_DOMAIN_NAME|g" $i
+ fi
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sed -i '' "s|http://localhost:3000|$VITE_DOMAIN_NAME|g" $i | |
| done | |
| for i in $env_files; | |
| do | |
| echo "Updating env variables of $i" | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| sed -i '' "s|http://localhost:3000|$VITE_DOMAIN_NAME|g" $i | |
| else | |
| sed -i "s|http://localhost:3000|$VITE_DOMAIN_NAME|g" $i | |
| fi | |
| done |
ballerine_install.sh
Outdated
| echo "Updating env variables for KYB in $i" | ||
| sed -i '' "s|http://localhost:5201|$KYB_DOMAIN|g" $i | ||
|
|
||
| echo "Updating env variables for Workflow Dashboard in $i" | ||
| sed -i '' "s|http://localhost:5200|$WORKFLOW_DASHBOARD_DOMAIN|g" $i | ||
|
|
||
| install_docker | ||
| echo "Updating env variables for Backoffice in $i" | ||
| sed -i '' "s|http://localhost:5137|$BACKOFFICE_DOMAIN|g" $i |
There was a problem hiding this comment.
sed -i command may fail due to platform differences.
The use of sed -i '' in updating Docker Compose files is platform-specific to macOS. This will cause errors on Linux systems. Adjust the script to handle both macOS and Linux.
Apply this diff to update the sed commands:
for i in $env_files;
do
echo "Updating env variables for KYB in $i"
- sed -i '' "s|http://localhost:5201|$KYB_DOMAIN|g" $i
+ if [[ "$OSTYPE" == "darwin"* ]]; then
+ sed -i '' "s|http://localhost:5201|$KYB_DOMAIN|g" $i
+ else
+ sed -i "s|http://localhost:5201|$KYB_DOMAIN|g" $i
+ fi
echo "Updating env variables for Workflow Dashboard in $i"
- sed -i '' "s|http://localhost:5200|$WORKFLOW_DASHBOARD_DOMAIN|g" $i
+ if [[ "$OSTYPE" == "darwin"* ]]; then
+ sed -i '' "s|http://localhost:5200|$WORKFLOW_DASHBOARD_DOMAIN|g" $i
+ else
+ sed -i "s|http://localhost:5200|$WORKFLOW_DASHBOARD_DOMAIN|g" $i
+ fi
echo "Updating env variables for Backoffice in $i"
- sed -i '' "s|http://localhost:5137|$BACKOFFICE_DOMAIN|g" $i
+ if [[ "$OSTYPE" == "darwin"* ]]; then
+ sed -i '' "s|http://localhost:5137|$BACKOFFICE_DOMAIN|g" $i
+ else
+ sed -i "s|http://localhost:5137|$BACKOFFICE_DOMAIN|g" $i
+ fi
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Updating env variables for KYB in $i" | |
| sed -i '' "s|http://localhost:5201|$KYB_DOMAIN|g" $i | |
| echo "Updating env variables for Workflow Dashboard in $i" | |
| sed -i '' "s|http://localhost:5200|$WORKFLOW_DASHBOARD_DOMAIN|g" $i | |
| install_docker | |
| echo "Updating env variables for Backoffice in $i" | |
| sed -i '' "s|http://localhost:5137|$BACKOFFICE_DOMAIN|g" $i | |
| echo "Updating env variables for KYB in $i" | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| sed -i '' "s|http://localhost:5201|$KYB_DOMAIN|g" $i | |
| else | |
| sed -i "s|http://localhost:5201|$KYB_DOMAIN|g" $i | |
| fi | |
| echo "Updating env variables for Workflow Dashboard in $i" | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| sed -i '' "s|http://localhost:5200|$WORKFLOW_DASHBOARD_DOMAIN|g" $i | |
| else | |
| sed -i "s|http://localhost:5200|$WORKFLOW_DASHBOARD_DOMAIN|g" $i | |
| fi | |
| echo "Updating env variables for Backoffice in $i" | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| sed -i '' "s|http://localhost:5137|$BACKOFFICE_DOMAIN|g" $i | |
| else | |
| sed -i "s|http://localhost:5137|$BACKOFFICE_DOMAIN|g" $i | |
| fi |
| @@ -23,7 +23,7 @@ services: | |||
| - ballerine-workflow-service | |||
| restart: on-failure | |||
| environment: | |||
| VITE_API_URL: 'http://${DOMAIN_NAME:-localhost:3000}/api/v1/' | |||
| VITE_API_URL: 'http://localhost:3000/api/v1/' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoding VITE_API_URL to localhost may limit flexibility.
Setting VITE_API_URL to 'http://localhost:3000/api/v1/' restricts the application to only work with a local API server. In production environments, the API might be hosted on a different domain. Consider making this URL configurable.
Apply this diff to restore configurability:
VITE_API_URL: 'http://localhost:3000/api/v1/'
+# Consider using an environment variable or a Docker argument to set VITE_API_URL dynamically.Committable suggestion skipped: line range outside the PR's diff.
| BACKOFFICE_CORS_ORIGIN: http://localhost:5137 | ||
| WORKFLOW_DASHBOARD_CORS_ORIGIN: http://localhost:5200 | ||
| PORT: ${WORKFLOW_SVC_PORT} | ||
| KYB_EXAMPLE_CORS_ORIGIN: http://${DOMAIN_NAME:-localhost}:${KYB_APP_PORT} | ||
| KYB_EXAMPLE_CORS_ORIGIN: http://localhost:5201 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoding CORS origins may cause cross-origin issues.
By hardcoding the CORS origins to localhost, services running on other domains won't be able to communicate with the API. This can cause issues in staging or production environments.
Apply this diff to make the CORS origins configurable:
BACKOFFICE_CORS_ORIGIN: http://localhost:5137
WORKFLOW_DASHBOARD_CORS_ORIGIN: http://localhost:5200
KYB_EXAMPLE_CORS_ORIGIN: http://localhost:5201
+# Consider using environment variables to set these origins dynamically based on deployment.Committable suggestion skipped: line range outside the PR's diff.
pratapalakshmi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
deploy/docker-compose-build-https.yml (2)
31-32: Consider multi-stage builds and build argumentsWhile building from local context is good for development, consider:
- Using multi-stage builds to optimize the final image
- Adding build arguments for flexibility
Example enhancement:
build: context: ../services/workflows-service/ + args: + NODE_ENV: ${NODE_ENV:-production} + target: ${BUILD_TARGET:-production}
Line range hint
1-116: Consider maintaining environment-based configurationThe current changes show a trend towards hardcoding values that were previously configurable through environment variables. While this might simplify initial setup, it could:
- Reduce deployment flexibility across different environments
- Make it harder to maintain different configurations for development, staging, and production
- Potentially expose sensitive information in the configuration
Consider maintaining the environment-based configuration approach and providing clear documentation or default values in a
.env.examplefile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
deploy/docker-compose-build-https.yml(4 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
deploy/docker-compose-build-https.yml
[error] 114-114: duplication of key "restart" in mapping
(key-duplicates)
| @@ -97,6 +98,7 @@ services: | |||
| timeout: 45s | |||
| interval: 10s | |||
| retries: 10 | |||
| restart: on-failure | |||
There was a problem hiding this comment.
Fix duplicate restart policy in caddy service
The caddy service has conflicting restart policies:
restart: unless-stopped(line 103)restart: on-failure(line 114)
Remove the duplicate restart policy and decide on a single strategy:
caddy:
image: caddy:latest
restart: unless-stopped
container_name: caddy
ports:
- 80:80
- 443:443
volumes:
- "../deploy/caddy/Caddyfile:/etc/caddy/Caddyfile"
- "../deploy/./caddy/site:/srv"
- "../deploy/caddy/caddy_data:/data"
- "../deploy/caddy/caddy_config:/config"
- restart: on-failureAlso applies to: 114-114
| @@ -23,12 +23,13 @@ services: | |||
| - ballerine-workflow-service | |||
| restart: on-failure | |||
| environment: | |||
| VITE_API_URL: 'http://${DOMAIN_NAME:-localhost:3000}/api/v1/' | |||
| VITE_API_URL: 'http://localhost:3000/api/v1/' | |||
There was a problem hiding this comment.
Avoid hardcoding localhost URLs in Docker configurations
Hardcoding localhost:3000 reduces deployment flexibility. Consider keeping the parameterized version using environment variables to support different deployment environments.
- VITE_API_URL: 'http://localhost:3000/api/v1/'
+ VITE_API_URL: 'http://${DOMAIN_NAME:-localhost:3000}/api/v1/'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| VITE_API_URL: 'http://localhost:3000/api/v1/' | |
| VITE_API_URL: 'http://${DOMAIN_NAME:-localhost:3000}/api/v1/' |
| BACKOFFICE_CORS_ORIGIN: http://localhost:5137 | ||
| WORKFLOW_DASHBOARD_CORS_ORIGIN: http://localhost:5200 | ||
| PORT: ${WORKFLOW_SVC_PORT} | ||
| KYB_EXAMPLE_CORS_ORIGIN: http://${DOMAIN_NAME:-localhost}:${KYB_APP_PORT} | ||
| KYB_EXAMPLE_CORS_ORIGIN: http://localhost:5201 |
There was a problem hiding this comment.
Revise CORS configuration for security and flexibility
- Hardcoding CORS origins to localhost limits deployment flexibility
- Consider security implications of CORS configuration in production
- BACKOFFICE_CORS_ORIGIN: http://localhost:5137
- WORKFLOW_DASHBOARD_CORS_ORIGIN: http://localhost:5200
- KYB_EXAMPLE_CORS_ORIGIN: http://localhost:5201
+ BACKOFFICE_CORS_ORIGIN: http://${DOMAIN_NAME:-localhost}:${BACKOFFICE_PORT:-5137}
+ WORKFLOW_DASHBOARD_CORS_ORIGIN: http://${DOMAIN_NAME:-localhost}:${WORKFLOW_DASHBOARD_PORT:-5200}
+ KYB_EXAMPLE_CORS_ORIGIN: http://${DOMAIN_NAME:-localhost}:${KYB_APP_PORT:-5201}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BACKOFFICE_CORS_ORIGIN: http://localhost:5137 | |
| WORKFLOW_DASHBOARD_CORS_ORIGIN: http://localhost:5200 | |
| PORT: ${WORKFLOW_SVC_PORT} | |
| KYB_EXAMPLE_CORS_ORIGIN: http://${DOMAIN_NAME:-localhost}:${KYB_APP_PORT} | |
| KYB_EXAMPLE_CORS_ORIGIN: http://localhost:5201 | |
| BACKOFFICE_CORS_ORIGIN: http://${DOMAIN_NAME:-localhost}:${BACKOFFICE_PORT:-5137} | |
| WORKFLOW_DASHBOARD_CORS_ORIGIN: http://${DOMAIN_NAME:-localhost}:${WORKFLOW_DASHBOARD_PORT:-5200} | |
| PORT: ${WORKFLOW_SVC_PORT} | |
| KYB_EXAMPLE_CORS_ORIGIN: http://${DOMAIN_NAME:-localhost}:${KYB_APP_PORT:-5201} |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
ballerine_install.sh (2)
1-5: Enhance script initialization with additional safety flagsAdd
set -uto catch undefined variables and consider adding a script description.#!/bin/bash -set -e +set -eu + +# Ballerine Installation Script +# Version: 1.0 +# Description: Automates the deployment of Ballerine services echo "Running as: $(id)"
132-155: Extract hardcoded URLs as constantsHardcoded URLs should be defined as constants at the beginning of the script for better maintainability.
#!/bin/bash + +# Default service URLs +readonly DEFAULT_WORKFLOW_SERVICE_URL="http://localhost:3000" +readonly DEFAULT_BACKOFFICE_URL="http://localhost:5137" +readonly DEFAULT_WORKFLOW_DASHBOARD_URL="http://localhost:5200" +readonly DEFAULT_KYB_URL="http://localhost:5201" update_docker_compose(){ echo "Updating Docker Compose..." WORKFLOW_SERVICE_DOMAIN=$1 read -p "Enter the backoffice domain: " BACKOFFICE_DOMAIN - check_http_https $BACKOFFICE_DOMAIN + validate_domain_url "$BACKOFFICE_DOMAIN" read -p "Enter the workflow dashboard domain: " WORKFLOW_DASHBOARD_DOMAIN - check_http_https $WORKFLOW_DASHBOARD_DOMAIN + validate_domain_url "$WORKFLOW_DASHBOARD_DOMAIN" read -p "Enter the kyb domain: " KYB_DOMAIN - check_http_https $KYB_DOMAIN + validate_domain_url "$KYB_DOMAIN" for i in $env_files; do echo "Updating env variables for KYB in $i" - sed -i '' "s|http://localhost:5201|$KYB_DOMAIN|g" $i + sed -i '' "s|$DEFAULT_KYB_URL|$KYB_DOMAIN|g" $i
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ballerine_install.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
ballerine_install.sh
[warning] 208-208: VERBOSE appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (2)
ballerine_install.sh (2)
84-112: Script must exit on unsupported operating systems
The script continues execution after detecting unsupported operating systems, which could lead to failures.
115-127: Make sed commands compatible across platforms
ballerine_install.sh
Outdated
| install_docker_ubuntu(){ | ||
| echo "Installing docker..." | ||
| # Add Docker's official GPG key: | ||
| sudo apt-get update | ||
| sudo apt-get install ca-certificates curl | ||
| sudo install -m 0755 -d /etc/apt/keyrings | ||
| sudo curl -fsSL https://download.docker.com/linux/ubuntu/gpg -o /etc/apt/keyrings/docker.asc | ||
| sudo chmod a+r /etc/apt/keyrings/docker.asc | ||
|
|
||
| # Add the repository to Apt sources: | ||
| echo \ | ||
| "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.asc] https://download.docker.com/linux/ubuntu \ | ||
| $(. /etc/os-release && echo "$VERSION_CODENAME") stable" | \ | ||
| sudo tee /etc/apt/sources.list.d/docker.list > /dev/null | ||
| sudo apt-get update | ||
| sudo apt-get install docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin | ||
| } | ||
|
|
||
|
|
||
| install_docker_macos(){ | ||
| echo "Install docker using the following docs" | ||
| echo "https://docs.docker.com/desktop/setup/install/mac-install/" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Docker installation security and functionality
The Docker installation functions need improvements in security and functionality:
- Add GPG key verification
- Pin Docker package versions
- Implement actual macOS installation
install_docker_ubuntu(){
echo "Installing docker..."
+
+ # Verify Docker's GPG key
+ DOCKER_GPG_FINGERPRINT="9DC858229FC7DD38854AE2D88D81803C0EBFCD88"
+
# Add Docker's official GPG key:
sudo apt-get update
sudo apt-get install ca-certificates curl
sudo install -m 0755 -d /etc/apt/keyrings
sudo curl -fsSL https://download.docker.com/linux/ubuntu/gpg -o /etc/apt/keyrings/docker.asc
+
+ # Verify fingerprint
+ if ! gpg --dry-run --quiet --import --import-options import-show /etc/apt/keyrings/docker.asc | grep -q "$DOCKER_GPG_FINGERPRINT"; then
+ echo "Error: Docker GPG key verification failed"
+ exit 1
+ fi
+
sudo chmod a+r /etc/apt/keyrings/docker.asc
# Add the repository to Apt sources:
echo \
"deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.asc] https://download.docker.com/linux/ubuntu \
$(. /etc/os-release && echo "$VERSION_CODENAME") stable" | \
sudo tee /etc/apt/sources.list.d/docker.list > /dev/null
sudo apt-get update
- sudo apt-get install docker-ce docker-ce-cli containerd.io docker-buildx-plugin docker-compose-plugin
+ # Pin specific versions for reproducibility
+ sudo apt-get install \
+ docker-ce=5:24.0.0-1~ubuntu.22.04~jammy \
+ docker-ce-cli=5:24.0.0-1~ubuntu.22.04~jammy \
+ containerd.io=1.6.27-1 \
+ docker-buildx-plugin=0.12.0-1~ubuntu.22.04~jammy \
+ docker-compose-plugin=2.24.0-1~ubuntu.22.04~jammy
}
install_docker_macos(){
- echo "Install docker using the following docs"
- echo "https://docs.docker.com/desktop/setup/install/mac-install/"
+ echo "Installing Docker Desktop for macOS..."
+
+ # Check if Homebrew is installed
+ if ! command -v brew >/dev/null 2>&1; then
+ echo "Error: Homebrew is required but not installed"
+ echo "Install from https://brew.sh"
+ exit 1
+ fi
+
+ # Install Docker Desktop
+ if ! brew install --cask docker; then
+ echo "Error: Failed to install Docker Desktop"
+ exit 1
+ fi
+
+ echo "Docker Desktop installed. Please start Docker Desktop from Applications"
}Committable suggestion skipped: line range outside the PR's diff.
| # Parse arguments | ||
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| -h|--help) | ||
| show_help | ||
| exit 0 | ||
| ;; | ||
| -d|--domain) | ||
| if [ -n "$2" ]; then | ||
| VITE_DOMAIN_NAME="$2" | ||
| echo "VITE DOMAIN: $VITE_DOMAIN_NAME" | ||
| check_http_https $VITE_DOMAIN_NAME | ||
| update_frontend_build_variables $VITE_DOMAIN_NAME | ||
| update_docker_compose $VITE_DOMAIN_NAME | ||
| shift 2 | ||
| else | ||
| echo "Error: --domain requires a domain name." | ||
| exit 1 | ||
| fi | ||
| ;; | ||
| -v|--verbose) | ||
| VERBOSE=true | ||
| echo "Verbose mode enabled." | ||
| shift | ||
| ;; | ||
| *) | ||
| echo "Unknown option: $1" | ||
| echo "Use -h or --help for usage information." | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
Address unused VERBOSE flag and improve argument validation
The VERBOSE flag is set but never used, and domain validation should happen before assignment.
+# Logging function
+log_verbose() {
+ if [ "${VERBOSE:-false}" = true ]; then
+ echo "[DEBUG] $*"
+ fi
+}
+
while [[ $# -gt 0 ]]; do
case "$1" in
-d|--domain)
if [ -n "$2" ]; then
+ # Validate domain before assignment
+ if ! validate_domain_url "$2"; then
+ exit 1
+ fi
VITE_DOMAIN_NAME="$2"
- echo "VITE DOMAIN: $VITE_DOMAIN_NAME"
- check_http_https $VITE_DOMAIN_NAME
+ log_verbose "VITE DOMAIN: $VITE_DOMAIN_NAME"
update_frontend_build_variables $VITE_DOMAIN_NAME
update_docker_compose $VITE_DOMAIN_NAME
shift 2Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 208-208: VERBOSE appears unused. Verify use (or export if used externally).
(SC2034)
ballerine_install.sh
Outdated
| deploy_ballerine() { | ||
| local input=$1 | ||
| echo "checking domain if suitable $input" | ||
| if [[ $input == *http://* && $input != *https://* ]]; then | ||
| cd deploy; sudo docker-compose -f docker-compose-build.yml up -d | ||
| elif [[ $input == *https://* && $input != *http://* ]]; then | ||
| cd deploy; sudo docker-compose -f docker-compose-build-https.yml up -d | ||
| elif [[ $input == *http://* && $input == *https://* ]]; then | ||
| echo "The string contains both 'http' and 'https'." | ||
| exit 1; | ||
| else | ||
| echo "The string contains neither 'http' nor 'https'." | ||
| exit 1; | ||
| fi | ||
| } |
There was a problem hiding this comment.
Critical: Refactor deployment function to improve security and reliability
The function has several issues that need to be addressed:
- Duplicates domain validation logic
- Uses sudo without checking permissions
- Lacks error handling for docker-compose
deploy_ballerine() {
local input=$1
- echo "checking domain if suitable $input"
- if [[ $input == *http://* && $input != *https://* ]]; then
- cd deploy; sudo docker-compose -f docker-compose-build.yml up -d
- elif [[ $input == *https://* && $input != *http://* ]]; then
- cd deploy; sudo docker-compose -f docker-compose-build-https.yml up -d
- elif [[ $input == *http://* && $input == *https://* ]]; then
- echo "The string contains both 'http' and 'https'."
- exit 1;
- else
- echo "The string contains neither 'http' nor 'https'."
- exit 1;
- fi
+
+ # Check if script is run with sudo
+ if [ "$EUID" -ne 0 ]; then
+ echo "Error: This script must be run with sudo privileges"
+ exit 1
+ fi
+
+ # Reuse existing domain validation
+ validate_domain_url "$input"
+
+ # Select compose file based on protocol
+ local compose_file
+ if [[ $input == *https://* ]]; then
+ compose_file="docker-compose-build-https.yml"
+ else
+ compose_file="docker-compose-build.yml"
+ fi
+
+ # Deploy with error handling
+ cd deploy || exit 1
+ if ! docker-compose -f "$compose_file" up -d; then
+ echo "Error: Failed to deploy Ballerine services"
+ exit 1
+ fi
}Committable suggestion skipped: line range outside the PR's diff.
ballerine_install.sh
Outdated
| create_caddy_file(){ | ||
| echo "Creating Caddy file..." | ||
| BACKOFFICE_DOMAIN=$1 | ||
| WORKFLOW_DASHBOARD_DOMAIN=$2 | ||
| KYB_DOMAIN=$3 | ||
| WORKFLOW_SERVICE_DOMAIN=$4 | ||
| mkdir -p "$PWD/deploy/caddy" | ||
| output_file="$PWD/deploy/caddy/Caddyfile" | ||
| cat <<EOF > "$output_file" | ||
| $BACKOFFICE_DOMAIN { | ||
| reverse_proxy backoffice:80 | ||
| } | ||
|
|
||
| $WORKFLOW_SERVICE_DOMAIN { | ||
| reverse_proxy workflow-service:3000 | ||
| } | ||
|
|
||
| install_docker | ||
| $KYB_DOMAIN { | ||
| reverse_proxy kyb-app:80 | ||
| } | ||
|
|
||
| if [[ ! -z "${WORKFLOW_SERVICE_DOMAIN_NAME}" ]]; then | ||
| ### Update frontend build variables only if domain_name is given | ||
| update_frontend_build_variables | ||
| update_env_docker_compose | ||
| fi | ||
| $WORKFLOW_DASHBOARD_DOMAIN { | ||
| reverse_proxy workflows-dashboard:80 | ||
| } | ||
| EOF | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and backup for Caddy configuration
The function should backup existing configuration and validate file creation.
create_caddy_file(){
echo "Creating Caddy file..."
BACKOFFICE_DOMAIN=$1
WORKFLOW_DASHBOARD_DOMAIN=$2
KYB_DOMAIN=$3
WORKFLOW_SERVICE_DOMAIN=$4
- mkdir -p "$PWD/deploy/caddy"
+ local caddy_dir="$PWD/deploy/caddy"
+ local output_file="$caddy_dir/Caddyfile"
+
+ # Create directory with error handling
+ if ! mkdir -p "$caddy_dir"; then
+ echo "Error: Failed to create Caddy directory"
+ exit 1
+ fi
+
+ # Backup existing configuration
+ if [ -f "$output_file" ]; then
+ if ! cp "$output_file" "${output_file}.backup"; then
+ echo "Error: Failed to backup existing Caddy configuration"
+ exit 1
+ fi
+ fi
- output_file="$PWD/deploy/caddy/Caddyfile"
cat <<EOF > "$output_file"
$BACKOFFICE_DOMAIN {
reverse_proxy backoffice:80
}
# ... rest of the configuration ...
EOF
+
+ # Verify file creation
+ if [ ! -f "$output_file" ]; then
+ echo "Error: Failed to create Caddy configuration file"
+ exit 1
+ fi
+
+ echo "Successfully created Caddy configuration at $output_file"
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ballerine_install.sh (1)
7-18: Enhance help message with default values and requirementsThe help message is clear but could be more informative by including:
- Default values when no arguments are provided
- Requirements for domain URLs (http/https)
- Examples with actual protocol prefixes
show_help() { echo "Usage: $0 [OPTIONS]" echo "" echo "Options:" echo " -h, --help Display this help message." - echo " -d, --domain vite_domain Vite Domain URL." + echo " -d, --domain vite_domain Vite Domain URL (must start with http:// or https://)." echo " -v, --verbose Enable verbose output." echo "" + echo "Defaults:" + echo " When no arguments are provided, all domains default to localhost" echo "Examples:" - echo " $0 --domain example.com" + echo " $0 --domain http://example.com" + echo " $0 --domain https://example.com" echo " $0 -v" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ballerine_install.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
ballerine_install.sh
[warning] 215-215: VERBOSE appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (8)
ballerine_install.sh (8)
84-114: LGTM: OS detection function improvements
The function now properly handles error cases and exits when appropriate, addressing previous review comments.
128-132: LGTM: Platform-specific sed command handling
The functions now properly handle sed command differences between macOS and Linux.
Also applies to: 152-160
21-35:
Improve domain validation with proper URL format checks
The current validation only checks for protocol presence but doesn't validate the URL format.
This issue was previously identified. Please implement the suggested improvements from the previous review comment that includes:
- Better URL format validation using regex
- More informative error messages
- Clear warning for non-secure HTTP protocol
37-51:
Critical: Improve deployment function security and reliability
The function has several critical issues that need to be addressed:
- Unsafe directory change without verification
- No error handling for docker-compose commands
- Duplicated domain validation logic
This issue was previously identified. Please implement the suggested improvements from the previous review comment that includes:
- Directory existence verification
- Error handling for docker-compose commands
- Reuse of existing domain validation
59-81:
Enhance Docker installation security and functionality
The Docker installation functions need improvements in security and functionality.
This issue was previously identified. Please implement the suggested improvements from the previous review comment that includes:
- GPG key verification
- Docker package version pinning
- Actual macOS installation implementation
165-191:
Improve Caddy configuration reliability
The function needs better error handling and backup functionality.
This issue was previously identified. Please implement the suggested improvements from the previous review comment that includes:
- Configuration backup
- Directory creation error handling
- File creation verification
227-229:
Add domain validation before deployment
The script should validate VITE_DOMAIN_NAME before deployment.
This issue was previously identified. Please implement the suggested improvements from the previous review comment that includes domain validation before deployment.
214-217:
Address unused VERBOSE flag
The VERBOSE flag is set but never used in the script.
Either implement the verbose logging functionality or remove the unused flag:
- -v|--verbose)
- VERBOSE=true
- echo "Verbose mode enabled."
- shift
- ;;If keeping the verbose mode, implement it using a logging function as suggested in previous review comments.
Likely invalid or redundant comment.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 215-215: VERBOSE appears unused. Verify use (or export if used externally).
(SC2034)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
ballerine_install.sh (2)
6-18: Enhance help message with more detailsThe help message could be more informative by including:
- Default values (e.g., "localhost")
- Examples for all available options including verbose mode
- Prerequisites (e.g., sudo access requirement)
show_help() { echo "Usage: $0 [OPTIONS]" echo "" echo "Options:" echo " -h, --help Display this help message." echo " -d, --domain vite_domain Vite Domain URL." echo " -v, --verbose Enable verbose output." echo "" + echo "Prerequisites:" + echo " - Requires sudo access for Docker operations" + echo "" + echo "Defaults:" + echo " - If no domain is provided, 'localhost' will be used" + echo "" echo "Examples:" echo " $0 --domain example.com" - echo " $0 -v" + echo " $0 --domain https://example.com -v" + echo " $0 --verbose" }
173-197: Externalize port configurationThe reverse proxy port numbers are hardcoded in the Caddy configuration. These should be configurable to support different deployment scenarios.
+# Add at the beginning of the script +BACKOFFICE_PORT=${BACKOFFICE_PORT:-80} +WORKFLOW_SERVICE_PORT=${WORKFLOW_SERVICE_PORT:-3000} +KYB_PORT=${KYB_PORT:-80} +WORKFLOW_DASHBOARD_PORT=${WORKFLOW_DASHBOARD_PORT:-80} create_caddy_file(){ # ... existing code ... cat <<EOF > "$output_file" $BACKOFFICE_DOMAIN { - reverse_proxy backoffice:80 + reverse_proxy backoffice:${BACKOFFICE_PORT} } $WORKFLOW_SERVICE_DOMAIN { - reverse_proxy workflow-service:3000 + reverse_proxy workflow-service:${WORKFLOW_SERVICE_PORT} } $KYB_DOMAIN { - reverse_proxy kyb-app:80 + reverse_proxy kyb-app:${KYB_PORT} } $WORKFLOW_DASHBOARD_DOMAIN { - reverse_proxy workflows-dashboard:80 + reverse_proxy workflows-dashboard:${WORKFLOW_DASHBOARD_PORT} } EOF }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ballerine_install.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
ballerine_install.sh
[warning] 223-223: VERBOSE appears unused. Verify use (or export if used externally).
(SC2034)
| check_http_https() { | ||
| local input=$1 | ||
| echo "checking domain if suitable $input" | ||
| if [[ $input == *http://* && $input != *https://* ]]; then | ||
| echo "The string contains 'http' but not 'https'." | ||
| elif [[ $input == *https://* && $input != *http://* ]]; then | ||
| echo "The string contains 'https' but not 'http'." | ||
| elif [[ $input == *http://* && $input == *https://* ]]; then | ||
| echo "The string contains both 'http' and 'https'." | ||
| exit 1; | ||
| else | ||
| echo "The string contains neither 'http' nor 'https'." | ||
| exit 1; | ||
| fi | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate code duplication in HTTP/HTTPS validation
The HTTP/HTTPS validation logic is duplicated between check_http_https and deploy_ballerine functions. This violates the DRY principle and makes maintenance harder.
Refactor to reuse the validation logic:
check_http_https() {
local input=$1
echo "checking domain if suitable $input"
+ local protocol=""
if [[ $input == *http://* && $input != *https://* ]]; then
echo "The string contains 'http' but not 'https'."
+ protocol="http"
elif [[ $input == *https://* && $input != *http://* ]]; then
echo "The string contains 'https' but not 'http'."
+ protocol="https"
elif [[ $input == *http://* && $input == *https://* ]]; then
echo "The string contains both 'http' and 'https'."
exit 1;
else
echo "The string contains neither 'http' nor 'https'."
exit 1;
fi
+ echo "$protocol"
}
deploy_ballerine() {
local input=$1
- echo "checking domain if suitable $input"
- if [[ $input == *http://* && $input != *https://* ]]; then
+ local protocol
+ protocol=$(check_http_https "$input")
+ if [[ "$protocol" == "http" ]]; then
if [[ "$OSTYPE" == "darwin"* ]]; then
cd deploy; sudo docker-compose -f docker-compose-build.yml up -d
else
cd deploy; sudo docker compose -f docker-compose-build.yml up -d
fi
- elif [[ $input == *https://* && $input != *http://* ]]; then
+ elif [[ "$protocol" == "https" ]]; then
if [[ "$OSTYPE" == "darwin"* ]]; then
cd deploy; sudo docker-compose -f docker-compose-build-https.yml up -d
else
cd deploy; sudo docker compose -f docker-compose-build-https.yml up -d
fi
- elif [[ $input == *http://* && $input == *https://* ]]; then
- echo "The string contains both 'http' and 'https'."
- exit 1;
else
- echo "The string contains neither 'http' nor 'https'."
cd deploy; sudo docker-compose -f docker-compose-build.yml up -d
fi
}Also applies to: 37-59
| echo "checking domain if suitable $input" | ||
| if [[ $input == *http://* && $input != *https://* ]]; then | ||
| if [[ "$OSTYPE" == "darwin"* ]]; then | ||
| cd deploy; sudo docker-compose -f docker-compose-build.yml up -d |
There was a problem hiding this comment.
Add safety checks for directory operations
The cd commands lack error handling and directory existence verification, which could lead to script failures.
- cd deploy; sudo docker-compose -f docker-compose-build.yml up -d
+ if [ ! -d "deploy" ]; then
+ echo "Error: deploy directory not found"
+ exit 1
+ fi
+ cd deploy || exit 1
+ sudo docker-compose -f docker-compose-build.yml up -dAlso applies to: 44-44, 48-48, 50-50, 57-57
| echo $env_files | ||
| for i in $env_files; | ||
| do | ||
| echo "Updating env variables of $i" | ||
| sed -i "s/localhost/${WORKFLOW_SERVICE_DOMAIN_NAME}/g" $i | ||
| if [[ "$OSTYPE" == "darwin"* ]]; then | ||
| sed -i '' "s|http://localhost:3000|$VITE_DOMAIN_NAME|g" $i | ||
| else | ||
| sed -i "s|http://localhost:3000|$VITE_DOMAIN_NAME|g" $i | ||
| fi |
There was a problem hiding this comment.
Add error handling for file operations
The find commands lack error handling, and the script continues even if no files are found.
- env_files=$(find ./apps -name "*.env.example")
- echo $env_files
+ env_files=$(find ./apps -name "*.env.example") || {
+ echo "Error: Failed to search for env files"
+ exit 1
+ }
+ if [ -z "$env_files" ]; then
+ echo "Warning: No .env.example files found"
+ return
+ }
+ echo "Found env files: $env_files"Also applies to: 156-168
| check_os | ||
|
|
||
| ## Bring docker-container up | ||
| cd deploy; sudo docker-compose -f docker-compose-build.yml up -d | ||
| deploy_ballerine $VITE_DOMAIN_NAME |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add service readiness check and cleanup handling
The script should verify service readiness after deployment and handle cleanup on exit.
+check_service_readiness() {
+ local max_attempts=30
+ local attempt=1
+ echo "Checking service readiness..."
+ while [ $attempt -le $max_attempts ]; do
+ if curl -s -f "$VITE_DOMAIN_NAME" >/dev/null; then
+ echo "Services are ready!"
+ return 0
+ fi
+ echo "Attempt $attempt/$max_attempts: Services not ready yet..."
+ sleep 10
+ attempt=$((attempt + 1))
+ done
+ echo "Error: Services failed to become ready"
+ return 1
+}
+
+cleanup() {
+ if [ -d "deploy" ]; then
+ cd deploy || exit 1
+ echo "Cleaning up containers..."
+ sudo docker-compose down 2>/dev/null || true
+ fi
+}
+
+# Set up trap for cleanup
+trap cleanup EXIT
+
check_os
deploy_ballerine $VITE_DOMAIN_NAME
+check_service_readiness || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| check_os | |
| ## Bring docker-container up | |
| cd deploy; sudo docker-compose -f docker-compose-build.yml up -d | |
| deploy_ballerine $VITE_DOMAIN_NAME | |
| check_service_readiness() { | |
| local max_attempts=30 | |
| local attempt=1 | |
| echo "Checking service readiness..." | |
| while [ $attempt -le $max_attempts ]; do | |
| if curl -s -f "$VITE_DOMAIN_NAME" >/dev/null; then | |
| echo "Services are ready!" | |
| return 0 | |
| fi | |
| echo "Attempt $attempt/$max_attempts: Services not ready yet..." | |
| sleep 10 | |
| attempt=$((attempt + 1)) | |
| done | |
| echo "Error: Services failed to become ready" | |
| return 1 | |
| } | |
| cleanup() { | |
| if [ -d "deploy" ]; then | |
| cd deploy || exit 1 | |
| echo "Cleaning up containers..." | |
| sudo docker-compose down 2>/dev/null || true | |
| fi | |
| } | |
| # Set up trap for cleanup | |
| trap cleanup EXIT | |
| check_os | |
| deploy_ballerine $VITE_DOMAIN_NAME | |
| check_service_readiness || exit 1 |
| check_os() { | ||
| # Get the operating system name | ||
| uname_out="$(uname -s)" | ||
|
|
||
| function update_frontend_build_variables() { | ||
| case "${uname_out}" in | ||
| Linux*) | ||
| # Check if the Linux distro is Ubuntu | ||
| if [ -f /etc/os-release ]; then | ||
| . /etc/os-release | ||
| if [[ $ID == "ubuntu" ]]; then | ||
| echo "The host is running Ubuntu." | ||
| install_docker_ubuntu | ||
| else | ||
| echo "The host is running a Linux distribution but not Ubuntu." | ||
| echo "We do not support this Linux distribution" | ||
| exit 1 | ||
| fi | ||
| else | ||
| echo "The host is running Linux but /etc/os-release is not available." | ||
| fi | ||
| ;; | ||
| Darwin*) | ||
| echo "The host is running macOS." | ||
| install_docker_macos | ||
| ;; | ||
| *) | ||
| echo "The operating system is not recognized." | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify Docker installation success
The script should verify that Docker was successfully installed and is running.
check_os() {
+ verify_docker() {
+ if ! command -v docker >/dev/null 2>&1; then
+ echo "Error: Docker installation failed"
+ exit 1
+ fi
+ if ! docker info >/dev/null 2>&1; then
+ echo "Error: Docker daemon is not running"
+ exit 1
+ fi
+ }
+
# Get the operating system name
uname_out="$(uname -s)"
case "${uname_out}" in
Linux*)
if [ -f /etc/os-release ]; then
. /etc/os-release
if [[ $ID == "ubuntu" ]]; then
echo "The host is running Ubuntu."
install_docker_ubuntu
+ verify_docker
else
echo "The host is running a Linux distribution but not Ubuntu."
echo "We do not support this Linux distribution"
exit 1
fi
else
echo "The host is running Linux but /etc/os-release is not available."
fi
;;
Darwin*)
echo "The host is running macOS."
install_docker_macos
+ verify_docker
;;
*)
echo "The operating system is not recognized."
exit 1
;;
esac
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| check_os() { | |
| # Get the operating system name | |
| uname_out="$(uname -s)" | |
| function update_frontend_build_variables() { | |
| case "${uname_out}" in | |
| Linux*) | |
| # Check if the Linux distro is Ubuntu | |
| if [ -f /etc/os-release ]; then | |
| . /etc/os-release | |
| if [[ $ID == "ubuntu" ]]; then | |
| echo "The host is running Ubuntu." | |
| install_docker_ubuntu | |
| else | |
| echo "The host is running a Linux distribution but not Ubuntu." | |
| echo "We do not support this Linux distribution" | |
| exit 1 | |
| fi | |
| else | |
| echo "The host is running Linux but /etc/os-release is not available." | |
| fi | |
| ;; | |
| Darwin*) | |
| echo "The host is running macOS." | |
| install_docker_macos | |
| ;; | |
| *) | |
| echo "The operating system is not recognized." | |
| exit 1 | |
| ;; | |
| esac | |
| } | |
| check_os() { | |
| verify_docker() { | |
| if ! command -v docker >/dev/null 2>&1; then | |
| echo "Error: Docker installation failed" | |
| exit 1 | |
| fi | |
| if ! docker info >/dev/null 2>&1; then | |
| echo "Error: Docker daemon is not running" | |
| exit 1 | |
| fi | |
| } | |
| # Get the operating system name | |
| uname_out="$(uname -s)" | |
| case "${uname_out}" in | |
| Linux*) | |
| if [ -f /etc/os-release ]; then | |
| . /etc/os-release | |
| if [[ $ID == "ubuntu" ]]; then | |
| echo "The host is running Ubuntu." | |
| install_docker_ubuntu | |
| verify_docker | |
| else | |
| echo "The host is running a Linux distribution but not Ubuntu." | |
| echo "We do not support this Linux distribution" | |
| exit 1 | |
| fi | |
| else | |
| echo "The host is running Linux but /etc/os-release is not available." | |
| fi | |
| ;; | |
| Darwin*) | |
| echo "The host is running macOS." | |
| install_docker_macos | |
| verify_docker | |
| ;; | |
| *) | |
| echo "The operating system is not recognized." | |
| exit 1 | |
| ;; | |
| esac | |
| } |
pratapalakshmi
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Chores