chore: Update incorrect docker tagging and add fargate tag in deployment telemetry #33472
Conversation
WalkthroughWalkthroughThe recent changes to the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Script
User->>Script: Run generate-infra-details.sh
Script->>Script: Set default cloud_provider to "others(including local)"
Script->>Script: Set default dep_tool to "likely docker"
Script->>Script: Call check_for_fargate()
Script->>Script: Determine if Fargate deployment
Script-->>User: Output deployment details
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional Context UsedShellCheck (5)
Additional comments not posted (3)
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 (
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/generate-infra-details.sh (1)
Line range hint
68-68: Quotes/backslashes in this variable will not be respected. Double quote to prevent globbing and word splitting.- echo $infra_json > $infra_file + echo "$infra_json" > "$infra_file"
trishaanand
changed the title
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9096240170. |
|
Deploy-Preview-URL: https://ce-33472.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/generate-infra-details.sh (1)
Line range hint
68-68: Ensure special characters are handled properly when redirecting output to a file.- echo $infra_json > $infra_file + echo "$infra_json" > "$infra_file"
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/generate-infra-details.sh (1)
Line range hint
74-74: Ensure proper handling of quotes and backslashes when writing JSON to a file.- echo $infra_json > $infra_file + echo "$infra_json" > "$infra_file"
| cloud_provider="digitalocean" | ||
| elif [[ $release_details == *"ecs"* ]];then | ||
| cloud_provider="alibaba" | ||
| elif [[ ! -z "${DYNO}" ]];then |
There was a problem hiding this comment.
Improve readability by using -n instead of ! -z.
- elif [[ ! -z "${DYNO}" ]];then
+ elif [[ -n "${DYNO}" ]];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.
| elif [[ ! -z "${DYNO}" ]];then | |
| elif [[ -n "${DYNO}" ]];then |
There was a problem hiding this comment.
@sharat87, the change to use -n instead of ! -z has been noted and is indeed a good improvement for readability. This change will make the condition more straightforward and easier to understand. Thank you for pointing this out!
If there's anything else you need, feel free to ask!
| ## Check if its a fargate deployment | ||
| function check_for_fargate() { | ||
| if [[ $cloud_provider == "amazon" && $dep_tool == "likely docker" && $efs == "present" ]]; then | ||
| dep_tool="ecs-fargate" | ||
| fi | ||
| } |
There was a problem hiding this comment.
The implementation of check_for_fargate correctly identifies Fargate deployments. Consider adding a comment explaining the conditions for clarity.
+ # Check if the deployment is on AWS Fargate
if [[ $cloud_provider == "amazon" && $dep_tool == "likely docker" && $efs == "present" ]]; then
dep_tool="ecs-fargate"
fiCommittable 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.
| ## Check if its a fargate deployment | |
| function check_for_fargate() { | |
| if [[ $cloud_provider == "amazon" && $dep_tool == "likely docker" && $efs == "present" ]]; then | |
| dep_tool="ecs-fargate" | |
| fi | |
| } | |
| ## Check if its a fargate deployment | |
| function check_for_fargate() { | |
| # Check if the deployment is on AWS Fargate | |
| if [[ $cloud_provider == "amazon" && $dep_tool == "likely docker" && $efs == "present" ]]; then | |
| dep_tool="ecs-fargate" | |
| fi | |
| } |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9206524337. |
|
Deploy-Preview-URL: https://ce-33472.dp.appsmith.com |
| cloud_provider="azure"; | ||
| elif [[ $release_details == *"cloud"* ]];then | ||
| cloud_provider="gcp"; | ||
| elif [[ $release_details == *"generic"* ]];then | ||
| cloud_provider="digitalocean" | ||
| elif [[ $release_details == *"ecs"* ]];then | ||
| cloud_provider="alibaba" |
There was a problem hiding this comment.
This if/elif chain is quickly getting out of hand. Can we add a comment to these with a full example string to help a bit please? Like:
| cloud_provider="azure"; | |
| elif [[ $release_details == *"cloud"* ]];then | |
| cloud_provider="gcp"; | |
| elif [[ $release_details == *"generic"* ]];then | |
| cloud_provider="digitalocean" | |
| elif [[ $release_details == *"ecs"* ]];then | |
| cloud_provider="alibaba" | |
| cloud_provider="azure"; | |
| elif [[ $release_details == *"cloud"* ]];then | |
| # Example: abcd.cloud-magic.com | |
| cloud_provider="gcp"; | |
| elif [[ $release_details == *"generic"* ]];then | |
| # Example: generic-cant-be-bothered-to-name-better.com | |
| cloud_provider="digitalocean" | |
| elif [[ $release_details == *"ecs"* ]];then | |
| cloud_provider="alibaba" |
Can we fix the indentation as well please?
| cloud_provider="digitalocean" | ||
| elif [[ $release_details == *"ecs"* ]];then | ||
| cloud_provider="alibaba" | ||
| elif [[ ! -z "${DYNO}" ]];then |
| @@ -47,14 +53,21 @@ function get_current_time(){ | |||
| currentTime="$(date -u -Iseconds)" | |||
| } | |||
|
|
|||
| ## Check if its a fargate deployment | |||
There was a problem hiding this comment.
| ## Check if its a fargate deployment | |
| # Check if it's a ECS Fargate deployment |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
deploy/docker/fs/opt/appsmith/generate-infra-details.sh (2)
12-12: Update comments to reflect the new default values forcloud_provider.Consider updating the example comments in the
get_cloud_providerfunction to reflect the new default value ofcloud_provideras "others(including local)" where applicable.Also applies to: 15-15, 18-18, 21-22, 24-24, 26-26, 28-28
Line range hint
78-78: Handle quotes and backslashes correctly in JSON output.- echo "$infra_json" + echo "$infra_json" | jq .Ensure proper handling of quotes and backslashes in JSON output to prevent errors in data formatting.
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
馃攳 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?