Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: preserve job result state in case of failure #1519

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Conversation

KnisterPeter
Copy link
Member

There is just one job field for the job result. This is also true for
matrix jobs. We need to preserve the failure state of a job to
have the whole job failing in case of one permuation of the matrix failed.

Closes #1518

There is just one job field for the job result. This is also true for
matrix jobs. We need to preserve the failure state of a job to
have the whole job failing in case of one permuation of the matrix failed.

Closes #1518
@KnisterPeter KnisterPeter self-assigned this Dec 19, 2022
Comment on lines 134 to +139
jobResult := "success"
jobResultMessage := "succeeded"
// we have only one result for a whole matrix build, so we need
// to keep an existing result state if we run a matrix
if len(info.matrix()) > 0 && rc.Run.Job().Result != "" {
jobResult = rc.Run.Job().Result
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too happy with this approach, but it is currently the right fix.
We should make this a bigger refactoring to integrate the continue-on-error for jobs and maybe rethink matrix handling.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 19, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 2 0 0.01s
✅ REPOSITORY gitleaks yes no 2.4s
✅ REPOSITORY git_diff yes no 0.0s
✅ REPOSITORY secretlint yes no 0.95s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@KnisterPeter KnisterPeter marked this pull request as ready for review December 19, 2022 14:24
@KnisterPeter KnisterPeter requested a review from a team as a code owner December 19, 2022 14:24
@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Merging #1519 (78394a9) into master (4989f44) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1519      +/-   ##
==========================================
+ Coverage   61.22%   61.26%   +0.04%     
==========================================
  Files          46       46              
  Lines        7141     7149       +8     
==========================================
+ Hits         4372     4380       +8     
  Misses       2462     2462              
  Partials      307      307              
Impacted Files Coverage Δ
pkg/runner/job_executor.go 88.40% <100.00%> (+0.71%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mergify mergify bot requested a review from a team December 19, 2022 14:45
This feature is not yet supported by act and if implemented
would make this test invalid
@mergify mergify bot merged commit b14398e into master Jan 10, 2023
@mergify mergify bot deleted the preserve-job-result branch January 10, 2023 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Act does not exit correctly when a job in a matrix fails
3 participants