-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add github action for C++ clang-format
style check
#1597
Conversation
Signed-off-by: Yinqing Hao <[email protected]>
Signed-off-by: Yinqing Hao <[email protected]>
Please help provide some test runs (like in your forked repos), to show both success and failure cases, thanks |
also cc @ttnghia to help check if the auto-formatted changes look good thanks |
Signed-off-by: Yinqing Hao <[email protected]>
Signed-off-by: Yinqing Hao <[email protected]>
Signed-off-by: Yinqing Hao <[email protected]>
seems we should have enough time to merge this into 23.12 to avoid auto-merge commits to override the format checks. re-targeted it to branch-23.12 |
.github/workflows/clang-format.yml
Outdated
name: clang format check | ||
|
||
on: | ||
pull_request_target: |
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.
just saw you were not getting the correct merged commit sha with pull_request_target + pre-commit/action
.
then try using pull_request
event directly.
some other options could be that keep pull_request_target trigger, but then checkout required merge sha (this one could help avoid people touching workflow file directly to try pass CI unexpectedly)
with:
ref: "${{ github.event.pull_request.merge_commit_sha }}"
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.
Yes. Seems it always checkout the default branch in the tests. Will fix this. Thanks!
Signed-off-by: Yinqing Hao <[email protected]>
Signed-off-by: Yinqing Hao <[email protected]>
name: clang format check | ||
|
||
on: | ||
pull_request: |
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.
Changed to pull_request
event
just caught a new merged commit format issue at https://github.com/NVIDIA/spark-rapids-jni/actions/runs/7014779963/job/19083019630?pr=1597 👍 please upmerge and re-format to get a clean pass, we should be good to merge |
Signed-off-by: Yinqing Hao <[email protected]>
build |
Fix #202.
This PR includes two changes:
.clang-format
styleclang-format
style check