Add merge reports option in report command by AleksandarSavchev · Pull Request #10 · gardener/diki

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

-	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

+	}
+
+	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

 #### 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

+	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

+	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

+			}
+
+			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

+			}
+
+			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

+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

+}
+
+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

+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

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


please use slices.Sort

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

+	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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 8, 2023

gardener-robot-ci-1 added the reviewed/ok-to-test label Sep 8, 2023

gardener-robot-ci-2 added needs/ok-to-test and removed reviewed/ok-to-test 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 Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2023

gardener-robot-ci-1 added the reviewed/ok-to-test label Sep 11, 2023

ghost removed the reviewed/ok-to-test label Sep 11, 2023

add new line

cdf68e1

gardener-robot-ci-1 added the reviewed/ok-to-test label Sep 11, 2023

ghost removed the reviewed/ok-to-test 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 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 label Sep 13, 2023

ghost removed the reviewed/ok-to-test 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 and removed reviewed/ok-to-test labels Sep 14, 2023

dimityrmirchev approved these changes Sep 14, 2023

View reviewed changes

gardener-robot added reviewed/lgtm and removed needs/changes labels Sep 14, 2023

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

gardener-robot added the status/closed label Sep 14, 2023

gardener-robot-ci-1 added the reviewed/ok-to-test label Sep 14, 2023

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

	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))

Conversation

AleksandarSavchev commented Sep 8, 2023 • edited by dimityrmirchev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimityrmirchev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimityrmirchev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AleksandarSavchev commented Sep 8, 2023 •

edited by dimityrmirchev

Loading