[ci](fix) license check#62495
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Blocking issue:
.github/workflows/license-eyes.yml: the new changed-file detection usesgit diff FETCH_HEAD...HEADafter checking out the PR head and fetching the base branch with--depth=1. In a normal Actions shallow checkout, that often leaves no merge base, so the workflow fails before it can build the incremental config. I reproduced this against this PR with:
git fetch --depth=1 origin f75194aef28615ea111ae0e1dfff99e8297e1cbd && git checkout FETCH_HEAD && git fetch --no-tags --depth=1 origin master && git diff --name-only FETCH_HEAD...HEAD
which returnsfatal: FETCH_HEAD...HEAD: no merge base.
Critical checkpoints:
- Goal of the current task: improve the license check by scoping PR runs to changed files and falling back to the full config when
.licenserc.yamlchanges. The current code does not reliably accomplish that goal because the changed-file computation can fail on ordinary PRs. - Modification size and focus: yes, the change is small and focused on one workflow.
- Concurrency: not applicable; this workflow executes sequential shell steps and introduces no shared-state concurrency concerns.
- Lifecycle or static initialization: not applicable.
- Configuration items: no new repository/runtime config was added.
- Incompatible changes: none.
- Functionally parallel code paths: push and manual-dispatch paths remain unchanged; the PR path is the one regressed by the new diff logic.
- Special conditional checks: the new
.licenserc.yamlconditional is reasonable, but it depends on the broken changed-file detection. - Test coverage: insufficient. No validation was added for the new shallow-fetch diff logic, and the regression is directly reproducible.
- Test results modified: none.
- Observability: existing workflow logs are sufficient to diagnose this failure.
- Transaction/persistence/data-write concerns: not applicable.
- FE/BE variable passing: not applicable.
- Performance: incremental checking is a good direction, but correctness needs to be restored first.
- Other issues: none beyond the blocking regression above.
|
/review |
There was a problem hiding this comment.
Blocking findings:
-
.github/workflows/license-eyes.yml:60uses${base_sha}..HEAD, which compares the PR head against the current tip ofgithub.base_refrather than the PR merge base. If the base branch advances after the PR branch is cut, files changed only on the base branch show up as modified here and get pulled intoadded_modified, so the incremental license check can start failing on unrelated files. This path needs merge-base semantics (...or equivalent) with the existing fallback to.licenserc.yamlwhen shallow history cannot resolve the merge base. -
.github/workflows/license-eyes.yml:73always setsconfig_file=.licenserc-incremental.yamlafter theACMRdiff, even when that diff is empty. In a delete-only PR,Generate incremental licensercis skipped, the incremental config file is never created, andCheck Licensestill tries to read it. That makes delete-only PRs fail the workflow incorrectly.
Critical checkpoint conclusions:
- Goal of the task: narrow license checking to the relevant PR files while still falling back to the full config when needed. The current patch does not fully achieve that goal because the pull-request diff logic is incorrect in the two cases above. No automated workflow test or repro step was added to prove the new behavior.
- Small, clear, focused change: Yes. The change is limited to one workflow file.
- Concurrency: Not applicable. This workflow step is sequential shell logic only.
- Special lifecycle / static initialization: Not applicable.
- Configuration changes: No new Doris runtime configuration items are introduced.
- Incompatible changes / rolling upgrade concerns: Not applicable.
- Parallel code paths: Yes. The
pushpath remains straightforward, but thepull_request_targetpath regresses because the changed-file selection no longer matches PR semantics. - Special conditional checks: Applicable, and currently incorrect. The new
config_file/added_modifiedconditions need adjustment to preserve correct behavior for base-branch drift and delete-only PRs. - Test coverage: Insufficient. There is no workflow-level validation for the new branches of the shell logic.
- Test result files modified: Not applicable.
- Observability: Existing logs are probably sufficient for this workflow; no additional observability is required here.
- Transaction / persistence / data-write concerns: Not applicable.
- FE/BE variable propagation: Not applicable.
- Performance: Acceptable for this workflow; no blocking performance issue found.
- Other issues: None beyond the two blocking correctness problems above.
| separator: "\n" | ||
| run: | | ||
| base_sha=$(git rev-parse FETCH_HEAD) | ||
| if ! all=$(git diff --name-only "${base_sha}..HEAD" 2>/dev/null); then |
There was a problem hiding this comment.
Using ${base_sha}..HEAD here compares the PR head against the current tip of the base branch, not against the PR merge base. If master advances after the branch is cut, files changed only on the base branch show up as M here and get copied into added_modified, so the incremental license check can fail on unrelated files. This needs merge-base semantics (... or equivalent), with the existing fallback to .licenserc.yaml when shallow history cannot resolve the merge base.
| git diff --name-only --diff-filter=ACMR "${base_sha}..HEAD" | ||
| echo "EOF" | ||
| } >> "$GITHUB_OUTPUT" | ||
| echo "config_file=.licenserc-incremental.yaml" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
config_file is set to .licenserc-incremental.yaml unconditionally here, even when git diff --diff-filter=ACMR returned no files. In a delete-only PR, Generate incremental licenserc is skipped, the incremental file is never created, and Check License still tries to read it. Please only set the incremental config when added_modified is non-empty, or skip the license step entirely for delete-only PRs.
|
/review |
There was a problem hiding this comment.
Blocking finding:\n\n1. .github/workflows/license-eyes.yml:60 still relies on \ after fetching the base branch with . In the normal shallow checkout used by GitHub Actions, that often leaves no merge base, so the changed-file detection falls back incorrectly or fails before the incremental config can be generated. I reproduced the exact sequence from this workflow against the current PR head:\n\n\n\nwhich returns . That means ordinary PR runs can still skip or break the license check instead of checking the actual changed files.\n\nCritical checkpoint conclusions:\n- Goal of the task: narrow license checking to relevant PR files while falling back to the full config when needed. The current code does not reliably accomplish that goal because the PR diff computation is still broken under shallow history. No test or validation step was added to prove this path.\n- Small, clear, focused change: Yes. The change is limited to one workflow file.\n- Concurrency: Not applicable. This is sequential workflow shell logic only.\n- Lifecycle or static initialization: Not applicable.\n- Configuration items: No new Doris runtime or repository configuration items were added.\n- Incompatible changes: None.\n- Functionally parallel code paths: Yes. The push and manual paths remain straightforward, but the PR path is still incorrect under the runner's checkout strategy.\n- Special conditional checks: Applicable. The fallback to \ is reasonable, but it depends on a diff command that cannot reliably establish PR changed files in shallow clones.\n- Test coverage: Insufficient. There is no workflow-level validation or repro coverage for the shallow-fetch case.\n- Test results modified: Not applicable.\n- Observability: Existing logs are sufficient to diagnose this workflow failure.\n- Transaction, persistence, and data-write concerns: Not applicable.\n- FE/BE variable passing: Not applicable.\n- Performance: Acceptable for this workflow; no blocking performance issue found.\n- Other issues: None beyond the blocking correctness issue above.
| separator: "\n" | ||
| run: | | ||
| base_sha=$(git rev-parse FETCH_HEAD) | ||
| if ! all=$(git diff --name-only "${base_sha}...HEAD" 2>/dev/null); then |
There was a problem hiding this comment.
This still fails in the runner's shallow-checkout layout. With the exact sequence used here:\n\n\n\n\n\nthere is often no merge base, and I reproduced that on this PR head locally in the Actions workspace (). So the PR path still cannot reliably compute changed files. This needs either enough history to guarantee a merge base, or a changed-files mechanism that does not depend on a shallow three-dot diff.
There was a problem hiding this comment.
Blocking finding:
.github/workflows/license-eyes.yml:60still relies ongit diff "${base_sha}...HEAD"after fetching the base branch with--depth=1. In the normal shallow checkout used by GitHub Actions, that often leaves no merge base, so the changed-file detection falls back incorrectly or fails before the incremental config can be generated. I reproduced the exact sequence from this workflow against the current PR head:
git fetch --depth=1 origin fde3732179486eab100e58500d2e53fdd9c68ff2 && git checkout --detach FETCH_HEAD && git fetch --no-tags --depth=1 origin master && base_sha=$(git rev-parse FETCH_HEAD) && git diff --name-only "${base_sha}...HEAD"
which returns fatal: ... no merge base. That means ordinary PR runs can still skip or break the license check instead of checking the actual changed files.
Critical checkpoint conclusions:
- Goal of the task: narrow license checking to relevant PR files while falling back to the full config when needed. The current code does not reliably accomplish that goal because the PR diff computation is still broken under shallow history. No test or validation step was added to prove this path.
- Small, clear, focused change: Yes. The change is limited to one workflow file.
- Concurrency: Not applicable. This is sequential workflow shell logic only.
- Lifecycle or static initialization: Not applicable.
- Configuration items: No new Doris runtime or repository configuration items were added.
- Incompatible changes: None.
- Functionally parallel code paths: Yes. The push and manual paths remain straightforward, but the PR path is still incorrect under the runner's checkout strategy.
- Special conditional checks: Applicable. The fallback to
.licenserc.yamlis reasonable, but it depends on a diff command that cannot reliably establish PR changed files in shallow clones. - Test coverage: Insufficient. There is no workflow-level validation or repro coverage for the shallow-fetch case.
- Test results modified: Not applicable.
- Observability: Existing logs are sufficient to diagnose this workflow failure.
- Transaction, persistence, and data-write concerns: Not applicable.
- FE/BE variable passing: Not applicable.
- Performance: Acceptable for this workflow; no blocking performance issue found.
- Other issues: None beyond the blocking correctness issue above.
| separator: "\n" | ||
| run: | | ||
| base_sha=$(git rev-parse FETCH_HEAD) | ||
| if ! all=$(git diff --name-only "${base_sha}...HEAD" 2>/dev/null); then |
There was a problem hiding this comment.
This still fails in the runner's shallow-checkout layout. With the exact sequence used here:
git fetch --no-tags --depth=1 origin ${{ github.base_ref }}
base_sha=$(git rev-parse FETCH_HEAD)
git diff --name-only "${base_sha}...HEAD"
there is often no merge base, and I reproduced that on this PR head in the Actions workspace (fatal: ... no merge base). So the PR path still cannot reliably compute changed files. This needs either enough history to guarantee a merge base, or a changed-files mechanism that does not depend on a shallow three-dot diff.
|
skip buildall |
3 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)