Human readable output #1631

simonbaird · 2024-05-16T22:24:35Z

Cleaned up and ready for serious review now.

I think this could be merged as is, but it does need some extra work on the following:

Instead of top level grouping by component, it should be top level grouping by violation/warning/success.
There should be a way to disable/enable colors, ideally with auto detection of terminal color support (as @zregvart suggested in the review here).

Those could be in followup PRs, or could be included in this one.

Ref: EC-563.

codecov · 2024-05-16T22:32:00Z

Codecov Report

Attention: Patch coverage is 45.33333% with 41 lines in your changes are missing coverage. Please review.

Project coverage is 80.14%. Comparing base (d4a939b) to head (1f08226).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1631      +/-   ##
==========================================
- Coverage   80.69%   80.14%   -0.56%     
==========================================
  Files          64       65       +1     
  Lines        4704     4779      +75     
==========================================
+ Hits         3796     3830      +34     
- Misses        908      949      +41

Flag	Coverage Δ
generative	`80.14% <45.33%> (-0.56%)`	⬇️
integration	`80.14% <45.33%> (-0.56%)`	⬇️
unit	`80.14% <45.33%> (-0.56%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
cmd/validate/image.go	`93.58% <100.00%> (+0.14%)`	⬆️
internal/applicationsnapshot/report.go	`70.34% <0.00%> (-1.68%)`	⬇️
internal/utils/helpers.go	`67.56% <0.00%> (-14.40%)`	⬇️
internal/utils/templates.go	`52.94% <52.94%> (ø)`

zregvart · 2024-05-17T11:25:47Z

internal/applicationsnapshot/report.go

 	JSON            = "json"
 	YAML            = "yaml"
-	APPSTUDIO       = "appstudio"
+	Text            = "text"


As you're refactoring, we could define a type OutputFormatType string and have these typed like that...

Yeah I was thinking about that. Maybe in another commit since I merged #1640 already.

zregvart · 2024-05-17T11:26:28Z

internal/applicationsnapshot/report.go

@@ -289,6 +309,67 @@ func generateMarkdownSummary(r *Report) ([]byte, error) {
 	return markdownBuffer.Bytes(), nil
 }

+func generateTextReport(r *Report) ([]byte, error) {
+
+	mainTemplate := hd.Doc(`


Perhaps use embed and have the template in a separate file?

Yep, that's done in the latest revision.

zregvart · 2024-05-17T11:28:44Z

internal/applicationsnapshot/report.go

+		// For convenience since juggling the {{- and -}} gets tricky
+		"NL": "\n",


Yes, though much less so once you get into the rhythm of using only/mostly {{-, see the templates for the docs for inspiration

Actually I didn't end up using it 😅

zregvart · 2024-05-17T11:34:30Z

internal/utils/helpers.go

@@ -155,3 +155,19 @@ func HasSuffix(str string, extensions []string) bool {
 func HasJsonOrYamlExt(str string) bool {
 	return HasSuffix(str, []string{".json", ".yaml", ".yml"})
 }
+
+func ansiColorText(str string, color int) string {


We use https://pkg.go.dev/github.com/doiit/picocolors (that no longer seems to exist) in the acceptance tests. We might want to think about disabling colors based on the environment, picocolors does something like this:

func isColorSupported() bool { noColor := os.Getenv("NO_COLOR") != "" || contains(os.Args, "--no-color") forceColor := os.Getenv("FORCE_COLOR") != "" || os.Getenv("CLICOLOR_FORCE") != "" || contains(os.Args, "--color") terminal := isatty.IsTerminal(os.Stdout.Fd()) || isatty.IsCygwinTerminal(os.Stdout.Fd()) || os.Getenv("TERM") == "dumb" ci := os.Getenv("CI") != "" return !noColor && (forceColor || terminal || ci) }

There are terminfo packages we could also use to se if the terminal has color support (think CI)

Yeah good idea.

Third commit has this now.

zregvart · 2024-05-17T11:44:33Z

internal/applicationsnapshot/report.go

+	t = template.Must(t.Parse(mainTemplate))
+
+	for k, v := range partials {
+		t = template.Must(t.Parse(fmt.Sprintf("{{- define \"%s\" -}}\n%s\n{{- end -}}", k, hd.Doc(v))))


Could put the nested templates in that same file that gets embedded, or use https://pkg.go.dev/text/template#ParseFS with embed to keep them in separate files

Yeah, I discovered ParseFS, using that now with embed.

zregvart · 2024-05-17T11:45:17Z

cmd/validate/image.go

@@ -72,6 +72,9 @@ func validateImageCmd(validate imageValidationFunc) *cobra.Command {
 	}{
 		strict: true,
 	}
+
+	validOutputFormats := applicationsnapshot.OutputFormats


I'll be using this in an upcoming commit for text format output. (Creating in a separate commit for easier review.) Ref: https://issues.redhat.com/browse/EC-563

Use the new templating functionality from the previous commit. Currently it's split up by component, but that's likely to change in future. Ref: https://issues.redhat.com/browse/EC-563

zregvart · 2024-05-22T12:30:44Z

cmd/validate/image.go

+	cmd.Flags().BoolVar(&data.noColor, "no-color", data.info, hd.Doc(`
+		Disable color when using text output even when the current terminal supports it`))
+
+	cmd.Flags().BoolVar(&data.forceColor, "force-color", data.info, hd.Doc(`


Perhaps just --color?

Suggested change

cmd.Flags().BoolVar(&data.forceColor, "force-color", data.info, hd.Doc(`

cmd.Flags().BoolVar(&data.forceColor, "color", data.info, hd.Doc(`

Ref: https://issues.redhat.com/browse/EC-563

The new templating helpers were introduced in enterprise-contract#1631. I want to use the same technique in some older templating. This required a little refactor of the helpers because we want to be able to use io.Writer directly rather than write to a buffer. Also add some extra unit tests.

Probably should have been included in enterprise-contract#1631.

The new templating helpers were introduced in enterprise-contract#1631. I want to use the same technique in some older templating. This required a little refactor of the helpers because we want to be able to use io.Writer directly rather than write to a buffer. Also add some extra unit tests.

zregvart reviewed May 17, 2024

View reviewed changes

simonbaird force-pushed the plain-text-output branch 2 times, most recently from 5db7690 to b5349fc Compare May 20, 2024 13:20

simonbaird mentioned this pull request May 20, 2024

One fix and one tidy up for output options #1640

Merged

simonbaird added 2 commits May 21, 2024 14:31

Introduce some embedded template utils

b76dfa5

I'll be using this in an upcoming commit for text format output. (Creating in a separate commit for easier review.) Ref: https://issues.redhat.com/browse/EC-563

Introduce new human readable "text" outout format

a02cec8

Use the new templating functionality from the previous commit. Currently it's split up by component, but that's likely to change in future. Ref: https://issues.redhat.com/browse/EC-563

simonbaird force-pushed the plain-text-output branch from b5349fc to a02cec8 Compare May 21, 2024 18:59

simonbaird marked this pull request as ready for review May 21, 2024 19:22

simonbaird force-pushed the plain-text-output branch 2 times, most recently from 6fe516d to 7a0cc64 Compare May 21, 2024 21:49

zregvart approved these changes May 22, 2024

View reviewed changes

Detect if color should be enabled or disabled

1f08226

Ref: https://issues.redhat.com/browse/EC-563

simonbaird force-pushed the plain-text-output branch from 7a0cc64 to 1f08226 Compare May 22, 2024 15:37

simonbaird merged commit 5585270 into enterprise-contract:main May 23, 2024
9 of 10 checks passed

simonbaird mentioned this pull request May 23, 2024

Use new template utils for inspect policy also #1644

Merged

simonbaird added a commit to simonbaird/ec-cli that referenced this pull request May 23, 2024

Run go mod tidy and check in the results

cc21aaf

Probably should have been included in enterprise-contract#1631.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Human readable output #1631

Human readable output #1631

simonbaird commented May 16, 2024 •

edited

codecov bot commented May 16, 2024 •

edited

zregvart May 17, 2024

simonbaird May 21, 2024

zregvart May 17, 2024

simonbaird May 21, 2024

zregvart May 17, 2024

simonbaird May 21, 2024

zregvart May 17, 2024

simonbaird May 21, 2024

simonbaird May 22, 2024

zregvart May 17, 2024

simonbaird May 21, 2024

zregvart May 17, 2024

zregvart May 22, 2024

		// For convenience since juggling the {{- and -}} gets tricky
		"NL": "\n",

	cmd.Flags().BoolVar(&data.forceColor, "force-color", data.info, hd.Doc(`
	cmd.Flags().BoolVar(&data.forceColor, "color", data.info, hd.Doc(`

Human readable output #1631

Human readable output #1631

Conversation

simonbaird commented May 16, 2024 • edited

codecov bot commented May 16, 2024 • edited

Codecov Report

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

simonbaird commented May 16, 2024 •

edited

codecov bot commented May 16, 2024 •

edited