Refactor commands output formats #96

attiasas · 2024-06-30T08:34:30Z

The pull request is targeting the dev branch.
The code has been validated to compile successfully by running go vet ./....
The code has been formatted properly using go fmt ./....
All static analysis checks passed.
All tests have passed. If this feature is not already covered by the tests, new tests have been added.
All changes are detailed at the description. if not already covered at JFrog Documentation, new documentation have been added.

Move all `Results` and outputs to their inner package, organize currently available code and renames to fit better.

Before:

After:

Move detection out of SCA to run first thing in audit

Allows to sync information from local jfrog-apps-config.
Prepare to remove config detection to separate command.
Prepare to remove dependency collection to separate command.

Change `Results` object to `ScanCommandResults`

Prepare the infrastructure to separate different sca children-targets in a single scan target

Separate converting `ScanCommandResults` to different output formats and printing the output.

output package - Write the results in a specific format as an output string.
conversion package - Parse the ScanCommandResults object to convert it to different formats that we are supporting.

Make implementing a new output format (by introducing an interface) easier, and maintain the currently supported.

Reduce code duplications
Easy to see what we support
Easy to implement new formats / integrate new scans and results

General validation improvments for results structs

Easy to support and maintain
Adds more tests
Exact/Partial match options (for unit + integration tests)
Easy to read what is tested
Refactor validations to its own package

github-actions · 2024-09-09T12:13:28Z

🐸 JFrog Frogbot

eranturgeman · 2024-09-12T11:51:46Z

commands/audit/audit_test.go

@@ -125,27 +297,15 @@ func TestAuditWithConfigProfile(t *testing.T) {
 			chdirCallback := clientTests.ChangeDirWithCallback(t, baseWd, tempDirPath)
 			defer chdirCallback()

+			auditParams.SetWorkingDirs([]string{tempDirPath}).SetIsRecursiveScan(true)


why is it necessary here? wouldn't it detect it alone without setting the working dirs?
Also, defining working dirs and recursive scan is different from what our flow does

eranturgeman · 2024-09-12T12:44:23Z

utils/results/output/securityJobSummary.go

@@ -187,11 +199,17 @@ func RecordSarifOutput(cmdResults *Results) (err error) {
 		log.Info("Results can be uploaded to Github security tab automatically by upgrading your JFrog subscription.")


I know it is not a change of yours but this condition doesn't seems right, as it is always evaluated to false:
!extended && !commandsummary.StaticMarkdownConfig.IsExtendedSummary()
(extended is set to 'true' just before checking the condition)

eranturgeman · 2024-09-12T13:07:55Z

jas/runner/jasrunner.go

+	Runner        *utils.SecurityParallelRunner
+	ServerDetails *config.ServerDetails
+	Scanner       *jas.JasScanner
+


Suggested change

Remove all new lines from the struct

eranturgeman · 2024-09-12T13:19:29Z

jas/runner/jasrunner.go

-	if len(scansToPreform) > 0 && !slices.Contains(scansToPreform, utils.ContextualAnalysisScan) {
-		log.Debug("Skipping contextual analysis scan as requested by input...")
-		return err
+func addJasScanTaskForModuleIfNeeded(params JasRunnerParams, subScan utils.SubScanType, task parallel.TaskFunc) (err error) {


I love how you re-wrote it, but we said this is the prioritization:

flags

config profile

jfrog apps config

it seems like we ignore the flags here that we can get from audit. Lets discuss it

eranturgeman · 2024-09-12T13:24:13Z

tests/testdata/output/dockerscan/docker_sarif.json

this file is empty. can we delete?

eranturgeman · 2024-09-12T13:24:45Z

tests/testdata/output/dockerscan/docker_simple_json.json

this file is empty. can we delete?

eranturgeman · 2024-09-12T14:00:23Z

utils/formats/sarifutils/sarifutils.go

@@ -65,6 +65,51 @@ func ReadScanRunsFromFile(fileName string) (sarifRuns []*sarif.Run, err error) {
 	return
 }

+func CopyResult(result *sarif.Result) *sarif.Result {


this functions doesn't seems to be used

eranturgeman · 2024-09-12T14:03:50Z

utils/formats/sarifutils/sarifutils.go

+	return nil
+}
+
+func GetRuleFullDescription(rule *sarif.ReportingDescriptor) string {


duplicated
see GetRuleFullDescriptionText

eranturgeman · 2024-09-12T14:05:16Z

utils/formats/sarifutils/sarifutils.go

+	return ""
+}
+
+func GetRuleShortDescription(rule *sarif.ReportingDescriptor) string {


duplicate
see GetRuleShortDescriptionText

eranturgeman · 2024-09-12T14:35:49Z

utils/results/conversion/sarifparser/sarifparser.go

+			location.PhysicalLocation = sarifutils.NewPhysicalLocation(patchedLocation)
+		}
+	}
+}


a return statement is missing in this funciton

eranturgeman · 2024-09-15T08:05:14Z

utils/validations/test_validate_sarif.go

+}
+
+func isPotentialSimilarResults(expected, actual *sarif.Result) bool {
+	return sarifutils.GetResultRuleId(actual) == sarifutils.GetResultRuleId(expected) && sarifutils.GetResultMsgText(actual) == sarifutils.GetResultMsgText(expected) && sarifutils.GetResultProperty(sarifparser.WatchSarifPropertyKey, actual) == sarifutils.GetResultProperty(sarifparser.WatchSarifPropertyKey, expected)


Should we compare by message? A text message is more likely to change in compare to ID or a property that we obtain
if you think it is not likely to break often we can leave it like that (or if it is absolutely necessary)

Refactor commands output formats

a10204d

attiasas added the ignore for release Automatically generated release notes label Jun 30, 2024

attiasas had a problem deploying to frogbot June 30, 2024 08:34 — with GitHub Actions Failure

some fixes

e1f0fbb

attiasas had a problem deploying to frogbot June 30, 2024 12:08 — with GitHub Actions Failure

Merge remote-tracking branch 'upstream/dev' into refactor_output

b7718f6

attiasas had a problem deploying to frogbot June 30, 2024 12:09 — with GitHub Actions Failure

more refactor

03946e9

attiasas had a problem deploying to frogbot June 30, 2024 12:24 — with GitHub Actions Failure

continue refactor

fe6d86d

attiasas had a problem deploying to frogbot June 30, 2024 12:53 — with GitHub Actions Failure

add parsers

9f129e4

attiasas had a problem deploying to frogbot June 30, 2024 14:34 — with GitHub Actions Failure

continue implement

fd8e025

attiasas had a problem deploying to frogbot July 1, 2024 13:45 — with GitHub Actions Failure

some changes

5fb51d9

attiasas had a problem deploying to frogbot July 2, 2024 06:54 — with GitHub Actions Failure

continue implement

adb3de1

attiasas had a problem deploying to frogbot July 2, 2024 16:35 — with GitHub Actions Failure

attiasas added 2 commits July 3, 2024 09:13

finish comp errors

77b3ab2

format

da0bfb9

attiasas had a problem deploying to frogbot July 3, 2024 06:14 — with GitHub Actions Failure

Merge remote-tracking branch 'upstream/dev' into refactor_output

c47d7fc

attiasas had a problem deploying to frogbot July 10, 2024 08:25 — with GitHub Actions Failure

format

218797a

attiasas had a problem deploying to frogbot July 10, 2024 09:08 — with GitHub Actions Failure

attiasas added the safe to test Approve running integration tests on a pull request label Jul 10, 2024

github-actions bot removed the safe to test Approve running integration tests on a pull request label Jul 10, 2024

continue refactor for tests

c9c2933

attiasas had a problem deploying to frogbot July 14, 2024 15:33 — with GitHub Actions Failure

github-actions bot removed the safe to test Approve running integration tests on a pull request label Sep 9, 2024

CR changes

ae39323

attiasas requested a deployment to frogbot September 10, 2024 06:21 — with GitHub Actions Waiting

start add docker tests

3c98706

attiasas requested a deployment to frogbot September 11, 2024 12:59 — with GitHub Actions Waiting

eranturgeman reviewed Sep 12, 2024

View reviewed changes

tests/testdata/output/dockerscan/docker_sarif.json Outdated

Copy link

Contributor

eranturgeman Sep 12, 2024

Choose a reason for hiding this comment

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

this file is empty. can we delete?

eranturgeman reviewed Sep 12, 2024

View reviewed changes

tests/testdata/output/dockerscan/docker_simple_json.json Outdated

Copy link

Contributor

eranturgeman Sep 12, 2024

Choose a reason for hiding this comment

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

this file is empty. can we delete?

eranturgeman reviewed Sep 12, 2024

View reviewed changes

fixing bugs adding docker tests

2840382

attiasas requested a deployment to frogbot September 15, 2024 06:27 — with GitHub Actions Waiting

Merge remote-tracking branch 'upstream/dev' into refactor_output

00e13c4

eranturgeman reviewed Sep 15, 2024

View reviewed changes

attiasas added 3 commits September 15, 2024 15:48

fix merge

6c3a32c

format

6815d23

fix static

650547f

attiasas requested a deployment to frogbot September 15, 2024 13:31 — with GitHub Actions Waiting

attiasas added 3 commits September 15, 2024 17:12

Merge remote-tracking branch 'upstream/dev' into refactor_output

bc48efa

merge

9e20652

fix conflict

6d95290

attiasas requested a deployment to frogbot September 15, 2024 14:19 — with GitHub Actions Waiting

attiasas added the safe to test Approve running integration tests on a pull request label Sep 15, 2024

github-actions bot removed the safe to test Approve running integration tests on a pull request label Sep 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor commands output formats #96

Refactor commands output formats #96

attiasas commented Jun 30, 2024 •

edited

Loading

github-actions bot commented Sep 9, 2024

eranturgeman Sep 12, 2024

eranturgeman Sep 12, 2024

eranturgeman Sep 12, 2024 •

edited

Loading

eranturgeman Sep 12, 2024

eranturgeman Sep 12, 2024

eranturgeman Sep 12, 2024

eranturgeman Sep 12, 2024

eranturgeman Sep 12, 2024

eranturgeman Sep 12, 2024

eranturgeman Sep 12, 2024

eranturgeman Sep 15, 2024

		@@ -187,11 +199,17 @@ func RecordSarifOutput(cmdResults *Results) (err error) {
		log.Info("Results can be uploaded to Github security tab automatically by upgrading your JFrog subscription.")

Refactor commands output formats #96

Are you sure you want to change the base?

Refactor commands output formats #96

Conversation

attiasas commented Jun 30, 2024 • edited Loading

Move all Results and outputs to their inner package, organize currently available code and renames to fit better.

Move detection out of SCA to run first thing in audit

Change Results object to ScanCommandResults

Separate converting ScanCommandResults to different output formats and printing the output.

Make implementing a new output format (by introducing an interface) easier, and maintain the currently supported.

General validation improvments for results structs

github-actions bot commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eranturgeman Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

attiasas commented Jun 30, 2024 •

edited

Loading

Move all `Results` and outputs to their inner package, organize currently available code and renames to fit better.

Change `Results` object to `ScanCommandResults`

Separate converting `ScanCommandResults` to different output formats and printing the output.

eranturgeman Sep 12, 2024 •

edited

Loading