fix: setup proxy variables in the start #32451
Conversation
WalkthroughWalkthroughThe recent changes focus on optimizing network configurations, enhancing Mixpanel event tracking, and improving Redis compatibility in the Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional Context UsedShellCheck (8)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -369,7 +374,7 @@ | |||
| echo "Compile Redis stable with page size of $page_size" | |||
| apt-get update | |||
| apt-get install --yes build-essential | |||
| curl --location https://download.redis.io/redis-stable.tar.gz | tar -xz -C /tmp | |||
| curl --connect-timeout 5 --location https://download.redis.io/redis-stable.tar.gz | tar -xz -C /tmp | |||
There was a problem hiding this comment.
Consider adding a --connect-timeout setting to the curl command used for downloading Redis source, to maintain consistency with other network requests in the script.
- curl --connect-timeout 5 --location https://download.redis.io/redis-stable.tar.gz | tar -xz -C /tmp
+ curl --connect-timeout 5 --location https://download.redis.io/redis-stable.tar.gz | tar -xz -C /tmpCommittable 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.
| curl --connect-timeout 5 --location https://download.redis.io/redis-stable.tar.gz | tar -xz -C /tmp | |
| curl --connect-timeout 5 --location https://download.redis.io/redis-stable.tar.gz | tar -xz -C /tmp |
There was a problem hiding this comment.
Actionable comments posted: 0
Actionable comments outside the diff hunks (2)
deploy/docker/fs/opt/appsmith/entrypoint.sh (2)
Line range hint
113-125: Consider declaring and assigning variables separately to avoid masking return values. This can help in identifying issues where commands fail but are not properly handled.- local generated_appsmith_mongodb_password=$(tr -dc A-Za-z0-9 </dev/urandom | head -c 13; echo "") + local generated_appsmith_mongodb_password + generated_appsmith_mongodb_password=$(tr -dc A-Za-z0-9 </dev/urandom | head -c 13; echo "")Apply similar changes to
generated_appsmith_encryption_password,generated_appsmith_encription_salt, andgenerated_appsmith_supervisor_password.
Line range hint
462-462: Ensure variables are double-quoted to prevent globbing and word splitting, which can lead to unexpected behavior.- su postgres -c "/usr/lib/postgresql/13/bin/createuser $1 -s" + su postgres -c "/usr/lib/postgresql/13/bin/createuser \"$1\" -s"
pratapaprasanna
force-pushed
the
use/setup/proxies/from/the/start
branch
from
ddc683e
to
02dc538
Compare
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/8704153760. |
|
Deploy-Preview-URL: https://ce-32451.dp.appsmith.com |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
pratapaprasanna
force-pushed
the
use/setup/proxies/from/the/start
branch
from
02dc538
to
347446f
Compare
| setup_proxy_variables | ||
| init_env_file |
There was a problem hiding this comment.
Shouldn't these be the other way around?
| setup_proxy_variables | |
| init_env_file | |
| init_env_file | |
| setup_proxy_variables |
pratapaprasanna
force-pushed
the
use/setup/proxies/from/the/start
branch
from
347446f
to
96d7266
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)
Line range hint
57-57: Consider separating declaration and assignment to avoid masking return values.- local generated_appsmith_mongodb_password=$(tr -dc A-Za-z0-9 </dev/urandom | head -c 13; echo "") + local generated_appsmith_mongodb_password + generated_appsmith_mongodb_password=$(tr -dc A-Za-z0-9 </dev/urandom | head -c 13; echo "")Apply similar changes for
generated_appsmith_encryption_password,generated_appsmith_encription_salt, andgenerated_appsmith_supervisor_password.Also applies to: 61-61, 65-65, 69-69
| setup_proxy_variables() { | ||
| export NO_PROXY="${NO_PROXY-localhost,127.0.0.1}" | ||
|
|
||
| echo "Running appsmith for cloudRun" | ||
| echo "creating mount point" | ||
| mkdir -p "$stacks_path" | ||
| echo "Mounting File Sytem" | ||
| mount -t nfs -o nolock "$FILESTORE_IP_ADDRESS:/$FILE_SHARE_NAME" /appsmith-stacks | ||
| echo "Mounted File Sytem" | ||
| echo "Setting HOSTNAME for Cloudrun" | ||
| export HOSTNAME="cloudrun" | ||
| fi | ||
| # Ensure `localhost` and `127.0.0.1` are in always present in `NO_PROXY`. | ||
| local no_proxy_lines | ||
| no_proxy_lines="$(echo "$NO_PROXY" | tr , \\n)" | ||
| if ! echo "$no_proxy_lines" | grep -q '^localhost$'; then | ||
| export NO_PROXY="localhost,$NO_PROXY" | ||
| fi | ||
| if ! echo "$no_proxy_lines" | grep -q '^127.0.0.1$'; then | ||
| export NO_PROXY="127.0.0.1,$NO_PROXY" | ||
| fi | ||
|
|
||
| # If one of HTTPS_PROXY or https_proxy are set, copy it to the other. If both are set, prefer HTTPS_PROXY. | ||
| if [[ -n ${HTTPS_PROXY-} ]]; then | ||
| export https_proxy="$HTTPS_PROXY" | ||
| elif [[ -n ${https_proxy-} ]]; then | ||
| export HTTPS_PROXY="$https_proxy" | ||
| fi | ||
|
|
||
| function get_maximum_heap() { | ||
| resource=$(ulimit -u) | ||
| echo "Resource : $resource" | ||
| if [[ "$resource" -le 256 ]]; then | ||
| maximum_heap=128 | ||
| elif [[ "$resource" -le 512 ]]; then | ||
| maximum_heap=256 | ||
| fi | ||
| # If one of HTTP_PROXY or http_proxy are set, copy it to the other. If both are set, prefer HTTP_PROXY. | ||
| if [[ -n ${HTTP_PROXY-} ]]; then | ||
| export http_proxy="$HTTP_PROXY" | ||
| elif [[ -n ${http_proxy-} ]]; then | ||
| export HTTP_PROXY="$http_proxy" | ||
| fi |
There was a problem hiding this comment.
Tip
Codebase Verification
The output from the verification script indicates that the entrypoint.sh script was sourced, but it encountered several issues, including permission denials and missing commands. Additionally, the script did not output the expected environment variables (NO_PROXY, HTTPS_PROXY, HTTP_PROXY) after sourcing, which was the primary goal of the test.
Given these issues, it is recommended to adjust the script to handle errors more gracefully and to ensure it outputs the necessary environment variables. A retest in a controlled environment where permissions and commands are not an issue might be necessary to verify the script's functionality thoroughly.
- The script encountered permission issues and missing commands, which prevented it from functioning as expected.
- The environment variables were not output after sourcing the script, indicating potential issues in the script's execution or environment setup.
Analysis chain
Refactored proxy setup looks good and should simplify maintenance.
The output from the verification script indicates that the environment variables NO_PROXY, HTTPS_PROXY, and HTTP_PROXY are not being set as expected. This could be due to the script not being executed in an environment where these variables are predefined, or there might be an issue with how the script sets or exports these variables.
To further investigate and ensure the script functions as intended, it would be beneficial to check the behavior of the script in an environment where these proxy variables are predefined. This will help confirm if the script correctly applies the proxy settings during deployment.
This script will help verify if the entrypoint.sh script correctly handles and applies the proxy settings when the environment variables are predefined.
The output from the verification script indicates that the entrypoint.sh script could not be sourced due to a path issue, which prevented the verification of the script's functionality. The environment variables NO_PROXY, HTTPS_PROXY, and HTTP_PROXY were correctly set before attempting to source the script, but without executing the script, we cannot confirm if it functions as intended.
To proceed, we need to correct the path issue and attempt to source the script again to verify its functionality.
This script will help verify if the entrypoint.sh script correctly handles and applies the proxy settings when the environment variables are predefined, addressing the path issue from the previous attempt.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the proxy settings are correctly applied during deployment.
# Test: Check the environment variables after deployment to ensure they are set correctly.
echo "NO_PROXY: $NO_PROXY"
echo "HTTPS_PROXY: $HTTPS_PROXY"
echo "HTTP_PROXY: $HTTP_PROXY"
Length of output: 102
Script:
#!/bin/bash
# Description: Verify the behavior of the proxy settings script in an environment with predefined proxy variables.
# Predefine some proxy variables
export NO_PROXY="localhost,127.0.0.1"
export HTTPS_PROXY="https://proxy.example.com:443"
export HTTP_PROXY="http://proxy.example.com:80"
# Source the script to apply the proxy settings
source /opt/appsmith/entrypoint.sh
# Check the environment variables after sourcing the script to ensure they are set correctly.
echo "NO_PROXY: $NO_PROXY"
echo "HTTPS_PROXY: $HTTPS_PROXY"
echo "HTTP_PROXY: $HTTP_PROXY"
Length of output: 646
Script:
#!/bin/bash
# Description: Correct the path issue and verify the behavior of the proxy settings script in an environment with predefined proxy variables.
# Predefine some proxy variables
export NO_PROXY="localhost,127.0.0.1"
export HTTPS_PROXY="https://proxy.example.com:443"
export HTTP_PROXY="http://proxy.example.com:80"
# Correct the path to the script and source it to apply the proxy settings
source deploy/docker/fs/opt/appsmith/entrypoint.sh
# Check the environment variables after sourcing the script to ensure they are set correctly.
echo "NO_PROXY: $NO_PROXY"
echo "HTTPS_PROXY: $HTTPS_PROXY"
echo "HTTP_PROXY: $HTTP_PROXY"
Length of output: 816
| } | ||
|
|
||
| function setup_backend_heap_arg() { | ||
| if [[ ! -z ${maximum_heap} ]]; then |
There was a problem hiding this comment.
Simplify the condition check for clarity.
- if [[ ! -z ${maximum_heap} ]]; then
+ if [[ -n ${maximum_heap} ]]; thenCommittable 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.
| if [[ ! -z ${maximum_heap} ]]; then | |
| if [[ -n ${maximum_heap} ]]; then |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9031381974. |
|
Deploy-Preview-URL: https://ce-32451.dp.appsmith.com |
Summary by CodeRabbit