Skip to content

Rtl #36765

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.

Sign up for GitHub

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

Closed
hajrezvan wants to merge 8 commits into from
Closed

Rtl #36765

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
626a46b
🐛 Debug file
hajrezvan Oct 8, 2024
808a553
🔧 Change depot to docker
hajrezvan Oct 8, 2024
dd9c9c7
🐛 change depot-ubuntu to ubuntu
hajrezvan Oct 8, 2024
b840761
🐛 change depot to docker
hajrezvan Oct 8, 2024
2f773fa
🔧 change depot to docker
hajrezvan Oct 8, 2024
0277e6a
🔧 a bit change
hajrezvan Oct 8, 2024
92a2c05
🐛: change many config
hossein-heydari79 Feb 20, 2024
85afb20
Merge branch 'release' into rtl
hajrezvan Oct 9, 2024
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
29 changes: 15 additions & 14 deletions .github/workflows/github-release.yml
View file
Open in desktop
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

@coderabbitai coderabbitai Bot Oct 8, 2024

Copy link
Copy Markdown
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

@coderabbitai coderabbitai Bot Oct 9, 2024

Copy link
Copy Markdown
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
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

@coderabbitai coderabbitai Bot Oct 8, 2024

Copy link
Copy Markdown
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

@coderabbitai coderabbitai Bot Oct 9, 2024

Copy link
Copy Markdown
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)