Adds script to detect breaking API changes/ semver #16541
Conversation
github-actions
bot
added
the
development-process
lucqui
force-pushed
the
detect-breaking-changes-script
branch
from
2af7bee to
ff0dd7e
Compare
|
Thanks @lucqui and @xudong963 -- this looks very cool |
Thanks @xudong963 will do! |
lucqui
force-pushed
the
detect-breaking-changes-script
branch
8 times, most recently
from
bb61f72 to
b950e65
Compare
|
@xudong963 generation of report worked w/ a public api change locally. |
lucqui
force-pushed
the
detect-breaking-changes-script
branch
from
1f0004f to
4e0e2f0
Compare
lucqui
changed the title
alamb
changed the title
There was a problem hiding this comment.
Thank you for this contribution @lucqui -- if we can get it working it will be super valuable
Local testing was performed. We will see this working in the ci process as well.
I think you can test it with a fork -- aka merge this code to your fork's main branch and then make a PR to your fork with a breaking change. That should trigger the workflow and we can see it in action!
.github/workflows/dev.yml
Outdated
|
|
||
| - name: Install GitHub CLI | ||
| run: | | ||
| if ! command -v gh &> /dev/null; then |
There was a problem hiding this comment.
As I understood it the Github hosted runners already have gh installed on them -- thus this command to setup and install apt-get seems unecessary
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: |
There was a problem hiding this comment.
We need the full history I believe.
.github/workflows/dev.yml
Outdated
| fetch-depth: 0 | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Install Rust toolchain |
There was a problem hiding this comment.
The other actions already have actions to setup rust -- can we reuse the same one please?
lucqui
force-pushed
the
detect-breaking-changes-script
branch
from
77ced4e to
bfc31bb
Compare
…analysis to detect API breaking changes in DataFusion PRs:- LogicalPlan enum modifications and variant removals - DataFrame API public method changes- Generates detailed reports and integrates with GitHub ActionsAddresses apache#16532
lucqui
force-pushed
the
detect-breaking-changes-script
branch
from
bfc31bb to
becb32b
Compare
…st toolchain installation (not needed for git-based analysis)- Remove GitHub CLI installation (pre-installed on GitHub runners) - Improve script robustness with consistent git diff range syntax- Make DataFrame API detection more specific to avoid false positivesAddresses reviewer comments on PR apache#16541
…Remove unnecessary token parameter (not needed for public repo)- Add clear comments explaining why fetch-depth: 0 is required- Improve base ref handling for GitHub Actions PR context- Use origin/$GITHUB_BASE_REF for proper remote branch referenceAddresses reviewer question about checkout configuration necessity.
|
Have we considered using a tool like cargo-semver-checks? It might be better to offload this semver checking logic to a tool dedicated for that purpose rather than rolling our own shell script. |
|
DataFusion doesn't currently maintain semver (we crank out major releases with semver breaking changes every month or two). So maybe we just need to ensure that are flagging each PR that has breaking changes appropriately (rather than blog the CI) cargo-semver-checks sounds good to me , though we need to make sure it is on the ASF approved list |
|
It seems we also have #15408 and #13665 👀 I guess we need to centralize this discussion again
I'll admit I was thinking of this from the POV of running |
Which issue does this PR close?
Closes #16532
Rationale for this change
Sometimes, we'll forget to add the breaking change label, so it'll be sweet if we can have such a script.
Follow the page to develop the script: https://datafusion.apache.org/contributor-guide/api-health.html#breaking-changes
What changes are included in this PR?
Are these changes tested?
Local testing was performed. We will see this working in the ci process as well.
Are there any user-facing changes?
No. This is a developer experience enhancement.