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 reporting job score panic #972

Merged
merged 2 commits into from
Nov 30, 2023
Merged

🐛 fix reporting job score panic #972

merged 2 commits into from
Nov 30, 2023

Conversation

imilchev
Copy link
Member

Fixes #960

I think this solves the problem we have seen in the issue above. I broke this in #943. There was even a discussion about that specific change https://github.com/mondoohq/cnspec/pull/943/files#r1392963805 The intention there was that we do not override scores if they already exist, since that was breaking the new exceptions logic.

I think I messed it up because with the flow in that PR there are situation where the score would be completely missing from the reporting job and that causes panics. I re-wrote the code to only set the score if it's not already set. That works for exceptions and also fixes the panic

Signed-off-by: Ivan Milchev <[email protected]>
@imilchev imilchev requested a review from arlimus November 30, 2023 17:01
Signed-off-by: Ivan Milchev <[email protected]>
Copy link
Member

@arlimus arlimus left a comment

Choose a reason for hiding this comment

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

Thanks for catching this! 🙏

@arlimus arlimus merged commit 4749e6c into main Nov 30, 2023
9 checks passed
@arlimus arlimus deleted the ivan/score-panic branch November 30, 2023 18:00
@github-actions github-actions bot locked and limited conversation to collaborators Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic: invalid score report
2 participants