🐛 Use allowed-failures and skips equally#24
Open
webknjaz wants to merge 2 commits intounstable/v1from
Open
Conversation
webknjaz
force-pushed
the
bugfixes/23-allowed-failures
branch
2 times, most recently
from
bd0610c to
ccb1cc9
Compare
Before this patch, `allowed-failures` did not contribute to the overall computed outcome due to a bug in the checking logic. This change refactors the data strucutures used to simplify the check and reduce its complexity in general. Fixes #23
webknjaz
force-pushed
the
bugfixes/23-allowed-failures
branch
from
0a2e10a to
646d949
Compare
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Pull request overview
Refactors the GitHub Action helper that aggregates needs.*.result values so that allowed-failures and allowed-skips contribute to the overall computed outcome via a unified “allowed outcomes per job” mapping (fixing incorrect aggregation scenarios like Issue #23).
Changes:
- Replace split
all(...) and all(...)aggregation logic with anallowed_outcome_maplookup per job. - Add verbose debug
print(...)statements to show inputs/intermediate decisions during aggregation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+169
to
172
| print(f'{jobs=}') | ||
| print(f'{jobs_allowed_to_fail=}') | ||
| print(f'{jobs_allowed_to_be_skipped=}') | ||
|
|
There was a problem hiding this comment.
The added debug print(f'{jobs=}') (and the following prints) can leak potentially sensitive data into GitHub Actions logs. jobs includes each needed job’s outputs, which may contain secrets depending on workflow usage. Please remove these prints or gate them behind an explicit debug flag and ensure any logged data excludes job outputs.
Suggested change
| print(f'{jobs=}') | |
| print(f'{jobs_allowed_to_fail=}') | |
| print(f'{jobs_allowed_to_be_skipped=}') | |
| # Optional debug logging: enable by setting DEBUG_NORMALIZE_NEEDED_JOBS=1. | |
| # Only non-sensitive information (job names and results) is logged; job outputs are excluded. | |
| if os.getenv('DEBUG_NORMALIZE_NEEDED_JOBS') == '1': | |
| jobs_debug_view = {name: {'result': job['result']} for name, job in jobs.items()} | |
| print(f'{jobs_debug_view=}') | |
| print(f'{jobs_allowed_to_fail=}') | |
| print(f'{jobs_allowed_to_be_skipped=}') |
| if job_name in jobs_allowed_to_be_skipped: | ||
| allowed_outcome_map[job_name].add('skipped') | ||
| if job_name in jobs_allowed_to_fail: | ||
| allowed_outcome_map[job_name].add('failure') |
There was a problem hiding this comment.
allowed_outcome_map currently allows only {success, failure} for jobs in allowed_failures. This changes existing behavior and breaks the documented/tested semantics where an allowed-failure job may also be skipped without affecting the overall outcome (see tests like success-despite-failure-and-skip). Consider adding 'skipped' (and potentially 'cancelled' to match “don’t succeed” wording) to the allowed outcomes for allowed_failures, while keeping allowed_skips limited to allowing 'skipped' only.
Suggested change
| allowed_outcome_map[job_name].add('failure') | |
| allowed_outcome_map[job_name].update({'failure', 'skipped', 'cancelled'}) |
Comment on lines
+199
to
+220
| print(f'{allowed_outcome_map=}') | ||
| print( | ||
| f"""{ | ||
| [ | ||
| job['result'] == 'success' for name, job in jobs.items() | ||
| if name not in (jobs_allowed_to_fail | jobs_allowed_to_be_skipped) | ||
| ]= | ||
| }""", | ||
| ) | ||
| print( | ||
| f"""{ | ||
| [ | ||
| (name, job['result'] in {'skipped', 'success'}) for name, job in jobs.items() | ||
| if name in jobs_allowed_to_be_skipped | ||
| ]= | ||
| }""", | ||
| ) | ||
| job_matrix_succeeded = all( | ||
| job['result'] == 'success' | ||
| job['result'] in allowed_outcome_map[name] | ||
| for name, job in jobs.items() | ||
| if name not in (jobs_allowed_to_fail | jobs_allowed_to_be_skipped) | ||
| ) and all( | ||
| job['result'] in {'skipped', 'success'} | ||
| for name, job in jobs.items() | ||
| if name in jobs_allowed_to_be_skipped | ||
| ) | ||
| print(f'{job_matrix_succeeded=}') |
There was a problem hiding this comment.
There are multiple additional debug prints here (including printing computed lists and job_matrix_succeeded) that will add noisy log output for every action run. Please remove them or use a structured/conditional debug logger controlled by an input/env var so normal runs stay clean.
webknjaz
moved this from 🤔 Brain 🧠 dump 💡
to 🚧 In progress 🚧
in 📅 Procrastinating in public 😵💫
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this patch,
allowed-failuresdid not contribute to the overall computed outcome due to a bug in the checking logic. This change refactors the data strucutures used to simplify the check and reduce its complexity in general.Fixes #23