Chore separate benchmark in ci/cd #9664
Conversation
WalkthroughA new GitHub Actions workflow, Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
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. 🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! 8000 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
.github/workflows/tests.yml (1)
279-279: Remove trailing spaces.There's a trailing whitespace at line 279 causing a linting error. Please remove it to adhere to YAML formatting standards.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 279-279: trailing spaces
(trailing-spaces)
.github/workflows/benchmark.yml (3)
64-65: Remove trailing spaces in run commands.YAML lint reports trailing spaces on these lines. Removing them will satisfy the linter:
- sed -i 's/traefik/localhost/g' .env - docker load --input /tmp/${{ env.IMAGE }}.tar + sed -i 's/traefik/localhost/g' .env + docker load --input /tmp/${{ env.IMAGE }}.tar🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 64-64: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
127-127: Add newline at end of file.A newline is required at EOF to conform with POSIX and YAML standards.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 127-127: no new line character at the end of file
(new-line-at-end-of-file)
79-87: (Optional) Pin Docker Compose installation for reproducibility.Downloading the latest compose manifest without version locking could lead to unexpected changes. Consider fetching a specific version or commit:
- curl https://appwrite.io/install/compose -o docker-compose.yml + curl https://appwrite.io/install/compose?version=1.2.3 -o docker-compose.yml
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/benchmark.yml(1 hunks).github/workflows/tests.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/tests.yml
[error] 279-279: trailing spaces
(trailing-spaces)
.github/workflows/benchmark.yml
[error] 64-64: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 127-127: no new line character at the end of file
(new-line-at-end-of-file)
🪛 actionlint (1.7.4)
.github/workflows/benchmark.yml
19-19: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
24-24: the runner of "docker/setup-buildx-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-27: the runner of "docker/build-push-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
42-42: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
.github/workflows/benchmark.yml (6)
3-5: Use a stable concurrency group key.
The current concurrency group${{ github.workflow }}-${{ github.ref }}includes the full ref (which changes per commit), preventing cancellation of previous runs on the same PR. Consider switching to${{ github.workflow }}-pr-${{ github.event.pull_request.number }}or${{ github.workflow }}-${{ github.head_ref }}to reliably cancel in-progress runs for the same PR.
7-10: Optimize cache key for cross-commit reuse.
BasingCACHE_KEYongithub.event.pull_request.head.shamakes it unique per commit, so your cache is never reused across commits on the same PR. If you want to benefit from caching during iterative runs, consider using the PR number or branch name in the key instead.
63-68: Remove trailing whitespace and improve readiness check.
Lines 64 and 65 have trailing spaces (lint error). Also, instead of a fixedsleep 10, consider waiting for the service health endpoint to be ready (e.g., polling/v1/health/version) to avoid flakiness.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 64-64: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
69-73: Add non-interactive install flags for apt.
sudo apt install ohamay hang waiting for confirmation. Usesudo apt install -y oha(orDEBIAN_FRONTEND=noninteractive).
89-104: Consider refactoring comment preparation.
The series ofechoandjqcommands work but are verbose and repetitive. You might extract common jq expressions into variables or a small helper script to improve readability and maintainability.
127-127: Add newline at end of file.
Static analysis flagged “no new line character at the end of file.” Please append a newline to satisfy POSIX conventions.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 127-127: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/benchmark.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/benchmark.yml
8000[error] 64-64: trailing spaces
(trailing-spaces)
[error] 65-65: trailing spaces
(trailing-spaces)
[error] 127-127: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: Benchmark
🔇 Additional comments (11)
.github/workflows/benchmark.yml (11)
11-11: Trigger only on pull_request events is appropriate.
Since this workflow is dedicated to benchmarking PR changes, restricting it toon: pull_requestis correct. No changes needed.
18-22: ✅ Updated checkout action to v4.
Theactions/checkout@v4withsubmodules: recursiveis correct and addresses the previous recommendation to bump from v3 to v4.
23-24: ✅ Bumpeddocker/setup-buildx-actionto v3.
This aligns with the latest workflow standards and satisfies the earlier actionlint suggestion.
26-35: Verifyoutputsparameter indocker/build-push-action.
Thewith:block usesoutputs: type=docker,dest=/tmp/${{ env.IMAGE }}.tar, but the documented flag inbuildx buildis--output(singular). Please confirm against docker/build-push-action@v6 docs whether this key should beoutputinstead ofoutputs.
41-46: ✅ Cache Appwrite image tarball withactions/cache@v4.
Usingactions/cache@v4to save and restore/tmp/${{ env.IMAGE }}.taris correct and consistent with other CI workflows.
74-75: ✅ Benchmark PR with Oha.
The command to run a 180‑second, JSON‑format benchmark against the health endpoint is correct.
76-86: ✅ Tear down and install latest version.
Cleaning up the PR environment and then fetching and starting the latest official Appwrite version is implemented correctly.
87-88: ✅ Benchmark latest release.
Running the same Oha benchmark against the latest version completes the comparison cycle as intended.
105-111: ✅ Upload benchmark artifact.
Usingactions/upload-artifact@v4with a 7‑day retention is appropriate for storing raw JSON results.
112-119: ✅ Locate existing bot comment.
peter-evans/find-comment@v3is configured correctly to detect a previous “Benchmark results” comment.
120-127: ✅ Create or update PR comment.
peter-evans/create-or-update-comment@v4inreplacemode will cleanly maintain a single up‑to‑date benchmark summary.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 127-127: no new line character at the end of file
(new-line-at-end-of-file)
✨ Benchmark results
⚡ Benchmark Comparison
|
What does this PR do?
Separated benchmark in CI/CD from tests so that they can run in parallel and tests can be restarted without waiting for benchmarking to complete.
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist
Summary by CodeRabbit
Summary by CodeRabbit