andygrove commented on PR #6072: URL: https://github.com/apache/datafusion-comet/pull/6072#issuecomment-5778789120
This looks good. I checked the conventions rather than assuming them: `ubuntu-slim`, `github-script@v9` and the `ci-nightly-failure` label are all used elsewhere in the repo, and `miri.yml` is schedule and dispatch only, so renaming the job from `Miri` to `Miri (<suite>)` cannot break a required check or the merge queue. The `tee` pipeline is right because `shell: bash` gives you `-eo pipefail`, which a bare `run:` would not. The fixture change checks out too. `gather_is_eligible` is `values.get_array_memory_size() <= GATHER_ELIGIBLE_CHILD_BYTES`, so reserving builder capacity drives exactly the quantity the predicate reads, and both assertions would fail loudly if that stopped being true. One thing. `ci.yml` uses `always() && ... result != 'success'` for its nightly report specifically so a cancelled run still gets reported, and this one uses `failure()`, which skips cancellation. With `cancel-in-progress: true` still on the concurrency group, a cancelled nightly would go unreported while the docs table this PR updates now says `miri.yml` reports its failures. Would matching `ci.yml`'s condition be worth it so the two behave the same way? Worth noting for later: this is the second copy of the nightly-issue script. Sharing the label and title format is what makes both workflows land on one issue, so the duplication is doing real work. Whoever adds the third one is the point at which a composite action pays for itself. -- 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]
