kosiew commented on issue #22192:
URL: https://github.com/apache/datafusion/issues/22192#issuecomment-4477233446
@rluvaton,
Thanks for the detailed scenarios.
I agree the interaction is hard to reason about without spelling out each
baseline separately. The important rule in this PR is:
- `check-semver-advisory` compares PR HEAD against the PR base branch
(`main` / `apache/${BASE_REF}`). It is advisory only.
- `check-semver-blocking` compares PR HEAD against the latest stable release
tag. This is the blocking/user-facing signal.
- The auto-managed `auto detected api change` label is tied only to the
latest-release/blocking result.
- A sticky comment is present when either signal fails, and has separate
sections/logs for blocking latest-release and advisory base-branch results.
- A semver incompatibility itself does not make either Actions job red in
the current implementation; the job records `success`/`failure` in artifacts
for the comment/label workflow. The Actions job only fails for
workflow/script/infrastructure errors.
| Scenario | Label | Sticky comment | CI job status / signal |
|---|---|---|---|
| Revert all breaking changes from `main` | Remove/no `auto detected api
change`, because latest-release passes. No advisory label. | Add/update comment
because advisory fails. Blocking section: `success`, no release breakage.
Advisory section: `failure`, flags reverting `hello(arg)` -> `hello()` and
`world(a, b)` -> `world()` relative to unreleased `main`. |
`check-semver-advisory` records failure signal; `check-semver-blocking` records
success signal. Both Actions jobs stay green unless infra fails. |
| Revert one breaking change | Add/keep `auto detected api change`, because
latest-release still fails on `world`. | Add/update comment. Blocking section:
`failure`, flags `world()` -> `world(a, b)` relative to latest release.
Advisory section: `failure`, flags `hello(arg)` -> `hello()` relative to
`main`. | Advisory records failure; blocking records failure. Both Actions jobs
stay green unless infra fails. |
| Add another independent breaking change (`how`) | Add/keep `auto detected
api change`, because latest-release fails. | Add/update comment. Blocking
section: `failure`, flags existing release breakages `hello`, `world`, plus new
`how`. Advisory section: `failure`, flags only new `how` relative to `main`. |
Advisory records failure; blocking records failure. Both Actions jobs stay
green unless infra fails. |
| Add non-breaking change depending on existing `main` break (`hello` body
change) | Add/keep `auto detected api change`, because latest-release still
fails. | Add/update comment. Blocking section: `failure`, flags
release-baseline API breakages `hello` and `world` (the body-only `println!` is
not semver breakage). Advisory section: `success`. | Advisory records success;
blocking records failure. Both Actions jobs stay green unless infra fails. |
| Add non-breaking change to unbroken function (`how` body change) |
Add/keep `auto detected api change`, because latest-release still fails for the
changed crate. | Add/update comment. Blocking section: `failure`, flags
release-baseline API breakages `hello` and `world`; `how` body change is not a
semver breakage. Advisory section: `success`. | Advisory records success;
blocking records failure. Both Actions jobs stay green unless infra fails. |
| Modify existing breaking change (`hello(arg: u32)` -> `hello(arg: usize)`)
| Add/keep `auto detected api change`, because latest-release fails. |
Add/update comment. Blocking section: `failure`, flags `hello()` -> `hello(arg:
usize)` and `world()` -> `world(a, b)` relative to latest release. Advisory
section: `failure`, flags `hello(arg: u32)` -> `hello(arg: usize)` relative to
`main`. | Advisory records failure; blocking records failure. Both Actions jobs
stay green unless infra fails. |
So, yes: the latest-release check can report pre-existing unreleased
breakage in a changed crate. That is intentional for the release-baseline
signal because it answers “would this PR’s changed published crate be
compatible for users upgrading from the latest release?” The base-branch
advisory signal is the one that answers “did this PR introduce new API breakage
relative to `main`?”
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
