Skip to content
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

Cu 86c0zjv0u add climate mediator package to platform #331

Closed
8 changes: 8 additions & 0 deletions climate-mediator/docker-compose.dev.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
version: '3.9'

services:
climate-mediator:
ports:
- target: 3000
published: 3011
mode: host
37 changes: 37 additions & 0 deletions climate-mediator/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
version: "3.9"

services:
climate-mediator:
image: ${CLIMATE_MEDIATOR_IMAGE}
networks:
openhim:
clickhouse:
minio:
public:
default:
environment:
OPENHIM_USERNAME: ${OPENHIM_USERNAME}
OPENHIM_PASSWORD: ${OPENHIM_PASSWORD}
REGISTER_MEDIATOR: ${REGISTER_MEDIATOR}
OPENHIM_MEDIATOR_URL: ${OPENHIM_MEDIATOR_URL}
TRUST_SELF_SIGNED: ${TRUST_SELF_SIGNED}
MINIO_ENDPOINT: ${MINIO_ENDPOINT}
MINIO_PORT: ${MINIO_PORT}
MINIO_BUCKET: ${MINIO_BUCKET}
MINIO_ACCESS_KEY: ${MINIO_ACCESS_KEY}
MINIO_SECRET_KEY: ${MINIO_SECRET_KEY}
Comment on lines +12 to +22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Review security configuration and credential handling.

Several security considerations need attention:

  1. Ensure sensitive credentials are properly managed and not exposed in logs or container inspection
  2. The TRUST_SELF_SIGNED variable suggests SSL/TLS certificate validation might be bypassed
  3. MinIO credentials should be handled securely

Consider these security improvements:

  • Use Docker secrets or a secure vault service for credential management
  • Review the necessity of trusting self-signed certificates in production
  • Implement credential rotation policies


networks:
openhim:
name: openhim_public
external: true
clickhouse:
name: clickhouse_public
external: true
minio:
name: minio_public
external: true
public:
name: climate_public
external: true
default:
36 changes: 36 additions & 0 deletions climate-mediator/importer/docker-compose.config.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
version: "3.9"

services:
climate-mediator-config-importer:
image: ${CLIMATE_MEDIATOR_CONFIG_IMPORTER_IMAGE}
networks:
openhim:
default:
environment:
OPENHIM_API_USERNAME: ${OPENHIM_USERNAME}
OPENHIM_API_PASSWORD: ${OPENHIM_PASSWORD}
NODE_TLS_REJECT_UNAUTHORIZED: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security Risk: TLS verification is disabled

Setting NODE_TLS_REJECT_UNAUTHORIZED=0 disables SSL/TLS certificate validation, which is a security risk in production environments. This should only be used in development/testing environments.

Consider:

  1. Using valid SSL certificates in production
  2. Making this configurable via environment variable to ensure it's enabled in production:
-     NODE_TLS_REJECT_UNAUTHORIZED: 0
+     NODE_TLS_REJECT_UNAUTHORIZED: ${NODE_TLS_REJECT_UNAUTHORIZED:-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.

Suggested change
NODE_TLS_REJECT_UNAUTHORIZED: 0
NODE_TLS_REJECT_UNAUTHORIZED: ${NODE_TLS_REJECT_UNAUTHORIZED:-1}

Comment on lines +10 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use Docker secrets for sensitive credentials

Consider using Docker secrets instead of environment variables for OPENHIM_API_USERNAME and OPENHIM_API_PASSWORD to enhance security.

    environment:
-     OPENHIM_API_USERNAME: ${OPENHIM_USERNAME}
-     OPENHIM_API_PASSWORD: ${OPENHIM_PASSWORD}
      NODE_TLS_REJECT_UNAUTHORIZED: 0
+   secrets:
+     - openhim_username
+     - openhim_password

+ secrets:
+   openhim_username:
+     external: true
+   openhim_password:
+     external: true

Committable suggestion skipped: line range outside the PR's diff.

command: sh -c "node openhimConfig.js"
configs:
- source: climate-mediator-config-importer-openhimConfig.js
target: /openhimConfig.js
- source: climate-mediator-config-importer-openhim-import.json
target: /openhim-import.json

configs:
climate-mediator-config-importer-openhimConfig.js:
file: ./volume/openhimConfig.js
name: climate-mediator-config-importer-openhimConfig.js-${climate_mediator_config_importer_openhimConfig_js_DIGEST:?err}
labels:
name: climate-mediator
climate-mediator-config-importer-openhim-import.json:
file: ./volume/openhim-import.json
name: climate-mediator-config-importer-openhim-import.json-${climate_mediator_config_importer_openhim_import_js_DIGEST:?err}
labels:
name: climate-mediator

networks:
openhim:
name: openhim_public
external: true
default:
92 changes: 92 additions & 0 deletions climate-mediator/importer/volume/openhim-import.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
{
"Users": [
{
"firstname": "Super",
"surname": "User",
"email": "[email protected]",
"provider": "token",
"groups": ["admin"],
"passwordAlgorithm": "sha512",
"passwordHash": "ea3824f17cf1379eb118a36bc7c8cf0f45712e2af7748567fca5313dec6fa66d61064e82a5e5cb88e998486ee3c7d0dac235bbeda8c341d6edc1c77406be2ab6",
"passwordSalt": "d4f622c0404f09bd959bfb263efa3452",
"expiry": null,
"locked": false,
"token": null,
"tokenType": null
}
],
Comment on lines +2 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security: Avoid committing credentials to version control

The configuration contains hardcoded credentials including password hash and salt. This poses a security risk as these values should be managed through environment variables or secure configuration management systems.

Consider:

  1. Moving sensitive data to environment variables
  2. Using a more robust password hashing algorithm like Argon2
  3. Using a production-specific email domain instead of openhim.org

Example structure using environment variables:

{
  "Users": [{
    "firstname": "Super",
    "surname": "User",
    "email": "${ADMIN_EMAIL}",
    "passwordHash": "${ADMIN_PASSWORD_HASH}",
    "passwordSalt": "${ADMIN_PASSWORD_SALT}",
    ...
  }]
}
🧰 Tools
🪛 Gitleaks

10-10: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

"Clients": [
{
"clientID": "instant-client",
"name": "Instant Client",
"roles": ["instant"],
"customTokenID": "test"
}
],
Comment on lines +18 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Security: Critical - Replace hardcoded test tokens across multiple services

The verification reveals a systemic security issue where multiple services are using hardcoded test tokens and overly broad "instant" role:

  • climate-mediator: customTokenID: "test"
  • mpi-mediator: customTokenID: "test"
  • fhir-ig-importer: customTokenID: "test"
  • interoperability-layer: customTokenID: "test"
  1. Replace all hardcoded "test" tokens with environment variables across all services
  2. Review and implement proper role-based access control instead of using the generic "instant" role
  3. Consider implementing different tokens for each service to limit the blast radius of potential token compromises
  4. Update client IDs to be more specific than "test" or generic "instant-client"
🔗 Analysis chain

Security: Replace test token and review access control

The client configuration contains a hardcoded test token and uses a single broad role.

  1. Replace "test" token with environment variable
  2. Consider implementing more granular roles based on specific operations

Let's verify if this client configuration is used elsewhere:


The previous search might have failed due to case sensitivity or word boundaries. Let's try a broader search to find any related client configurations and token usage patterns in JSON files.


Let's examine the actual client configurations in these files to understand if this is a widespread pattern that needs to be addressed across multiple services.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for references to the client ID or token
rg -l "instant-client|test" --type json

Length of output: 426


Script:

#!/bin/bash
# Search for references to the client ID or token with case-insensitive and word boundary matches
rg -il '\b(instant-client|instant|customTokenID)\b' --type json

# Also search for any client configurations in JSON files
fd -e json -x grep -l '"Clients"\|"clientID"\|"customTokenID"' {} \;

Length of output: 1139


Script:

#!/bin/bash
# Check the content of all openhim-import files to see if they share similar client configurations
echo "=== Checking climate-mediator configuration ==="
cat climate-mediator/importer/volume/openhim-import.json | jq '.Clients'

echo "=== Checking mpi-mediator configuration ==="
cat mpi-mediator/importer/volume/openhim-import.json | jq '.Clients'

echo "=== Checking fhir-ig-importer configuration ==="
cat fhir-ig-importer/importer/volume/openhim-import.json | jq '.Clients'

echo "=== Checking interoperability-layer configuration ==="
cat interoperability-layer-openhim/importer/volume/openhim-import.json | jq '.Clients'

Length of output: 1267

"Channels": [
{
"name": "Climate Mediator",
"urlPattern": "^/climate.*$",
"routes": [
{
"name": "Climate Mediator Endpoint",
"host": "climate-mediator",
"port": "3000",
"primary": true,
"type": "http",
"path": "/climate/upload"
}
],
"allow": ["instant"],
"methods": ["POST", "GET"],
"type": "http",
"updatedBy": {
"id": "65d47e98223a0672c3c9808b",
"name": "Super User"
}
}
],
Comment on lines +26 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Security: Enhance channel configuration with additional safeguards

The current channel configuration could benefit from additional security measures:

  1. Consider a more specific URL pattern to prevent unintended matches
  2. Add rate limiting configuration to prevent abuse
  3. Configure request size limits for the upload endpoint
  4. Remove hardcoded user ID from updatedBy
 {
   "name": "Climate Mediator",
-  "urlPattern": "^/climate.*$",
+  "urlPattern": "^/climate/upload$",
   "routes": [...],
+  "requestBody": {
+    "limit": "10mb"
+  },
+  "rateLimit": {
+    "requests": 100,
+    "window": "15minutes"
+  },
   "updatedBy": {
-    "id": "65d47e98223a0672c3c9808b",
+    "id": "${ADMIN_USER_ID}",
     "name": "Super User"
   }
 }
📝 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.

Suggested change
"Channels": [
{
"name": "Climate Mediator",
"urlPattern": "^/climate.*$",
"routes": [
{
"name": "Climate Mediator Endpoint",
"host": "climate-mediator",
"port": "3000",
"primary": true,
"type": "http",
"path": "/climate/upload"
}
],
"allow": ["instant"],
"methods": ["POST", "GET"],
"type": "http",
"updatedBy": {
"id": "65d47e98223a0672c3c9808b",
"name": "Super User"
}
}
],
"Channels": [
{
"name": "Climate Mediator",
"urlPattern": "^/climate/upload$",
"routes": [
{
"name": "Climate Mediator Endpoint",
"host": "climate-mediator",
"port": "3000",
"primary": true,
"type": "http",
"path": "/climate/upload"
}
],
"allow": ["instant"],
"methods": ["POST", "GET"],
"type": "http",
"requestBody": {
"limit": "10mb"
},
"rateLimit": {
"requests": 100,
"window": "15minutes"
},
"updatedBy": {
"id": "${ADMIN_USER_ID}",
"name": "Super User"
}
}
],

"Mediators": [
{
"urn": "urn:mediator:climate-mediator",
"version": "0.0.1",
"name": "Climate mediator",
"description": "A mediator handling interactions between the OpenHIM Core service and climate related data",
"endpoints": [
{
"name": "Climate Endpoint",
"type": "http",
"status": "enabled",
"host": "climate-mediator",
"port": 3000,
"path": "/climate",
"primary": true,
"forwardAuthHeader": false
}
],
"defaultChannelConfig": [
{
"name": "Climate Mediator",
"urlPattern": "^/climate.*$",
"routes": [
{
"name": "Climate Mediator Endpoint",
"host": "climate-mediator",
"port": "3000",
"primary": true,
"type": "http",
"path": "/climate/upload"
}
],
"allow": ["instant"],
"methods": ["POST", "GET"],
"type": "http"
}
],
"configDefs": [],
"_lastHeartbeat": "2024-12-14T12:23:22.769Z",
"_uptime": 5650.734193405
}
],
"ContactGroups": []
}
54 changes: 54 additions & 0 deletions climate-mediator/importer/volume/openhimConfig.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
'use strict'

const fs = require('fs')
const https = require('https')
const path = require('path')

const OPENHIM_CORE_SERVICE_NAME = 'openhim-core'
const OPENHIM_MEDIATOR_API_PORT = 8080
const OPENHIM_API_PASSWORD =
process.env.OPENHIM_API_PASSWORD || 'instant101'
const OPENHIM_API_USERNAME =
process.env.OPENHIM_API_USERNAME || '[email protected]'
Comment on lines +9 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security: Remove hardcoded default credentials

Hardcoded default credentials pose a security risk. Consider:

  1. Making these environment variables required
  2. Using more secure defaults if absolutely necessary
  3. Adding validation to ensure strong passwords
-const OPENHIM_API_PASSWORD =
-  process.env.OPENHIM_API_PASSWORD || 'instant101'
-const OPENHIM_API_USERNAME =
-  process.env.OPENHIM_API_USERNAME || '[email protected]'
+const OPENHIM_API_PASSWORD = process.env.OPENHIM_API_PASSWORD
+const OPENHIM_API_USERNAME = process.env.OPENHIM_API_USERNAME
+
+if (!OPENHIM_API_PASSWORD || !OPENHIM_API_USERNAME) {
+  throw new Error('OpenHIM API credentials must be provided via environment variables')
+}
📝 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.

Suggested change
const OPENHIM_API_PASSWORD =
process.env.OPENHIM_API_PASSWORD || 'instant101'
const OPENHIM_API_USERNAME =
process.env.OPENHIM_API_USERNAME || '[email protected]'
const OPENHIM_API_PASSWORD = process.env.OPENHIM_API_PASSWORD
const OPENHIM_API_USERNAME = process.env.OPENHIM_API_USERNAME
if (!OPENHIM_API_PASSWORD || !OPENHIM_API_USERNAME) {
throw new Error('OpenHIM API credentials must be provided via environment variables')
}


const authHeader = new Buffer.from(
`${OPENHIM_API_USERNAME}:${OPENHIM_API_PASSWORD}`
).toString('base64')

const jsonData = JSON.parse(
fs.readFileSync(path.resolve(__dirname, 'openhim-import.json'))
)
Comment on lines +18 to +20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for configuration file loading

The synchronous file reading operation lacks error handling and could block the event loop. Consider:

  1. Using async/await with proper error handling
  2. Validating the JSON structure after parsing
-const jsonData = JSON.parse(
-  fs.readFileSync(path.resolve(__dirname, 'openhim-import.json'))
-)
+try {
+  const configPath = path.resolve(__dirname, 'openhim-import.json')
+  const jsonData = JSON.parse(fs.readFileSync(configPath))
+  
+  // Validate required fields
+  if (!jsonData.Users || !jsonData.Channels || !jsonData.Mediators) {
+    throw new Error('Invalid configuration file structure')
+  }
+} catch (error) {
+  throw new Error(`Failed to load OpenHIM configuration: ${error.message}`)
+}

Committable suggestion skipped: line range outside the PR's diff.


const data = JSON.stringify(jsonData)

const options = {
protocol: 'https:',
hostname: OPENHIM_CORE_SERVICE_NAME,
port: OPENHIM_MEDIATOR_API_PORT,
path: '/metadata',
method: 'POST',
headers: {
'Content-Type': 'application/json',
'Content-Length': data.length,
Authorization: `Basic ${authHeader}`
}
}
Comment on lines +24 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add SSL/TLS verification options

The HTTPS request configuration lacks SSL/TLS verification options, which could lead to security vulnerabilities. Consider:

  1. Adding proper certificate verification
  2. Making the protocol configurable via environment variables
 const options = {
   protocol: 'https:',
   hostname: OPENHIM_CORE_SERVICE_NAME,
   port: OPENHIM_MEDIATOR_API_PORT,
   path: '/metadata',
   method: 'POST',
+  rejectUnauthorized: process.env.NODE_ENV !== 'development',
   headers: {
     'Content-Type': 'application/json',
     'Content-Length': data.length,
     Authorization: `Basic ${authHeader}`
   }
 }

Committable suggestion skipped: line range outside the PR's diff.


const req = https.request(options, res => {
if (res.statusCode == 401) {
throw new Error(`Incorrect OpenHIM API credentials`)
}

if (res.statusCode != 201) {
throw new Error(`Failed to import OpenHIM config: ${res.statusCode}`)
}

console.log('Successfully Imported OpenHIM Config')
})

req.on('error', error => {
throw new Error(`Failed to import OpenHIM config: ${error}`)
})

req.write(data)
req.end()
Comment on lines +37 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance request error handling and add timeout

The current error handling could be more robust. Consider:

  1. Adding request timeout
  2. Parsing error response bodies
  3. Proper request cleanup
+const TIMEOUT_MS = 5000
+
 const req = https.request(options, res => {
+  let responseData = ''
+
+  res.on('data', chunk => {
+    responseData += chunk
+  })
+
+  res.on('end', () => {
   if (res.statusCode == 401) {
-    throw new Error(`Incorrect OpenHIM API credentials`)
+    throw new Error('Incorrect OpenHIM API credentials')
   }

   if (res.statusCode != 201) {
-    throw new Error(`Failed to import OpenHIM config: ${res.statusCode}`)
+    const errorMessage = responseData ? JSON.parse(responseData).error : 'Unknown error'
+    throw new Error(`Failed to import OpenHIM config (${res.statusCode}): ${errorMessage}`)
   }

   console.log('Successfully Imported OpenHIM Config')
+  })
 })

 req.on('error', error => {
   throw new Error(`Failed to import OpenHIM config: ${error}`)
 })

+req.setTimeout(TIMEOUT_MS, () => {
+  req.destroy()
+  throw new Error('Request timed out while importing OpenHIM config')
+})

 req.write(data)
 req.end()
📝 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.

Suggested change
const req = https.request(options, res => {
if (res.statusCode == 401) {
throw new Error(`Incorrect OpenHIM API credentials`)
}
if (res.statusCode != 201) {
throw new Error(`Failed to import OpenHIM config: ${res.statusCode}`)
}
console.log('Successfully Imported OpenHIM Config')
})
req.on('error', error => {
throw new Error(`Failed to import OpenHIM config: ${error}`)
})
req.write(data)
req.end()
const TIMEOUT_MS = 5000
const req = https.request(options, res => {
let responseData = ''
res.on('data', chunk => {
responseData += chunk
})
res.on('end', () => {
if (res.statusCode == 401) {
throw new Error('Incorrect OpenHIM API credentials')
}
if (res.statusCode != 201) {
const errorMessage = responseData ? JSON.parse(responseData).error : 'Unknown error'
throw new Error(`Failed to import OpenHIM config (${res.statusCode}): ${errorMessage}`)
}
console.log('Successfully Imported OpenHIM Config')
})
})
req.on('error', error => {
throw new Error(`Failed to import OpenHIM config: ${error}`)
})
req.setTimeout(TIMEOUT_MS, () => {
req.destroy()
throw new Error('Request timed out while importing OpenHIM config')
})
req.write(data)
req.end()

22 changes: 22 additions & 0 deletions climate-mediator/package-metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"id": "climate-mediator",
"name": "Climate Mediator",
"description": "This package creates a mediator service that is used in processing climate related data",
"type": "infrastructure",
"version": "0.0.1",
"dependencies": ["interoperability-layer-openhim", "datalake"],
"environmentVariables": {
"TRUST_SELF_SIGNED": "true",
"OPENHIM_MEDIATOR_URL": "https://openhim-core:8080",
"OPENHIM_USERNAME": "[email protected]",
"OPENHIM_PASSWORD": "instant101",
"REGISTER_MEDIATOR": "true",
"CLIMATE_MEDIATOR_IMAGE": "jembi/climate-mediator:latest",
"CLIMATE_MEDIATOR_CONFIG_IMPORTER_IMAGE": "node:erbium-alpine",
"MINIO_BUCKET": "climate-mediator",
"MINIO_ACCESS_KEY": "tCroZpZ3usDUcvPM3QT6",
"MINIO_SECRET_KEY": "suVjMHUpVIGyWx8fFJHTiZiT88dHhKgVpzvYTOKK",
"MINIO_ENDPOINT": "minio-01",
"MINIO_PORT": "9090"
}
}
82 changes: 82 additions & 0 deletions climate-mediator/swarm.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#!/bin/bash

declare ACTION=""
declare MODE=""
declare COMPOSE_FILE_PATH=""
declare UTILS_PATH=""
declare STACK="climate"

function init_vars() {
ACTION=$1
MODE=$2

COMPOSE_FILE_PATH=$(
cd "$(dirname "${BASH_SOURCE[0]}")" || exit
pwd -P
)

UTILS_PATH="${COMPOSE_FILE_PATH}/../utils"

readonly ACTION
readonly MODE
readonly COMPOSE_FILE_PATH
readonly UTILS_PATH
readonly STACK
}

# shellcheck disable=SC1091
function import_sources() {
source "${UTILS_PATH}/docker-utils.sh"
source "${UTILS_PATH}/log.sh"
}
Comment on lines +27 to +31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for required utility files.

The script should validate the existence of required utility files and handle missing dependencies gracefully.

 # shellcheck disable=SC1091
 function import_sources() {
+  local required_files=("${UTILS_PATH}/docker-utils.sh" "${UTILS_PATH}/log.sh")
+  
+  for file in "${required_files[@]}"; do
+    if [[ ! -f "$file" ]]; then
+      echo "Error: Required file not found: $file"
+      exit 1
+    fi
+  done
+
   source "${UTILS_PATH}/docker-utils.sh"
   source "${UTILS_PATH}/log.sh"
 }
📝 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.

Suggested change
# shellcheck disable=SC1091
function import_sources() {
source "${UTILS_PATH}/docker-utils.sh"
source "${UTILS_PATH}/log.sh"
}
# shellcheck disable=SC1091
function import_sources() {
local required_files=("${UTILS_PATH}/docker-utils.sh" "${UTILS_PATH}/log.sh")
for file in "${required_files[@]}"; do
if [[ ! -f "$file" ]]; then
echo "Error: Required file not found: $file"
exit 1
fi
done
source "${UTILS_PATH}/docker-utils.sh"
source "${UTILS_PATH}/log.sh"
}


function initialize_package() {
local climate_mediator_dev_compose_filename=""

if [[ "${MODE}" == "dev" ]]; then
log info "Running package in DEV mode"
climate_mediator_dev_compose_filename="docker-compose.dev.yml"
else
log info "Running package in PROD mode"
fi

(
docker::deploy_service "$STACK" "${COMPOSE_FILE_PATH}" "docker-compose.yml" "$climate_mediator_dev_compose_filename"
) || {
log error "Failed to deploy package"
exit 1
}
docker::deploy_config_importer $STACK "$COMPOSE_FILE_PATH/importer/docker-compose.config.yml" "climate-mediator-config-importer" "climate-mediator"
}
Comment on lines +33 to +50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix critical issues in initialize_package function.

Several issues need to be addressed:

  1. Missing quotes around variables in docker::deploy_config_importer call
  2. No error handling for config importer deployment
  3. Missing validation of compose files
 function initialize_package() {
   local climate_mediator_dev_compose_filename=""
+  local compose_files=("${COMPOSE_FILE_PATH}/docker-compose.yml")
 
   if [[ "${MODE}" == "dev" ]]; then
     log info "Running package in DEV mode"
     climate_mediator_dev_compose_filename="docker-compose.dev.yml"
+    compose_files+=("${COMPOSE_FILE_PATH}/${climate_mediator_dev_compose_filename}")
   else
     log info "Running package in PROD mode"
   fi
 
+  # Validate compose files exist
+  for file in "${compose_files[@]}"; do
+    if [[ ! -f "$file" ]]; then
+      log error "Compose file not found: $file"
+      exit 1
+    fi
+  done
+
   (
     docker::deploy_service "$STACK" "${COMPOSE_FILE_PATH}" "docker-compose.yml" "$climate_mediator_dev_compose_filename"
   ) || {
     log error "Failed to deploy package"
     exit 1
   }
-  docker::deploy_config_importer $STACK "$COMPOSE_FILE_PATH/importer/docker-compose.config.yml" "climate-mediator-config-importer" "climate-mediator"
+  
+  (
+    docker::deploy_config_importer "$STACK" \
+      "${COMPOSE_FILE_PATH}/importer/docker-compose.config.yml" \
+      "climate-mediator-config-importer" \
+      "climate-mediator"
+  ) || {
+    log error "Failed to deploy config importer"
+    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.

Suggested change
function initialize_package() {
local climate_mediator_dev_compose_filename=""
if [[ "${MODE}" == "dev" ]]; then
log info "Running package in DEV mode"
climate_mediator_dev_compose_filename="docker-compose.dev.yml"
else
log info "Running package in PROD mode"
fi
(
docker::deploy_service "$STACK" "${COMPOSE_FILE_PATH}" "docker-compose.yml" "$climate_mediator_dev_compose_filename"
) || {
log error "Failed to deploy package"
exit 1
}
docker::deploy_config_importer $STACK "$COMPOSE_FILE_PATH/importer/docker-compose.config.yml" "climate-mediator-config-importer" "climate-mediator"
}
function initialize_package() {
local climate_mediator_dev_compose_filename=""
local compose_files=("${COMPOSE_FILE_PATH}/docker-compose.yml")
if [[ "${MODE}" == "dev" ]]; then
log info "Running package in DEV mode"
climate_mediator_dev_compose_filename="docker-compose.dev.yml"
compose_files+=("${COMPOSE_FILE_PATH}/${climate_mediator_dev_compose_filename}")
else
log info "Running package in PROD mode"
fi
# Validate compose files exist
for file in "${compose_files[@]}"; do
if [[ ! -f "$file" ]]; then
log error "Compose file not found: $file"
exit 1
fi
done
(
docker::deploy_service "$STACK" "${COMPOSE_FILE_PATH}" "docker-compose.yml" "$climate_mediator_dev_compose_filename"
) || {
log error "Failed to deploy package"
exit 1
}
(
docker::deploy_config_importer "$STACK" \
"${COMPOSE_FILE_PATH}/importer/docker-compose.config.yml" \
"climate-mediator-config-importer" \
"climate-mediator"
) || {
log error "Failed to deploy config importer"
exit 1
}
}


function destroy_package() {
docker::stack_destroy "$STACK"

docker::prune_configs "climate-mediator"
}
Comment on lines +52 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and confirmation prompt for destructive operations.

The destroy function should handle errors and prompt for confirmation before destroying resources.

 function destroy_package() {
+  read -p "Are you sure you want to destroy the $STACK stack? [y/N] " -n 1 -r
+  echo
+  if [[ ! $REPLY =~ ^[Yy]$ ]]; then
+    log info "Operation cancelled"
+    exit 0
+  fi
+
+  (
     docker::stack_destroy "$STACK"
+  ) || {
+    log error "Failed to destroy stack"
+    exit 1
+  }
 
+  (
     docker::prune_configs "climate-mediator"
+  ) || {
+    log error "Failed to prune configs"
+    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.

Suggested change
function destroy_package() {
docker::stack_destroy "$STACK"
docker::prune_configs "climate-mediator"
}
function destroy_package() {
read -p "Are you sure you want to destroy the $STACK stack? [y/N] " -n 1 -r
echo
if [[ ! $REPLY =~ ^[Yy]$ ]]; then
log info "Operation cancelled"
exit 0
fi
(
docker::stack_destroy "$STACK"
) || {
log error "Failed to destroy stack"
exit 1
}
(
docker::prune_configs "climate-mediator"
) || {
log error "Failed to prune configs"
exit 1
}
}


main() {
init_vars "$@"
import_sources

if [[ "${ACTION}" == "init" ]] || [[ "${ACTION}" == "up" ]]; then
if [[ "${CLUSTERED_MODE}" == "true" ]]; then
log info "Running package in Cluster node mode"
else
log info "Running package in Single node mode"
fi

initialize_package
elif [[ "${ACTION}" == "down" ]]; then
log info "Scaling down package"

docker::scale_services "$STACK" 0
elif [[ "${ACTION}" == "destroy" ]]; then
log info "Destroying package"
destroy_package
else
log error "Valid options are: init, up, down, or destroy"
fi
}

main "$@"
Comment on lines +58 to +82
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add proper variable declaration and error handling in main function.

Several improvements needed:

  1. CLUSTERED_MODE variable is undefined
  2. Missing validation for MODE value
  3. No cleanup handling for unexpected exits
+declare CLUSTERED_MODE
+
 main() {
   init_vars "$@"
   import_sources
 
+  # Validate MODE
+  if [[ "${MODE}" != "dev" && "${MODE}" != "prod" ]]; then
+    log error "Invalid MODE: ${MODE}. Must be 'dev' or 'prod'"
+    exit 1
+  }
+
+  # Set up cleanup trap
+  trap 'cleanup' EXIT
+
   if [[ "${ACTION}" == "init" ]] || [[ "${ACTION}" == "up" ]]; then
     if [[ "${CLUSTERED_MODE}" == "true" ]]; then
       log info "Running package in Cluster node mode"
     else
       log info "Running package in Single node mode"
     fi
 
     initialize_package
   elif [[ "${ACTION}" == "down" ]]; then
     log info "Scaling down package"
 
     docker::scale_services "$STACK" 0
   elif [[ "${ACTION}" == "destroy" ]]; then
     log info "Destroying package"
     destroy_package
   else
     log error "Valid options are: init, up, down, or destroy"
+    exit 1
   fi
 }
 
+function cleanup() {
+  # Perform any necessary cleanup
+  if [[ $? -ne 0 ]]; then
+    log error "Script failed. Cleaning up..."
+    # Add cleanup logic here
+  fi
+}
+
 main "$@"

Committable suggestion skipped: line range outside the PR's diff.

3 changes: 3 additions & 0 deletions config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ packages:
- database-postgres
- reprocess-mediator
- fhir-ig-importer
- datalake
- climate-mediator

profiles:
- name: cdr-dw
Expand All @@ -47,6 +49,7 @@ profiles:
- kafka-unbundler-consumer
- fhir-ig-importer
- reprocess-mediator
- datalake
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Looking at the config.yaml and docker-compose files, I can see that the climate-mediator has dependencies on OpenHIM, ClickHouse, and MinIO (datalake) services, which are all part of the cdr-dw profile. The climate-mediator should therefore be included in the cdr-dw profile since it needs to interact with these services.

The climate-mediator should be added to the cdr-dw profile

The climate-mediator depends on services that are part of the cdr-dw profile:

  • OpenHIM for routing and API management
  • ClickHouse for data storage
  • MinIO (datalake) for file storage
🔗 Analysis chain

Consider adding climate-mediator to appropriate profile

The climate-mediator package is added to the root packages list but not included in any profile. Based on the addition of datalake to the cdr-dw profile, should the climate-mediator also be included in this or another profile?

Please clarify:

  1. Which profile(s) should include the climate-mediator package?
  2. Are there any specific deployment requirements or dependencies that influenced this decision?
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if climate-mediator has dependencies on other packages in cdr-dw profile

echo "Checking climate-mediator dependencies..."
fd -t f "docker-compose.*\.yml$" -p climate-mediator/ --exec grep -l "depends_on\|external_links\|links" {}

Length of output: 204


Script:

#!/bin/bash
# Let's first check if climate-mediator exists and its location
fd climate-mediator

# Check the content of config.yaml to understand profiles and packages
cat config.yaml

# Search for any references to climate-mediator in docker compose files
fd -t f "docker-compose.*\.yml$" -x cat {}

Length of output: 131114

envFiles:
- cdr-dw.env

Expand Down
Loading
Loading