Closed binary-ho closed 4 weeks ago
The CI workflow has been significantly enhanced to improve efficiency and clarity. A new job, ci-target-check
, has been introduced to conditionally determine which parts of the codebase require builds, benchmarks, or tests based on recent changes. This optimization results in streamlined execution of the build, bench, and sharding-test jobs, ensuring they only run when relevant files are modified. Overall, these modifications foster a more responsive and maintainable CI process.
Files | Change Summary |
---|---|
.github/workflows/ci.yml |
Added ci-target-check job to optimize CI workflow; reorganized steps; improved conditional execution for existing jobs (build , bench , sharding-test ); clarified linting steps. |
🐇 In the land of code, we hop and play,
With new checks in place, we brighten the day.
Builds run swift, tests dance with delight,
A streamlined CI, oh what a sight!
Bouncing through paths, we’re agile and free,
Celebrating changes, just you wait and see! 🌟
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 51.06%. Comparing base (
80c6ea0
) to head (57ea87b
). Report is 2 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@binary-ho If you are still working on the PR and it is not ready for the review, I recommend to create PR in draft mode.
thank you for review kevin As discussed, I have organized the dependencies for the bench tests. As a result, the benchmark tests will be triggered by changes in the following packages. Could you please check this?
bench:
- 'admin/**'
- 'api/converter/**'
- 'api/types/**'
- 'client/**'
- 'pkg/document/**'
- 'pkg/locker/**'
- 'server/**'
- 'test/helper/**'
"pkg/document"
"pkg/document/json"
"pkg/document/presence"
"pkg/document/change"
"pkg/document/key"
"pkg/document/time"
"pkg/document/crdt"
"pkg/locker"
"server"
"server/backend"
"server/backend/database"
"server/backend/sync"
"server/documents"
"server/logging"
"server/packs"
"server/profiling"
"server/profiling/prometheus"
"server/clients"
"server/projects"
"server/rpc"
"api/types"
"api/converter"
"client"
"test/helper"
(For reference)
document_bench_test
"pkg/document"
grpc_bench_test
"admin"
"api/types"
"client"
"pkg/document"
"pkg/document/json"
"pkg/document/presence"
"server"
"server/backend/database"
"server/logging"
"test/helper"
locker_bench_test
"pkg/locker"
push_pull_bench_test
"api/converter"
"api/types"
"pkg/document"
"pkg/document/change"
"pkg/document/json"
"pkg/document/key"
"pkg/document/presence"
"pkg/document/time"
"server/backend"
"server/backend/database"
"server/documents"
"server/packs"
"server/profiling/prometheus"
"test/helper"
sync_bench_test
"server/backend/sync"
text_editing_bench_test
"api/types"
"client"
"server/backend"
"server/backend/database"
"server/clients"
"server/profiling"
"server/profiling/prometheus"
"server/projects"
"server/rpc"
tree_editing_bench_test
"api/converter"
"pkg/document/crdt"
"pkg/document/json"
"test/helper"
@krapie , @binary-ho
For some reason the CI was broken in main. Now I've rolled back the main branch to the previous commit.
https://github.com/yorkie-team/yorkie/actions/runs/10402716854
The Reason build failed is that the checkout step was missing in the ci-target-check job.
The following step should be added:
- name: Check out code
uses: actions/checkout@v4
The ci-target-check job should be updated as follows:
jobs:
ci-target-check:
...
steps:
- name: Check out code
uses: actions/checkout@v4
- name: CI target check by path
uses: dorny/paths-filter@v3
id: ci-target-check
with:
filters: |
build: '**'
bench:
- 'pkg/**'
- 'server/**'
- 'client/**'
- 'admin/**'
- 'api/converter/**'
sharding-test:
- 'server/backend/database/**'
The reason why the PR build succeeded was that “dorny/paths-filter@v3” did not require a checkout in the PR.
I misunderstood this comment and removed it entirely.
This issue was found by @devleejb. and @krapie has confirmed the issue and will take action to address it.
Thank you, devleejb and krapie
@binary-ho I will merge the following PR: https://github.com/yorkie-team/yorkie/pull/965 soon. Therefore, you don't have to take further actions :)
1. What this PR does / why we need it:
The execution time of the jobs included in the CI was too long. We had to wait longer to receive results due to unnecessary tests.
To prevent unnecessary CI tests from running, each CI test now only runs when there are changes related to the corresponding code.
Target packages:
2. Which issue(s) this PR fixes
Fine-tune CI Workflows in PR fixes #954
3. Special notes for your reviewer
Please check the target packages for each CI job.
Although I tried to classify them on my own, there are a few parts that feel ambiguous.
3.1 Build test target
There’s no specific target in the CI yml file, but I added
yorkie/design/**
to paths-ignore.If there was a particular reason for not including the design package in the related issue #873, please let me know!
As a result, changes to the following targets will be ignored. (No CI tests will run)
If there are any other targets that should be added, please let me know.
Additionally, a job has been added to check the packages affected by changes.
And each job checks the flag values as follows and executes the job if the value is true.
3.2 Bench test target
By default, all packages under
yorkie/pkg
are included. I marked these withO
in the diagram below.The parts that felt ambiguous are marked with
?
in the diagram, and I’ve left additional questions.3.2.1 (Question) Packages that need confirmation
Should the following packages be included or excluded in the benchmark targets?
yorkie/client
package → Since it mainly serves as a layer for making calls likeyorkie/api
, I want to exclude it. (I have excluded it for now.)yorkie/server/packs
package → This package defines functions for pushing or pulling changes. I believe onlyserver/packs
within the server package should be included in the bench target. Is this correct? (I have included it for now.)3.2.2 Packages excluded from the bench test target
yorkie/api
→ Excluded from the bench target since it only makes API callsyorkie/cmd
,yorkie/internal
,yorkie/design
→ Excluded from the bench targetyorkie/server
(excluding theyorkie/server/packs
package)3.3 Shard test target
yorkie/server/backend/database
3.4 Test Result
I tested to ensure everything works correctly.
3.4.1 Build test
Only the build will be executed when packages unrelated to CRDT and shard-test are modified.
→ Github Action Result: https://github.com/yorkie-team/yorkie/actions/runs/10387699150
3.4.2 Bench test
After modifying the packs.go file, both the build test and benchmark were executed out of the three jobs.
(The build failure was due to adding a
TODO
comment for testing purposes)→ Github Action Result: https://github.com/yorkie-team/yorkie/actions/runs/10387811024
3.4.3 Sharding-test
After modifying the server/backend/database/memory/database.go file, the sharding test was additionally executed.
(The build failure was due to adding a
TODO
comment for testing purposes)→ Github Action Result: https://github.com/yorkie-team/yorkie/actions/runs/10387851119
Summary by CodeRabbit
New Features
Improvements