jiayuasu commented on PR #1095:
URL: https://github.com/apache/sedona-db/pull/1095#issuecomment-5220323446
Correction to my comment above: I claimed the `geopandas` job should keep a
plain `needs: ["expr"]` and that adding `if: ${{ !cancelled() }}` would be
wrong. The first half was wrong, and the run on the previous commit
demonstrated it.
What happened: `macOS-amd64` was cancelled after running long. `expr` ran
anyway thanks to its `!cancelled()` and `expr-build` succeeded — but
`geopandas` was **skipped**, because a cancelled job upstream in the graph
skips dependents that have not opted in. So the plain `needs` did not just
require `expr` to succeed, it also inherited the cancellation.
The part of my reasoning that was sound is that this job cannot simply copy
`!cancelled()` either: it downloads the `release-expr-wheels` artifact, and
`wheels-expr.yml` uploads that after its test step, so running when `expr`
failed would only produce a confusing "artifact not found" failure rather than
a clean skip. The condition now states both requirements explicitly:
```yaml
if: ${{ !cancelled() && needs.expr.result == 'success' }}
```
Do not skip merely because something upstream was cancelled, but do require
`expr` to have succeeded.
Worth noting this is the one class of bug that could not be caught by
reading the workflow — it needed the job to actually be reached in a run with a
cancelled platform, which is why it was worth waiting for an execution rather
than merging on the strength of the wiring alone. It still has not run end to
end; that remains pending on the wheel builds being stable enough for the chain
to complete.
--
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]
