Apply allow-partial-results to JAS scans #223
Merged
Conversation
This file contains 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
…r capturing and handling (commented above them)
eranturgeman
had a problem deploying
to
frogbot
GitHub Actions
Failure
eranturgeman
commented
Nov 5, 2024
| @@ -242,7 +242,8 @@ func RunJasScans(auditParallelRunner *utils.SecurityParallelRunner, auditParams | |||
| } | |||
| auditParallelRunner.JasWg.Add(1) | |||
| if _, jasErr := auditParallelRunner.Runner.AddTaskWithError(createJasScansTasks(auditParallelRunner, scanResults, serverDetails, auditParams, jasScanner, jfrogAppsConfig), func(taskErr error) { | |||
| generalError = errors.Join(generalError, fmt.Errorf("failed while adding JAS scan tasks: %s", taskErr.Error())) | |||
| // TODO this change was for capturing a missed error that is coming from the threads | |||
| scanResults.AddGeneralError(fmt.Errorf("failed while adding JAS scan tasks: %s", taskErr.Error()), auditParams.AllowPartialResults()) | |||
There was a problem hiding this comment.
This change is a bug fix.
We missed capturing errors coming from the threads and tried to propagate them up the stack, where they "got lost" in the process or got override. This fix ensure we add the to the GeneralError so it will be examined later on in the process
eranturgeman
commented
Nov 5, 2024
| } | ||
| if generalError := runner.AddJasScannersTasks(params); generalError != nil { | ||
| if generalError = runner.AddJasScannersTasks(params); generalError != nil { | ||
| // TODO this fix was in order to avoid capturing the error twice when using partial-results. if this is disables the error is collected twice - once from the target error and once from general error | ||
| _ = targetResult.AddTargetError(fmt.Errorf("%s failed to add JAS scan tasks: %s", logPrefix, generalError.Error()), auditParams.AllowPartialResults()) |
There was a problem hiding this comment.
This is a bug fix.
After adding the error to TargetError (or logging it if partial results are allowed) we left generalError as it was and it propagated up the stack and caught again later in the process. this caused the error to be logged twice.
After handling it, due to the new way we handle errors, there is no need to propagate the error by returning it after we handle it once
…sed value of allowPartial to the relevant funcs
eranturgeman
had a problem deploying
to
frogbot
GitHub Actions
Failure
eranturgeman
had a problem deploying
to
frogbot
GitHub Actions
Failure
eranturgeman
added
improvement
github-actions
bot
removed
the
safe to test
eranturgeman
added
the
safe to test
github-actions
bot
removed
the
safe to test
eranturgeman
added
ignore for release
github-actions
bot
removed
the
safe to test
Merging this branch will not change overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
devbranch.go vet ./....go fmt ./....This PR introduces improvement for the Allow-Partial-Results ability that was previously applied only on SCA scan and now is applied of JAS scans throughout its process.
This enables the code to capture failures and continue running even if some of the JAS scans have failed. Errors that we cannot continue after them remained as is and will fail the flow.
This is a continuance for this PR: #200