Address coderabbitai comments

Author: anlowee

tools/deployment/presto-clp/scripts/generate-configs.py renamed to tools/deployment/presto-clp/scripts/generate-user-env-vars-file.py

@@ -50,7 +50,7 @@ def main(argv=None) -> int:
             clp_config_file_path,
             clp_package_dir.resolve(),
         )
-        return False
+        return 1
 
     with open(clp_config_file_path, "r") as clp_config_file:
         clp_config = yaml.safe_load(clp_config_file)
@@ -103,28 +103,15 @@ def _add_clp_env_vars(
     clp_archive_output_storage_type = _get_config_value(
         clp_config, "archive_output.storage.type", "fs"
     )
-    if "fs" == clp_archive_output_storage_type:
-        clp_archives_dir = _get_config_value(
-            clp_config,
-            "archive_output.storage.directory",
-            str(clp_package_dir / "var" / "data" / "archives"),
-        )
-        if Path(clp_archives_dir).is_absolute():
-            env_vars["CLP_ARCHIVES_DIR"] = clp_archives_dir
-        else:
-            env_vars["CLP_ARCHIVES_DIR"] = str(clp_package_dir / clp_archives_dir)
-    elif "s3" == clp_archive_output_storage_type:
-        # This will not be used, just to ensure CLP_ARCHIVES_DIR is not an empty string
-        clp_archives_dir = _get_config_value(
-            clp_config,
-            "archive_output.storage.directory",
-            str(clp_package_dir / "var" / "data" / "staged-archives"),
-        )
-        if Path(clp_archives_dir).is_absolute():
-            env_vars["CLP_ARCHIVES_DIR"] = clp_archives_dir
-        else:
-            env_vars["CLP_ARCHIVES_DIR"] = str(clp_package_dir / clp_archives_dir)
-
+    env_vars["CLP_ARCHIVES_DIR"] = _resolve_archives_dir(
+        clp_package_dir,
+        _get_config_value(clp_config, "archive_output.storage.directory"),
+        clp_package_dir
+        / "var"
+        / "data"
+        / ("archives" if clp_archive_output_storage_type == "fs" else "staged-archives"),
+    )
+    if "s3" == clp_archive_output_storage_type:
         s3_config_key_prefix = f"archive_output.storage.s3_config"
         s3_credentials_key_prefix = f"{s3_config_key_prefix}.aws_authentication.credentials"
 
@@ -174,6 +161,11 @@ def _add_clp_env_vars(
     return True
 
 
+def _resolve_archives_dir(base_dir: Path, configured: Optional[str], default: Path) -> str:
+    effective = configured or str(default)
+    return effective if Path(effective).is_absolute() else str(base_dir / effective)
+
+
 def _add_worker_env_vars(coordinator_common_env_file_path: Path, env_vars: Dict[str, str]) -> bool:
     """
     Adds environment variables for worker config values to `env_vars`.

tools/deployment/presto-clp/scripts/set-up-config.sh

@@ -23,6 +23,6 @@ echo "Installing required Python packages..."
 pip3 install -r "${script_dir}/requirements.txt"
 
 echo "Generating environment variables file for user-configured properties..."
-python3 "${script_dir}/generate-configs.py" \
+python3 "${script_dir}/generate-user-env-vars-file.py" \
     --clp-package-dir "${clp_package_dir}" \
     --output-file "${script_dir}/../.env"

tools/deployment/presto-clp/worker/scripts/generate-configs.sh

@@ -48,13 +48,19 @@ update_config_file() {
     local file_path=$1
     local key=$2
     local value=$3
+    local sensitive=${4:-false}
 
     if grep --quiet "^${key}=.*$" "$file_path"; then
         sed --in-place "s|^${key}=.*|${key}=${value}|" "$file_path"
     else
         echo "${key}=${value}" >>"$file_path"
     fi
-    log "INFO" "Set ${key}=${value} in ${file_path}"
+    if [[ "$sensitive" == "true" ]]; then
+        local masked="****${value: -4}"
+        log "INFO" "Set ${key}=${masked} in ${file_path}"
+    else
+        log "INFO" "Set ${key}=${value} in ${file_path}"
+    fi
 }
 
 apt-get update && apt-get install --assume-yes --no-install-recommends jq wget
@@ -76,13 +82,22 @@ mv "${PRESTO_CONFIG_DIR}/clp.properties" "${PRESTO_CONFIG_DIR}/catalog"
 
 # Update clp.properties
 readonly CLP_PROPERTIES_FILE="/opt/presto-server/etc/catalog/clp.properties"
-if [ -n "${PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER:-}" ]; then
+if [ "${PRESTO_WORKER_CLPPROPERTIES_STORAGE_TYPE:-}" = "s3" ]; then
     log "INFO" "Enable S3 support"
+    for var in PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER \
+           PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID \
+           PRESTO_WORKER_CLPPROPERTIES_S3_SECRET_ACCESS_KEY \
+           PRESTO_WORKER_CLPPROPERTIES_S3_END_POINT; do
+        if [ -z "${!var:-}" ]; then
+            log "ERROR" "Missing required env var: $var"
+            exit 1
+        fi
+    done
     update_config_file "$CLP_PROPERTIES_FILE" "clp.storage-type" "${PRESTO_WORKER_CLPPROPERTIES_STORAGE_TYPE}"
     update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-auth-provider" "${PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER}"
-    update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-access-key-id" "${PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID}"
+    update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-access-key-id" "${PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID}" true
     update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-end-point" "${PRESTO_WORKER_CLPPROPERTIES_S3_END_POINT}"
-    update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-secret-access-key" "${PRESTO_WORKER_CLPPROPERTIES_S3_SECRET_ACCESS_KEY}"
+    update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-secret-access-key" "${PRESTO_WORKER_CLPPROPERTIES_S3_SECRET_ACCESS_KEY}" true
 fi
 
 # Update config.properties