-
Notifications
You must be signed in to change notification settings - Fork 4
add license header check v2 #38
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]>
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, just a quick skim. Lets mark this as draft for now, we will get back to this in July to see if fulfill the latest requirement at that moment, thanks
Please also include update to the common repo directly in this change, as this one is main development based, thanks
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
|
It looks like simply adding a switch to the existing action may be more efficient here as we will always need to change the ref place. So old branches can keep using it with default value, and new release/branch-* will just need to add a param for the action. Thanks |
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
|
After discussion offline, added a flag Test runs: |
Signed-off-by: Yanxuan Liu <[email protected]>
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.
Pull Request Overview
Adds an optional flag to control whether the license header check runs on PRs targeting the main branch.
- Introduce a new
check_mainboolean input (default:false). - Update the composite action script to skip or perform the check on the
mainbranch based oncheck_main.
Comments suppressed due to low confidence (2)
license-header-check/action.yml:26
- The new
check_maininput isn't documented in the action's README or usage examples. Please update the documentation to include its purpose, default value, and usage.
check_main:
license-header-check/action.yml:45
- Consider adding or updating test cases to cover the scenario when
check_mainistrue, ensuring that license header checks run on PRs targeting the main branch.
if [[ "${{ inputs.check_main }}" == "false" ]]; then
license-header-check/action.yml
Outdated
| if [[ "${{ inputs.check_main }}" == "false" ]]; then | ||
| if [[ "${{ github.event.pull_request.base.ref }}" == "main" ]]; then | ||
| echo "Skip this check for Merge to main PR." | ||
| exit 0 | ||
| fi |
Copilot
AI
Jun 10, 2025
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.
[nitpick] The nested if blocks around check_main can be simplified into a single conditional to improve readability, for example: if [[ "${{ inputs.check_main }}" == "false" && "${{ github.event.pull_request.base.ref }}" == "main" ]]; then ... fi.
| if [[ "${{ inputs.check_main }}" == "false" ]]; then | |
| if [[ "${{ github.event.pull_request.base.ref }}" == "main" ]]; then | |
| echo "Skip this check for Merge to main PR." | |
| exit 0 | |
| fi | |
| if [[ "${{ inputs.check_main }}" == "false" && "${{ github.event.pull_request.base.ref }}" == "main" ]]; then | |
| echo "Skip this check for Merge to main PR." | |
| exit 0 | |
| fi |
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
Signed-off-by: Yanxuan Liu <[email protected]>
pxLi
left a comment
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.
Thanks! Please help file dummy PR of common repo to do a post-merge check~
test run works as expected. https://github.com/NVIDIA/spark-rapids-common/actions/runs/15549785211/job/43778010969?pr=39 |
fix #37
Add v2 for new branch strategy. Allow to check main branch
test run: https://github.com/YanxuanLiu/spark-rapids/actions/runs/15484134743/job/43595238006?pr=31