You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jedcunningham (via GitHub)" <gi...@apache.org> on 2026/01/06 23:42:45 UTC
[PR] Fix basic static checks comparing same commit with itself [airflow]
jedcunningham opened a new pull request, #60194:
URL: https://github.com/apache/airflow/pull/60194
The prek command was comparing `github.sha` with itself, which checked no files. Now uses `merge-base` to check all files changed in the PR against the base branch.
This ensures static checks are actually ran in PRs like #60168.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #60194:
URL: https://github.com/apache/airflow/pull/60194#discussion_r2666614907
##########
.github/workflows/basic-tests.yml:
##########
@@ -219,21 +219,40 @@ jobs:
python-version: ${{ steps.breeze.outputs.host-python-version }}
platform: ${{ inputs.platform }}
save-cache: true
- - name: Fetch incoming commit ${{ github.sha }} with its parent
+ - name: Fetch incoming commit ${{ github.sha }} with history
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
ref: ${{ github.sha }}
- fetch-depth: 2
+ fetch-depth: 50
persist-credentials: false
+ - name: Calculate merge-base for PR
+ id: merge-base
+ run: |
+ git fetch origin "$BASE_BRANCH" --depth=50
Review Comment:
The hardcoded depth value of 50 is repeated multiple times (lines 226, 231, 238, 239). Consider extracting this as an environment variable or input parameter to improve maintainability and make it easier to adjust if needed.
##########
.github/workflows/basic-tests.yml:
##########
@@ -219,21 +219,40 @@ jobs:
python-version: ${{ steps.breeze.outputs.host-python-version }}
platform: ${{ inputs.platform }}
save-cache: true
- - name: Fetch incoming commit ${{ github.sha }} with its parent
+ - name: Fetch incoming commit ${{ github.sha }} with history
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
ref: ${{ github.sha }}
- fetch-depth: 2
+ fetch-depth: 50
persist-credentials: false
+ - name: Calculate merge-base for PR
+ id: merge-base
+ run: |
+ git fetch origin "$BASE_BRANCH" --depth=50
+
+ # Try to find merge-base, deepen if not found
+ MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
+
+ if [ -z "$MERGE_BASE" ]; then
+ echo "Merge-base not found with depth=50, fetching more history..."
Review Comment:
The magic number 450 for the fallback deepen operation lacks explanation. Consider adding a comment explaining why this specific depth was chosen (e.g., 'Deepen to accommodate PRs with many commits or long-running branches').
```suggestion
echo "Merge-base not found with depth=50, fetching more history..."
# Deepen history significantly (by an additional 450 commits) to accommodate PRs with many commits
# or long-running branches where the merge-base is far back in history.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60194:
URL: https://github.com/apache/airflow/pull/60194#issuecomment-3721059707
So basically if you want to have prek to execute on "only your changes" from a branch you have PR to main with - do this (without running any of `git merge-base` thingies):
```
prek --from-ref maim
```
(we have it in our docs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60194:
URL: https://github.com/apache/airflow/pull/60194#issuecomment-3717980062
Nice catch @jedcunningham BTW :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on PR #60194:
URL: https://github.com/apache/airflow/pull/60194#issuecomment-3720294545
Nice, I'd considered just adding "^" but somehow I confused myself thinking that would only go 1 commit up - that if we had many commits on the branch it'd only look at the last one. I guess it must squash? Anyways, glad its working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60194:
URL: https://github.com/apache/airflow/pull/60194#issuecomment-3721050873
BTW. There is also another interesting thing I learned with `prek`
There is a very nice shortcut to not have to find `merge-base`.
* `git diff main..your_pr_branch`
* `git diff main...your_pr_branch`
Spot the difference .... there are THREE dots in second case. And the ... is actually showing the changes in your branch from the `merge-base`
And ... there are few other gotchas: https://darekkay.com/blog/git-commit-ranges/#git-diff
For example `git diff main...your-branch` is not reverse of `git diff your-branch...diff` as you would suspect. The first one shows **your** changes since merge-base, the second shows **SURPRISE** -> all the changes in `main` since `merge-base` 😱 😱 😱 😱 😱 😱 😱 😱 😱
Git is wild.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #60194: Fix basic static checks comparing same commit with itself
URL: https://github.com/apache/airflow/pull/60194
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60194:
URL: https://github.com/apache/airflow/pull/60194#issuecomment-3717842737
Superseded by https://github.com/apache/airflow/pull/60202
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60194:
URL: https://github.com/apache/airflow/pull/60194#issuecomment-3717979278
I merged my fix and rebased https://github.com/apache/airflow/pull/60168 to verify that it works (it should fail)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60194:
URL: https://github.com/apache/airflow/pull/60194#issuecomment-3721003819
> Ah, a merge commit. Confusing since it has 2 parents. Apparently the "first" parent is the commit from the base branch, which is why this works. TIL.
Correct. This is actually what we **very** heavily bank on (this why I knew it by heart).
Basically whole selective checks code is based on the fact that we have "merge" commit passed as github.sha and it's first parent is base branch.
https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/commands/ci_commands.py#L163:
```python
def get_changed_files(commit_ref: str | None) -> tuple[str, ...]:
if commit_ref is None:
return ()
cmd = [
"git",
"diff-tree",
"--no-commit-id",
"--name-only",
"-r",
commit_ref + "^",
commit_ref,
]
result = run_command(cmd, check=False, capture_output=True, text=True)
if result.returncode != 0:
get_console().print(
f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}"
)
return ()
changed_files = tuple(result.stdout.splitlines()) if result.stdout else ()
get_console().print("\n[info]Changed files:[/]\n")
get_console().print(changed_files)
get_console().print()
return changed_files
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #60194:
URL: https://github.com/apache/airflow/pull/60194#discussion_r2666643446
##########
.github/workflows/basic-tests.yml:
##########
@@ -219,21 +219,40 @@ jobs:
python-version: ${{ steps.breeze.outputs.host-python-version }}
platform: ${{ inputs.platform }}
save-cache: true
- - name: Fetch incoming commit ${{ github.sha }} with its parent
+ - name: Fetch incoming commit ${{ github.sha }} with history
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
ref: ${{ github.sha }}
- fetch-depth: 2
+ fetch-depth: 50
persist-credentials: false
+ - name: Calculate merge-base for PR
+ id: merge-base
+ run: |
+ git fetch origin "$BASE_BRANCH" --depth=50
+
+ # Try to find merge-base, deepen if not found
+ MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
+
+ if [ -z "$MERGE_BASE" ]; then
+ echo "Merge-base not found with depth=50, fetching more history..."
+ git fetch --deepen=450
+ git fetch origin "$BASE_BRANCH" --deepen=450
+ MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
+ fi
+
+ echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
+ env:
+ BASE_BRANCH: ${{ github.base_ref || 'main' }}
Review Comment:
```suggestion
git fetch origin "$BASE_BRANCH" --depth=$FETCH_DEPTH
# Try to find merge-base, deepen if not found
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
if [ -z "$MERGE_BASE" ]; then
echo "Merge-base not found with depth=$FETCH_DEPTH, fetching more history..."
git fetch --deepen=$DEEPEN
git fetch origin "$BASE_BRANCH" --deepen=$DEEPEN
MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
fi
echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
env:
BASE_BRANCH: ${{ github.base_ref || 'main' }}
FETCH_DEPTH: 50
DEEPEN: 450
```
##########
.github/workflows/basic-tests.yml:
##########
@@ -219,21 +219,40 @@ jobs:
python-version: ${{ steps.breeze.outputs.host-python-version }}
platform: ${{ inputs.platform }}
save-cache: true
- - name: Fetch incoming commit ${{ github.sha }} with its parent
+ - name: Fetch incoming commit ${{ github.sha }} with history
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
ref: ${{ github.sha }}
- fetch-depth: 2
+ fetch-depth: 50
persist-credentials: false
+ - name: Calculate merge-base for PR
+ id: merge-base
+ run: |
+ git fetch origin "$BASE_BRANCH" --depth=50
+
+ # Try to find merge-base, deepen if not found
+ MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD 2>/dev/null || echo "")
+
+ if [ -z "$MERGE_BASE" ]; then
+ echo "Merge-base not found with depth=50, fetching more history..."
+ git fetch --deepen=450
+ git fetch origin "$BASE_BRANCH" --deepen=450
+ MERGE_BASE=$(git merge-base "origin/$BASE_BRANCH" HEAD)
+ fi
+
+ echo "sha=${MERGE_BASE}" >> $GITHUB_OUTPUT
+ env:
+ BASE_BRANCH: ${{ github.base_ref || 'main' }}
Review Comment:
Small nit, maybe we can make the values parameterised for both `--depth` and `--deepen` from env :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60194:
URL: https://github.com/apache/airflow/pull/60194#issuecomment-3717851738
This is precisely why we needed `2` history - because in pull_request the `github.sha` is merge and it's parent is the commit that was in the target branch when the merge was created (i.e. last time the PR was rebased).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix basic static checks comparing same commit with itself [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #60194:
URL: https://github.com/apache/airflow/pull/60194#issuecomment-3717990670
Yep. Nicely failed now https://github.com/apache/airflow/actions/runs/20776566467/job/59663769292?pr=60168
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org