-
Notifications
You must be signed in to change notification settings - Fork 4
Skip header check for merge to main PR #28
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
Conversation
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
license-header-check/action.yml
Outdated
| LICENSE_PATTERN="Copyright \(c\) .*?${CURRENT_YEAR},? NVIDIA CORPORATION." | ||
| # Skip for merge to main | ||
| PR_TITLE="${{ github.event.pull_request.title }}" |
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.
check title is not promising and could cause some issue, can we do the check based on the PR.base?
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, changed to use base.ref
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
license-header-check/action.yml
Outdated
| LICENSE_PATTERN="Copyright \(c\) .*?${CURRENT_YEAR},? NVIDIA CORPORATION." | ||
| # Skip for merge to main PR | ||
| if [[ "${{ github.event.pull_request.base.sha }}" == "main" ]]; then |
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.
Im not sure, sha could be the actual commit id, may try .ref
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 base.ref. test run: https://github.com/YanxuanLiu/spark-rapids/actions/runs/13404579617/job/37442083494?pr=22#step:4:33
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.
do we have a target main run log? thanks
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.
https://github.com/YanxuanLiu/spark-rapids/actions/runs/13404579617/job/37442083494?pr=24#step:4:33 it still got branch-25.02 since the feature branch is based on branch-25.02
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.
then we could try pass the ENV from caller side or try find some other event property for this case
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.
Corrected the commit history of forked repo and tested again, now it can get correct base.ref https://github.com/YanxuanLiu/spark-rapids/actions/runs/13406205693/job/37446469023?pr=25
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.
LGTM. please verify the case for skip checking merge to main when need to correct the base.refbase.ref == main before merge thanks
|
updated with latest run: https://github.com/YanxuanLiu/spark-rapids/actions/runs/13406205693 previous test failure was false alarm due to common commits in 2 alive PRs |

fix #27
Test run: https://github.com/YanxuanLiu/spark-rapids/actions/runs/13388432338/job/37390380186?pr=22