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.

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

fix: Reverted changes for skip client cache #37782

Merged
merged 1 commit into from
Nov 27, 2024
Merged

fix: Reverted changes for skip client cache #37782

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 17 additions & 17 deletions .github/workflows/client-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,23 +66,23 @@ jobs:
with:
fetch-tags: true

# - name: Get changed files in the client folder
# id: changed-files-specific
# uses: tj-actions/changed-files@v41
# with:
# files: "app/client/**"

- name: Updating the client changed file variable
- name: Get changed files in the client folder
id: changed-files-specific
run: echo "any_changed=true" >> "$GITHUB_OUTPUT"
uses: tj-actions/changed-files@v41
with:
files: "app/client/**"

#- name: Run step if any file(s) in the client folder change
# if: steps.changed-files-specific.outputs.any_changed == 'true'
# env:
# ALL_CHANGED_FILES: ${{ steps.changed-files-specific.outputs.all_changed_files }}
# run: |
# echo "One or more files in the server folder has changed."
# echo "List all the files that have changed: $ALL_CHANGED_FILES"
# - name: Updating the client changed file variable
# id: changed-files-specific
# run: echo "any_changed=true" >> "$GITHUB_OUTPUT"

- name: Run step if any file(s) in the client folder change
if: steps.changed-files-specific.outputs.any_changed == 'true'
env:
ALL_CHANGED_FILES: ${{ steps.changed-files-specific.outputs.all_changed_files }}
run: |
echo "One or more files in the server folder has changed."
echo "List all the files that have changed: $ALL_CHANGED_FILES"
Comment on lines +69 to +85
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

Version inconsistency detected in tj-actions/changed-files usage

The action is used with different versions across workflows:

  • client-build.yml: v41
  • server-build.yml: v44
  • ci-client-cyclic-deps-check.yml: v41

Please update all workflows to use the latest version (v44) for consistency and to benefit from the latest features and bug fixes.

Locations that need updates:

  • .github/workflows/client-build.yml:71
  • .github/workflows/ci-client-cyclic-deps-check.yml:32
🔗 Analysis chain

LGTM: Improved file change detection implementation

The switch to tj-actions/changed-files@v41 is a good improvement over the manual echo approach. The implementation correctly uses the action's outputs for conditional execution.

Let's verify the action's configuration:

Let me analyze the configuration details of the changed-files action across different workflows to ensure consistency and identify any potential version mismatches.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify tj-actions/changed-files configuration
# Check if the action is properly configured in other workflows
rg -p "tj-actions/changed-files" .github/workflows/

Length of output: 312

Script:

#!/bin/bash
# Check the configuration details of changed-files action across workflows
cat .github/workflows/client-build.yml .github/workflows/server-build.yml .github/workflows/ci-client-cyclic-deps-check.yml | awk '/tj-actions\/changed-files/,/with:/ { p=1 } p; /^[^ ]/ { p=0 }'

Length of output: 26401


- name: Check compliance
if: inputs.pr != 0 && steps.changed-files-specific.outputs.any_changed == 'true'
Expand Down Expand Up @@ -209,11 +209,11 @@ jobs:
git config --global user.email "$gituseremail"
git config --global user.name "$gituser"
git clone https://$cachetoken@github.com/appsmithorg/cibuildcache.git
git lfs install
git lfs migrate import --everything --yes
if [ "$reponame" = "appsmith" ]; then export repodir="CE"; fi
if [ "$reponame" = "appsmith-ee" ]; then export repodir="EE"; fi
cd cibuildcache/$repodir/release/client
git lfs install
git lfs migrate import --everything --yes
Comment on lines +215 to +216
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove redundant Git LFS installation

The git lfs install command at line 215 is redundant as it's already executed earlier in the step. This duplication may cause unnecessary processing.

Remove the redundant command:

-          git lfs install
           git lfs migrate import --everything --yes
📝 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
git lfs install
git lfs migrate import --everything --yes
git lfs migrate import --everything --yes

git lfs pull ./build.tar
mv ./build.tar ../../../../../build.tar

Expand Down