Add merge reports option in report command #10

AleksandarSavchev · 2023-09-08T14:48:25Z

What this PR does / why we need it:
This PR add a new functionality to the diki report command which allows the merge of multiple json reports into a single html report. To use this feature the distinct-by flag should be set with unique attributes which would be used for each provider to distinct the different reports.

TODO: write documentation

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

The `diki report` command can now be used to merge multiple reports into a single report by setting the `--distinct-by` flag.

dimityrmirchev

Thanks! I left some comments. Also it would be nice if we can have a simple test for the MergeReport function

dimityrmirchev · 2023-09-12T12:49:28Z

cmd/diki/app/app.go

-	if len(args) != 1 {
-		return errors.New("report requires a single filepath argument")
+	if len(args) == 0 {
+		return errors.New("report requires a minimum of one filepath arguments")


Suggested change

return errors.New("report requires a minimum of one filepath arguments")

return errors.New("report command requires a minimum of one filepath argument")

dimityrmirchev · 2023-09-12T12:50:54Z

cmd/diki/app/app.go

+	}
+
+	if len(args) > 1 && len(opts.distinctBy) == 0 {
+		return errors.New("report requires a single filepath argument when flag distinct-by is not set ")


Suggested change

return errors.New("report requires a single filepath argument when flag distinct-by is not set ")

return errors.New("report command requires a single filepath argument when the distinct-by flag is not set ")

dimityrmirchev · 2023-09-12T12:55:22Z

README.md

@@ -38,11 +38,18 @@ diki run --config=config.yaml --provider=gardener --ruleset-id=disa-kubernetes-s

 #### Report

-Generate an html report
+Diki report generates a human readable html report from the .json output of a `diki run` execution. Merged reports can be generated using the `distinct-by` flag, the value of this flag is a list of key:value pairs where the keys are the IDs of the providers we want to include in the merged report and the values are the unique metadata fields to be used for IDs of the different reports.


Suggested change

Diki report generates a human readable html report from the .json output of a `diki run` execution. Merged reports can be generated using the `distinct-by` flag, the value of this flag is a list of key:value pairs where the keys are the IDs of the providers we want to include in the merged report and the values are the unique metadata fields to be used for IDs of the different reports.

Diki can generate a human readable report from the output files of a `diki run` execution. Merged reports can be produced by setting the `distinct-by` flag. The value of this flag is a list of `key=value` pairs where the keys are the IDs of the providers we want to include in the merged report and the values are the unique metadata fields to be used as distinction values between different provider runs.

dimityrmirchev · 2023-09-12T12:56:08Z

cmd/diki/app/app.go

+	for _, arg := range args {
+		fileData, err := os.ReadFile(filepath.Clean(arg))
+		if err != nil {
+			return fmt.Errorf("failed to read file %s:%w", arg, err)


Suggested change

return fmt.Errorf("failed to read file %s:%w", arg, err)

return fmt.Errorf("failed to read file %s: %w", arg, err)

dimityrmirchev · 2023-09-12T13:00:26Z

pkg/report/render.go

-		return r.templates["report"].Execute(w, rep)
+		return r.templates[tmplReportName].Execute(w, rep)
+	case *MergedReport:
+		return r.templates[tmplMergedReportName].Execute(w, rep)
 	default:
 		return fmt.Errorf("unsupported report type: %T", rep)


Suggested change

return fmt.Errorf("unsupported report type: %T", rep)

return fmt.Errorf("unsupported report type: %T", report)

dimityrmirchev · 2023-09-12T13:39:01Z

pkg/report/merged_report.go

+			}
+
+			if _, ok := mergedProvider.Metadata[uniqueAttr]; ok {
+				return &MergedReport{}, fmt.Errorf("distinct attribute %s is not unique", mergedProvider.DistinctBy)


Suggested change

return &MergedReport{}, fmt.Errorf("distinct attribute %s is not unique", mergedProvider.DistinctBy)

return nil, fmt.Errorf("distinct attribute %s is not unique", mergedProvider.DistinctBy)

dimityrmirchev · 2023-09-12T13:40:02Z

pkg/report/merged_report.go

+			}
+
+			mergedProvider.Metadata[uniqueAttr] = report.Providers[idx].Metadata
+			mergedProvider.Metadata[uniqueAttr]["time"] = report.Time.Format("01-02-2006")


Should we also add the time of the report?

That would be a good addition for a more detailed report. I have added HH-MM-SS now as well.

dimityrmirchev · 2023-09-12T13:43:45Z

pkg/report/merged_report.go

+func numOfMergedRulesWithStatus(ruleset *MergedRuleset, status rule.Status) int {
+	num := 0
+	for _, rule := range ruleset.Rules {
+		for _, check := range rule.Checks {


Maybe we can use slices.ContainsFunc here?

dimityrmirchev · 2023-09-12T13:44:55Z

pkg/report/merged_report.go

+}
+
+func metadataTextForMergedProvider(mp MergedProvider) map[string]string {
+	metadataTexts := map[string]string{}


Suggested change

metadataTexts := map[string]string{}

metadataTexts := make(map[string]string, len(mp.Metadata))

dimityrmirchev · 2023-09-12T13:46:13Z

pkg/report/merged_report.go

+func metadataTextForMergedProvider(mp MergedProvider) map[string]string {
+	metadataTexts := map[string]string{}
+	for id, metadata := range mp.Metadata {
+		metadataText := "("


I think this is a good place to use strings.Builder

dimityrmirchev · 2023-09-14T07:09:30Z

pkg/report/merged_report.go

+	for key := range distinctByAttrs {
+		distinctByAttrsProviders = append(distinctByAttrsProviders, key)
+	}
+	sort.Strings(distinctByAttrsProviders)


please use slices.Sort

dimityrmirchev · 2023-09-14T07:29:16Z

pkg/report/report_suite_test.go

+	handler := slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelInfo})
+	logger := slog.New(handler)
+	testLogger = logger


This is not used

dimityrmirchev

Thanks!
/lgtm

Add merge reports option in report command

83bd800

gardener-robot added needs/review Needs review size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Sep 8, 2023

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 8, 2023

gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 8, 2023

AleksandarSavchev added 2 commits September 11, 2023 14:51

Improve usage string

69b7ee1

Improve Report section in README.md

498e790

gardener-robot added size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) and removed size/l Size of pull request is large (see gardener-robot robot/bots/size.py) labels Sep 11, 2023

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 11, 2023

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 11, 2023

add new line

cdf68e1

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 11, 2023

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 11, 2023

AleksandarSavchev marked this pull request as ready for review September 11, 2023 12:08

AleksandarSavchev requested a review from a team as a code owner September 11, 2023 12:08

dimityrmirchev requested changes Sep 12, 2023

View reviewed changes

gardener-robot added the needs/changes Needs (more) changes label Sep 12, 2023

AleksandarSavchev added 8 commits September 13, 2023 15:57

Improve diki report documentation

258a088

Imrprove error messages

3b1256f

Sort providers in merged report

94701ad

Imrpove time string in merged report for runs

4dae979

Use strings.Builder

c68f480

Small imrpovements

8a6c642

Imrpove method comments for merged structs

2c53b66

Add unit tests for MergeReport

eb235b8

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 13, 2023

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 13, 2023

dimityrmirchev reviewed Sep 14, 2023

View reviewed changes

AleksandarSavchev added 4 commits September 14, 2023 10:41

Improve strings Builder usage

3c1e32f

Remove unused logger

a4f5365

Use slices.Sort

a5c6b62

Small gocritic improvement

14ffbae

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 14, 2023

dimityrmirchev approved these changes Sep 14, 2023

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else labels Sep 14, 2023

dimityrmirchev merged commit 9a49947 into gardener:main Sep 14, 2023

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Sep 14, 2023

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 14, 2023

AleksandarSavchev deleted the add-merged-reports branch February 7, 2024 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add merge reports option in report command #10

Add merge reports option in report command #10

AleksandarSavchev commented Sep 8, 2023 •

edited by dimityrmirchev

Loading

dimityrmirchev left a comment

dimityrmirchev Sep 12, 2023

dimityrmirchev Sep 12, 2023

dimityrmirchev Sep 12, 2023

dimityrmirchev Sep 12, 2023

dimityrmirchev Sep 12, 2023

dimityrmirchev Sep 12, 2023

dimityrmirchev Sep 12, 2023

AleksandarSavchev Sep 13, 2023

dimityrmirchev Sep 12, 2023

dimityrmirchev Sep 12, 2023

dimityrmirchev Sep 12, 2023

dimityrmirchev Sep 14, 2023

dimityrmirchev Sep 14, 2023

dimityrmirchev left a comment

	return errors.New("report requires a minimum of one filepath arguments")
	return errors.New("report command requires a minimum of one filepath argument")

	return errors.New("report requires a single filepath argument when flag distinct-by is not set ")
	return errors.New("report command requires a single filepath argument when the distinct-by flag is not set ")

	return fmt.Errorf("failed to read file %s:%w", arg, err)
	return fmt.Errorf("failed to read file %s: %w", arg, err)

	return fmt.Errorf("unsupported report type: %T", rep)
	return fmt.Errorf("unsupported report type: %T", report)

	return &MergedReport{}, fmt.Errorf("distinct attribute %s is not unique", mergedProvider.DistinctBy)
	return nil, fmt.Errorf("distinct attribute %s is not unique", mergedProvider.DistinctBy)

	metadataTexts := map[string]string{}
	metadataTexts := make(map[string]string, len(mp.Metadata))

Add merge reports option in report command #10

Add merge reports option in report command #10

Conversation

AleksandarSavchev commented Sep 8, 2023 • edited by dimityrmirchev Loading

dimityrmirchev left a 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

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

dimityrmirchev left a comment

Choose a reason for hiding this comment

AleksandarSavchev commented Sep 8, 2023 •

edited by dimityrmirchev

Loading