Possibly incorrect aggregation?

[max-sixty]

Thanks for building this!

One issue today though — am I reading this wrong? It seems like the two failures are allowed, yet the job aggregates this as a failure?

https://github.com/PRQL/prql/actions/runs/5942757316

# ❌ Some of the required to succeed jobs failed 😢😢😢

🛈 Some of the allowed to fail jobs did not succeed.
🛈 Some of the allowed to be skipped jobs did not succeed.
📝 Job statuses:
📝 build-web → ✓ success [required to succeed or be skipped]
📝 check-links-book → ✓ success [required to succeed or be skipped]
📝 check-links-markdown → ❌ failure [allowed to fail]
📝 lint-megalinter → ✓ success [required to succeed or be skipped]
📝 nightly → ❌ failure [allowed to fail]
📝 publish-web → ⬜ skipped [required to succeed or be skipped]
📝 test-dotnet → ✓ success [required to succeed or be skipped]
📝 test-elixir → ✓ success [required to succeed or be skipped]
📝 test-java → ✓ success [required to succeed or be skipped]
📝 test-js → ✓ success [required to succeed or be skipped]
📝 test-lib → ✓ success [required to succeed or be skipped]
📝 test-php → ✓ success [required to succeed or be skipped]
📝 test-python → ✓ success [required to succeed or be skipped]
📝 test-rust → ✓ success [required to succeed or be skipped]
📝 test-rust-main → ✓ success [required to succeed or be skipped]

Upvote & Fund

• We're using Polar.sh so you can upvote and help fund this issue.
• We receive the funding once the issue is completed & confirmed by you.
• Thank you in advance for helping prioritize & fund our backlog.

[webknjaz]

Looks like an input validation issue. The allowed-skips input accepts either a comma-separated or a JSON list. But you're passing a mapping object here: https://github.com/PRQL/prql/actions/runs/5942757316/workflow#L477 (serialized as a string; the value is visible at https://github.com/PRQL/prql/actions/runs/5942757316/job/16118911482#step:2:4).

We'll need to error out if https://github.com/re-actors/alls-green/blob/223e4bb/src/normalize_needed_jobs_status.py#L45 returns a dictnon-list.

P.S. In your case, if you want to allow everything to be skipped, you'll have to pre-process the jobs dict before passing it as an action input, transforming it into an acceptable list format.

[max-sixty]

Thanks @webknjaz , that makes sense, appreciate you looking.

One question though — why does the report suggest that all the jobs are either allowed to fail or be skipped — i.e. the report seems to interpret the input correctly, even though it's incorrectly formatted? Or am I reading the report incorrectly?

[webknjaz]

i.e. the report seems to interpret the input correctly, even though it's incorrectly formatted?

Fair enough. Upon closer inspection, I think that there might be an actual bug with allowed-failures @ https://github.com/re-actors/alls-green/blob/223e4bb7a751b91f43eda76992bcfbf23b8b0302/src/normalize_needed_jobs_status.py#L162C12-L162C46.

[max-sixty]

I think this might be a case of it not working, after having made the suggested change:

https://github.com/PRQL/prql/actions/runs/5967503062/job/16189922489?pr=3391

📝 Job statuses:
📝 build-web → ✓ success [required to succeed or be skipped]
📝 check-links-book → ✓ success [required to succeed or be skipped]
📝 check-links-markdown → ✓ success [allowed to fail]
📝 lint-megalinter → ✓ success [required to succeed or be skipped]
📝 nightly → ❌ failure [allowed to fail]
📝 publish-web → ⬜ skipped [required to succeed or be skipped]
📝 test-dotnet → ✓ success [required to succeed or be skipped]
📝 test-elixir → ✓ success [required to succeed or be skipped]
📝 test-java → ✓ success [required to succeed or be skipped]
📝 test-js → ✓ success [required to succeed or be skipped]
📝 test-lib → ✓ success [required to succeed or be skipped]
📝 test-php → ✓ success [required to succeed or be skipped]
📝 test-python → ✓ success [required to succeed or be skipped]
📝 test-rust → ✓ success [required to succeed or be skipped]
📝 test-rust-main → ✓ success [required to succeed or be skipped]

[webknjaz]

I reproduced this locally with your inputs and I think I have a patch. I'm going to push a commit for you to test, but will probably not make a release just yet. Instead, I'll attempt adding some more comprehensive testing before doing that.

[webknjaz]

@max-sixty Try pinning the action to commit cf9edfcf932a0ed6b431433fa183829c68b30e3f. Though, the smoke-test in the PR is failing. So I'll need to inspect the GHA workflow to see if the test is incorrect or the fix is incomplete. Anyway, I already have some thoughts of refactoring, but that can't be done w/o proper testing, especially now that some major projects depend on the action and I don't want to break anybody...

[max-sixty]

Great, I put that in, I'll keep an eye on it!