-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
ci: misc improvements #6575
base: develop
Are you sure you want to change the base?
ci: misc improvements #6575
Conversation
WalkthroughThe pull request modifies the GitHub Actions workflow in 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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? 🪧 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
CodeRabbit Configuration File (
|
@@ -28,14 +28,15 @@ jobs: | |||
uses: actions/checkout@v4 | |||
with: | |||
ref: ${{ github.event.pull_request.head.sha }} | |||
fetch-depth: 0 | |||
fetch-depth: 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't make it fetch-depth: 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we have to fetch the entire PR history / up to develop, for stuff like clang-diff-format linter and scripted diffs and whatnot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have some mechanism to warn pull requests with >50 commits that they should consolidate or split their PRs to be under this limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me, I was going to suggest something lower like 128 since there really is little reason for a PR that large to exist but since we aren't implementing a warning mechanism, 999 is more than enough to cover any PR, sane or otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly; no. I don't ever want to merge a PR more than 50 commits :D
@coderabbitai is wrong here:
source https://github.com/actions/checkout?tab=readme-ov-file#usage Going from 0 to 50 means going from all commits to up to 50 of them i.e. it's same or less commit history, not more of it. |
@UdjinM6 Thank you for the correction. You are absolutely right. I misinterpreted the ✏️ Learnings added
✅ Actions performedSummary regeneration triggered. |
Could probably get rid of some git commands now UdjinM6@133018f https://github.com/UdjinM6/dash/actions/runs/13303037024. However, I'm not 100% sure fetch-depth changes in general are going to work correctly... Could break |
@UdjinM6 I don't think that commit will work in a PR context; I think it'll only work in a push context |
I tried testing it by merging this PR + my suggestion into develop in my repo and creating another PR and it turns out that this PR breaks GH CI on develop: https://github.com/UdjinM6/dash/actions/runs/13317182118/job/37194741201 To fix this we could simply ask git to ignore its internal fetch checks:
https://git-scm.com/docs/git-fetch#Documentation/git-fetch.txt--u It does say that
but it should be safe to do this in GH CI imo. Here is what it looks like: UdjinM6@674855a https://github.com/UdjinM6/dash/actions/runs/13319312731. Now back to the original idea - pls check UdjinM6#22. It's a dummy PR to test 133018f. I don't see any issues 🤷♂️ |
Issue being fixed or feature implemented
See commits, but also:
Currently we fetch the entire history which takes about 15 seconds (https://github.com/dashpay/dash/actions/runs/13297199526/job/37131957876); with this patch, we fetch 50 commits of history and the ref develop only, resulting in the step taking only ~2 seconds. (https://github.com/PastaPastaPasta/dash/actions/runs/13300061362/job/37139947716)
Additionally, run unit tests in a dedicated step. May as well, gives better access to viewing ccache stats, and gives idea of how long unit tests are running.
Finally, compress and cleanup ccache. Github only gives us 10 gigs of caching, so whatever we can do to make that more efficient and make cache misses due to a cache being evicted less likely, I think are important
What was done?
How Has This Been Tested?
CI
Breaking Changes
Checklist:
Go over all the following points, and put an
x
in all the boxes that apply.