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

Rtl #36765

Closed
wants to merge 8 commits into from
Closed

Rtl #36765

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions .github/workflows/github-release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,16 @@ jobs:
steps:
- name: Environment details
run: |
echo ${{ secrets.DOCKER_HUB_USERNAME }}
echo "PWD: $PWD"
echo "GITHUB_REF: $GITHUB_REF"
echo "GITHUB_SHA: $GITHUB_SHA"
echo "GITHUB_EVENT_NAME: $GITHUB_EVENT_NAME"
Comment on lines +25 to 29
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

Let's be careful with our secrets, students!

I noticed that we're echoing the DockerHub username, which is stored as a secret. While it's important to have visibility into our environment, we must be cautious about potentially exposing sensitive information in our logs.

Consider removing or masking the DockerHub username output:

- echo ${{ secrets.DOCKER_HUB_USERNAME }}
+ echo "DOCKER_HUB_USERNAME: ***"

This way, we maintain visibility without risking the exposure of our DockerHub credentials. Remember, in the world of DevOps, secrecy is golden!

📝 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
echo ${{ secrets.DOCKER_HUB_USERNAME }}
echo "PWD: $PWD"
echo "GITHUB_REF: $GITHUB_REF"
echo "GITHUB_SHA: $GITHUB_SHA"
echo "GITHUB_EVENT_NAME: $GITHUB_EVENT_NAME"
echo "DOCKER_HUB_USERNAME: ***"
echo "PWD: $PWD"
echo "GITHUB_REF: $GITHUB_REF"
echo "GITHUB_SHA: $GITHUB_SHA"
echo "GITHUB_EVENT_NAME: $GITHUB_EVENT_NAME"


- name: Login to DockerHub
uses: docker/login-action@v3.3.0
with:
username: ${{ secrets.DOCKER_HUB_USERNAME }}
password: ${{ secrets.DOCKER_HUB_ACCESS_TOKEN }}
- name: Get the version
id: get_version
run: |
Expand All @@ -40,7 +45,7 @@ jobs:
needs:
- prelude

runs-on: ubuntu-latest-4-cores
runs-on: ubuntu-latest

defaults:
run:
Expand Down Expand Up @@ -209,8 +214,8 @@ jobs:
- name: Checkout the merged commit from PR and base branch
uses: actions/checkout@v4

- name: Set up Depot CLI
uses: depot/setup-action@v1
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v2

- name: Download the client build artifact
uses: actions/download-artifact@v4
Expand Down Expand Up @@ -247,31 +252,27 @@ jobs:
run: |
scripts/generate_info_json.sh

# As pg docker image is continuously updated for each scheduled cron on release, we are using the nightly tag while building the latest tag
- name: Place server artifacts-es
run: |
if [[ -f scripts/prepare_server_artifacts.sh ]]; then
PG_TAG=nightly scripts/prepare_server_artifacts.sh
else
echo "No script found to prepare server artifacts"
exit 1
scripts/prepare_server_artifacts.sh
fi

Comment on lines +258 to 260
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

Let's think about error-proofing our workflow!

I see we've removed the safety net of checking if our script exists before running it. While I appreciate the confidence, remember that in the world of DevOps, we always prepare for the unexpected!

Consider this change:

-          scripts/prepare_server_artifacts.sh
+          if [[ -f scripts/prepare_server_artifacts.sh ]]; then
+            scripts/prepare_server_artifacts.sh
+          else
+            echo "Warning: prepare_server_artifacts.sh not found. Skipping this step."
+          fi

This way, our workflow won't stumble if the script goes missing. It's like having a backup lesson plan - always good to be prepared!

What do you think, class? Shall we add this extra layer of caution to our workflow?

📝 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
scripts/prepare_server_artifacts.sh
fi
if [[ -f scripts/prepare_server_artifacts.sh ]]; then
scripts/prepare_server_artifacts.sh
else
echo "Warning: prepare_server_artifacts.sh not found. Skipping this step."
fi

- name: Login to DockerHub
uses: docker/login-action@v1
uses: docker/login-action@v2
with:
username: ${{ secrets.DOCKER_HUB_USERNAME }}
password: ${{ secrets.DOCKER_HUB_ACCESS_TOKEN }}

- name: Build and push fat image
uses: depot/build-push-action@v1
- name: Build and push Docker image
uses: docker/build-push-action@v3
with:
context: .
push: true
platforms: linux/arm64,linux/amd64
build-args: |
APPSMITH_SEGMENT_CE_KEY=${{ secrets.APPSMITH_SEGMENT_CE_KEY }}
BASE=${{ vars.DOCKER_HUB_ORGANIZATION }}/base-${{ vars.EDITION }}:nightly
BASE=appsmith/base-${{ vars.EDITION }}:nightly
tags: |
${{ vars.DOCKER_HUB_ORGANIZATION }}/appsmith-${{ vars.EDITION }}:${{needs.prelude.outputs.tag}}
${{ vars.DOCKER_HUB_ORGANIZATION }}/appsmith-${{ vars.EDITION }}:latest
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:${{needs.prelude.outputs.tag}}
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:latest
Comment on lines +277 to +278
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

Don't forget to dot your i's and cross your t's!

Eagle-eyed students might have noticed that we're missing a new line at the end of our file. While it might seem trivial, following YAML best practices helps maintain consistency and can prevent unexpected behavior in some parsers.

Let's add that missing new line:

 ${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:${{needs.prelude.outputs.tag}}
 ${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:latest
+

Remember, attention to detail is what separates a good DevOps engineer from a great one!

📝 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
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:${{needs.prelude.outputs.tag}}
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:latest
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:${{needs.prelude.outputs.tag}}
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:latest
🧰 Tools
🪛 yamllint

[error] 278-278: no new line character at the end of file

(new-line-at-end-of-file)

Comment on lines +267 to +278
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

Excellent work on our Docker image building process!

Class, I'm impressed with the improvements you've made here. Let's review:

  1. Updating to docker/build-push-action@v3 is like upgrading our lab equipment - always a good move!
  2. Adding multi-platform support is fantastic. It's like making our project accessible in multiple languages - very inclusive!
  3. Using secrets for the Docker Hub username is top-notch security practice. Remember, treat your secrets like your diary - keep them private!

However, we're missing one small detail. Can anyone spot it? That's right - we're missing a newline at the end of our file! Let's fix that:

 ${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:${{needs.prelude.outputs.tag}}
 ${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:latest
+

Adding this newline is like putting a period at the end of a sentence - it's a small touch that makes everything complete and follows best practices.

Great job overall, class! Keep up the excellent work in making our workflow more robust and versatile.

📝 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
- name: Build and push Docker image
uses: docker/build-push-action@v3
with:
context: .
push: true
platforms: linux/arm64,linux/amd64
build-args: |
APPSMITH_SEGMENT_CE_KEY=${{ secrets.APPSMITH_SEGMENT_CE_KEY }}
BASE=${{ vars.DOCKER_HUB_ORGANIZATION }}/base-${{ vars.EDITION }}:nightly
BASE=appsmith/base-${{ vars.EDITION }}:nightly
tags: |
${{ vars.DOCKER_HUB_ORGANIZATION }}/appsmith-${{ vars.EDITION }}:${{needs.prelude.outputs.tag}}
${{ vars.DOCKER_HUB_ORGANIZATION }}/appsmith-${{ vars.EDITION }}:latest
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:${{needs.prelude.outputs.tag}}
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:latest
- name: Build and push Docker image
uses: docker/build-push-action@v3
with:
context: .
push: true
platforms: linux/arm64,linux/amd64
build-args: |
APPSMITH_SEGMENT_CE_KEY=${{ secrets.APPSMITH_SEGMENT_CE_KEY }}
BASE=appsmith/base-${{ vars.EDITION }}:nightly
tags: |
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:${{needs.prelude.outputs.tag}}
${{ secrets.DOCKER_HUB_USERNAME }}/appsmith-${{ vars.EDITION }}:latest
🧰 Tools
🪛 yamllint

[error] 278-278: no new line character at the end of file

(new-line-at-end-of-file)

4 changes: 2 additions & 2 deletions app/client/.husky/check-staged-files.sh
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ else
echo "Skipping server side check..."
fi

if [ "$is_client_change" -ge 1 ]; then
echo "Running client check..."
if [ "$is_client_change" -ge 1 ]; then
echo "Running client check ..."
npx lint-staged --cwd app/client
else
echo "Skipping client side check..."
Expand Down
2 changes: 1 addition & 1 deletion app/client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
],
"scripts": {
"analyze": "yarn cra-bundle-analyzer",
"start": "BROWSER=none REACT_APP_ENVIRONMENT=DEVELOPMENT REACT_APP_CLIENT_LOG_LEVEL=debug HOST=dev.appsmith.com craco --max_old_space_size=4096 start",
"start": "BROWSER=none EXTEND_ESLINT=true REACT_APP_ENVIRONMENT=DEVELOPMENT REACT_APP_CLIENT_LOG_LEVEL=debug HOST=0.0.0.0 craco --max_old_space_size=7168 start",
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

Attention to Configuration Consistency!

It appears that while the start script has been successfully updated, the test and test:ci scripts still reference https://dev.appsmith.com. To maintain consistency and avoid potential conflicts or unexpected behaviors, please update these scripts accordingly.

  • Update CYPRESS_BASE_URL in test scripts:
    • File: app/client/package.json
    • Lines:
      "test": "CYPRESS_BASE_URL=https://dev.appsmith.com cypress/test.sh",
      "test:ci": "CYPRESS_BASE_URL=https://dev.appsmith.com cypress/test.sh --env=ci",

Please ensure all instances of the old HOST value are updated to match the new configuration.

🔗 Analysis chain

Class, let's examine the changes in our development environment setup!

Now, pay attention to the modifications in our start script:

  1. We've changed the HOST from dev.appsmith.com to 0.0.0.0. This is like opening our classroom doors to everyone! It allows connections from any available network interface, which is excellent for testing across different devices.

  2. We've also increased our max_old_space_size from 4096 to 7168. Think of this as giving our application a bigger brain! It allows for more memory usage, which can be beneficial for larger projects or when working with complex data structures.

However, remember that with great power comes great responsibility. Make sure your development machine can handle this increased memory allocation.

Any questions about these changes?

Let's run a quick check to ensure our changes don't conflict with any other scripts:


Great effort so far! Let's ensure we're searching in the right places.

It looks like the previous attempt couldn't find the package.json file. No worries! We'll adjust our approach to locate all instances of package.json in the repository and then search within them for the specified patterns.

Here's a script to help us with that:

Let's run this script to verify that the changes we've made are consistent across all relevant package.json files.


Great job sticking with this! Let's dive deeper into our package.json file.

It seems our previous search didn't find the package.json file. No worries! We'll now target the specific location to ensure everything is in order.

Here's a new script to check the app/client/package.json file directly:

Let's execute this to confirm that our changes are consistent within the app/client/package.json file.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the new HOST and max_old_space_size values are used consistently across all scripts

# Test: Search for any other occurrences of dev.appsmith.com or the old max_old_space_size value
rg 'dev\.appsmith\.com|--max_old_space_size=4096' package.json

Length of output: 117


Script:

#!/bin/bash
# Description: Locate all package.json files and search for specific patterns to ensure consistency.

# Find all package.json files and search for occurrences of 'dev.appsmith.com' or '--max_old_space_size=4096'
fd 'package\.json' . -x grep -H 'dev\.appsmith\.com|--max_old_space_size=4096' {}

Length of output: 85


Script:

#!/bin/bash
# Description: Search for 'dev.appsmith.com' and '--max_old_space_size=4096' in app/client/package.json

rg 'dev\.appsmith\.com|--max_old_space_size=4096' app/client/package.json

Length of output: 233

"build": "./build.sh",
"build-airgap": "node download-assets.js && ./build.sh",
"build-local": "craco --max-old-space-size=4096 build --config craco.build.config.js",
Expand Down
2 changes: 1 addition & 1 deletion app/client/src/sagas/InitSagas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ import {
fetchFeatureFlagsInit,
fetchProductAlertInit,
} from "actions/userActions";
import { embedRedirectURL, validateResponse } from "./ErrorSagas";
import { validateResponse } from "./ErrorSagas";
import type { ApiResponse } from "api/ApiResponses";
import type { ProductAlert } from "reducers/uiReducers/usersReducer";
import type { FeatureFlags } from "ee/entities/FeatureFlag";
Expand Down