fix(ci): make Chrome removal safe and guard missing Cypress results#41730
fix(ci): make Chrome removal safe and guard missing Cypress results#41730Arbab1308 wants to merge 3 commits intoappsmithorg:releasefrom
Conversation
WalkthroughUpdated GitHub Actions workflow Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi maintainers, could you please approve the pending workflows for this PR? Thanks! |
|
Hi maintainers — this PR fixes hosted CI by:
Could you please approve the pending workflows for this fork PR? 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci-test-hosted.yml:
- Around line 142-143: Replace the blanket suppression "|| true" on the apt
removal step so genuine apt failures still surface: before calling sudo apt-get
remove -y google-chrome-stable, check package presence with dpkg -s
google-chrome-stable (or similar existence test) and only run apt-get remove
when dpkg reports the package is installed; keep set -euxo pipefail so real
failures (network, permission, apt errors) are not masked by unconditional
suppression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 142ede29-a0a2-42f3-83e6-97def8d7b800
📒 Files selected for processing (1)
.github/workflows/ci-test-hosted.yml
Updated to avoid blanket suppression; now checks presence via dpkg -s before removing chrome. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3251
The implementation looks functionally solid on both changes:
One minor cosmetic nit — the - name: Install Google Chrome 129.0.6668.100
run: |
set -euxo pipefail
if dpkg -s google-chrome-stable >/dev/null 2>&1; then
sudo apt-get remove -y google-chrome-stable
else
echo "Google Chrome Stable is not installed; skipping removal."
fi
...
- name: Rename reports
if: failure()
run: |
mkdir -p ~/results
if [ -d "${{ github.workspace }}/app/client/results" ]; then
mv ${{ github.workspace }}/app/client/results ~/results/${{ matrix.job }}
else
echo "No cypress results directory found; skipping mv."
fiOtherwise the logic is clean — this is ready from a technical standpoint. The remaining blocker is the PR description warning (unfilled template), which the project maintainers may require before merging. 🧠 Learnings used |
“Applied dpkg presence check; also tidied formatting as suggested |
|
Applied dpkg presence check; also tidied formatting as suggested |
|
Requesting maintainer review/approval when possible. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
/ok-to-test |
Arbab1308
force-pushed
the
fix/ci-hosted-chrome-install-clean
branch
from
33d17af to
2954d76
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci-test-hosted.yml (1)
268-273: Tidy up theif/else/fiblock and quote the path.Functionally correct, but the
mvline isn't indented inside theif— a bit jarring to read. Also worth quoting the workspace path in case it ever contains spaces, and using theGITHUB_WORKSPACEenv var is slightly cleaner than${{ github.workspace }}interpolation inside shell.♻️ Suggested cosmetic cleanup
mkdir -p ~/results - if [ -d "${{ github.workspace }}/app/client/results" ]; then - mv ${{ github.workspace }}/app/client/results ~/results/${{ matrix.job }} - else + if [ -d "${{ github.workspace }}/app/client/results" ]; then + mv "${{ github.workspace }}/app/client/results" ~/results/${{ matrix.job }} + else echo "No cypress results directory found; skipping mv." fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-test-hosted.yml around lines 268 - 273, The if/else/fi block in the CI step is mis-indented and uses GitHub Actions interpolation inside the shell; fix it by properly indenting the mv under the if, quote the workspace path, and switch to the GITHUB_WORKSPACE env var (e.g., test for "${GITHUB_WORKSPACE}/app/client/results" and mv "${GITHUB_WORKSPACE}/app/client/results" "~/results/${{ matrix.job }}"); keep the mkdir -p ~/results and the echo branch intact and ensure the block ends with fi.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci-test-hosted.yml:
- Around line 268-273: The if/else/fi block in the CI step is mis-indented and
uses GitHub Actions interpolation inside the shell; fix it by properly indenting
the mv under the if, quote the workspace path, and switch to the
GITHUB_WORKSPACE env var (e.g., test for
"${GITHUB_WORKSPACE}/app/client/results" and mv
"${GITHUB_WORKSPACE}/app/client/results" "~/results/${{ matrix.job }}"); keep
the mkdir -p ~/results and the echo branch intact and ensure the block ends with
fi.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c2ac197-60ec-465a-a490-b17f6f6a94f9
📒 Files selected for processing (1)
.github/workflows/ci-test-hosted.yml
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
1 participant
Description
This PR makes the hosted Cypress CI workflow more robust on
ubuntu-latest.What changed
google-chrome-stableif it is actually installed (checked viadpkg -s), while keepingset -euxo pipefailso real failures still surface.app/client/resultsis not created (e.g., when Cypress fails before generating reports).Testing
Fixes
Summary by CodeRabbit