Add README and documentation #5

viswajithiii · 2020-09-17T16:46:15Z

Add README and documentation.
Write commands to print out the list of checks and templates, both plain and in markdown format. The markdown docs are checked in to the repo for better documentation.

misberner · 2020-09-18T10:10:00Z

README.md

+with `.yaml` or `.yml` extensions are parsed, and Kubernetes objects are loaded from them. If a file is passed,
+it is parsed irrespective of extension.
+
+Users can pass a config file using the `--config` file to control which checks are executed, and to configure custom checks.


--config flag

misberner · 2020-09-18T10:12:13Z

docs/README.md

+with `.yaml` or `.yml` extensions are parsed, and Kubernetes objects are loaded from them. If a file is passed,
+it is parsed irrespective of extension.
+
+Users can pass a config file using the `--config` file to control which checks are executed, and to configure custom checks.


flag again (is there some way to not duplicate the contents between this and the top-level readme)?

Fixed. And I just removed the duplicate content from the top-level README in favour of just linking.

misberner · 2020-09-18T10:13:22Z

README.md

+If you have `go` installed, you can run `go get golang.stackrox.io/kube-linter/cmd/kube-linter`.
+
+`kube-linter` binaries can be downloaded from [the Releases page](https://github.com/stackrox/kube-linter/releases).
+Download the `kube-linter` binary, and add it to your PATH.


IMHO a readme in an open source repository should always include custom build instructions and contributing guidelines (or references to these)

Added instructions for building from source. Contributing guidelines, will do in a follow-up -- we still need to iron those out.

misberner · 2020-09-18T10:17:07Z

internal/command/checks/command.go

+	}()
+
+	formatsToRenderFuncs = map[string]func([]check.Check, io.Writer){
+		common.PlainFormat: func(checks []check.Check, out io.Writer) {


Move these to separate functions instead of inlining them. I would also encourage you to use Go templates for formatting. Especially the markdown formatting (a) looks ugly in code and (b) is extremely limited as you cannot add any custom styling, notes, etc.
(You might need to add either a custom function or translate the checks into a separate type to realize the "enabled by default" flag)

Done. Also moved to templates.

misberner · 2020-09-18T10:19:08Z

internal/command/checks/command.go

+		return sb.String()
+	}()
+
+	formatsToRenderFuncs = map[string]func([]check.Check, io.Writer){


Possibly irrelevant (see below), but the brace following the closing parens without space in between looks weird - doesn't the linter flag this?

That brace is actually consistent with go fmt since it applies to the map, and not the func (although I agree it pattern matches with braces around a func and looks weird).

misberner · 2020-09-18T10:23:49Z

internal/command/common/format_wrapper.go

+)
+var (
+	// AllSupportedFormats contains the list of all supported formats.
+	AllSupportedFormats = set.NewStringSet()


Does Go have some form of dependency resolution when calling global variable initializers? To me it looks like there is nothing ensuring that this initialization has taken place at them time when newFormat("plain") above calls AllSupportedFormats.Add.

It also seems weird that we store a set of supported formats. That should be a derived property, derived from a map mapping the IDs to something semantic like a render function. In short - why not derive this from formatToRenderFunc?

Does Go have some form of dependency resolution when calling global variable initializers? To me it looks like there is nothing ensuring that this initialization has taken place at them time when newFormat("plain") above calls AllSupportedFormats.Add.

Yup, Go handles this really well. See https://golang.org/ref/spec#Program_initialization_and_execution.

In short - why not derive this from formatToRenderFunc?

The problem is that it's shared between the checks and templates command, which have their own formatToRenderFuncs. I've written unit tests to ensure the keys of that map and this are in sync.

misberner · 2020-09-18T10:24:37Z

internal/command/templates/command.go

+		return sb.String()
+	}()
+
+	formatsToRenderFuncs = map[string]func([]check.Template, io.Writer){


misberner · 2020-09-18T20:18:30Z

internal/command/checks/command.go

+	}
+)
+
+func renderPlain(checks []check.Check, out io.Writer) error { //nolint:unparam // The function signature is required to match formatToRenderFuncs


Why don't you use a Go template for this?

This function is pretty simple, so I think it's better to keep it as is. I find normal code easier to read (plus the compile-time safety is nice).

misberner · 2020-09-18T20:20:57Z

internal/command/checks/command.go

+}
+
+const (
+	markDownTemplateStr = `The following table enumerates built-in checks:


Ok so what I actually had in mind was something like --format go-template="{{ template str }}" or --format go-template=@file. It is fairly established in the Go world to allow users to specify Go templates for all "print"-like commands (e.g., go list, kubectl get.. etc) (although the file variant is something non-standard, but useful here).
This works for the initial iteration but having to resort to {{backtick}} is.. ugly..

Ahh got it. Yeah, I planned to support Go template output for the lint command but wasn't sure if it was worth it for these commands... Will do in a follow-up.

misberner · 2020-09-18T20:23:49Z

internal/command/common/markdown.go

+			"backtick": func() string {
+				return "`"
+			},
+			"joinstrings": strings.Join,


tbh I'd prefer to use a standard Go template library like sprig instead, especially if you want to eventually allow users to provide their own templates

misberner · 2020-09-18T20:24:08Z

internal/command/templates/command.go

+)
+
+var (
+	dashes = func() string {


Maybe worth adding a stringutils.Repeat?

misberner · 2020-09-18T20:35:48Z

internal/command/templates/command.go

+	markDownTemplate = common.MustInstantiateTemplate(markDownTemplateStr)
+)
+
+func renderPlain(templates []check.Template, out io.Writer) error { //nolint:unparam // The function signature is required to match formatToRenderFuncs


which parameter is unused? oO

error is alwasys nil.

connorgorman · 2020-09-19T00:12:21Z

config.yaml.example

+    key: "app"
+checks:
+  # if doNotAutoAddDefaults is true, default checks are not automatically added.
+  # Otherwise, they are.


Remove, this is pretty obvious based on the previous line

Also, I think doNotAutoAddDefaults is backwards, should be autoAddDefaults that defaults to true

it's also just a bit too verbose

connorgorman · 2020-09-19T00:17:48Z

internal/ternary/ternary.go

@@ -0,0 +1,9 @@
+package ternary


why is this in the PR? Also, adding a ternary is a bit of a strange thing in golang as it feels like it should be a language level thing

Yeah I was using it initially but not anymore. Removed.

connorgorman

Minor stuff. Looks good

Comments addressed

viswajithiii added 11 commits September 17, 2020 09:45

Add README

415c989

WIP

cf25700

Add warning

6671609

Fix

8e0343d

Add generated docs for templates

4471ade

WIP

4da4489

WIP

12b1935

Style

9a2b1cc

WIP

e29a268

Try

34135fd

More documentation

b069f0b

viswajithiii changed the title ~~Add README~~ Add README and documentation Sep 18, 2020

viswajithiii added 2 commits September 17, 2020 21:22

WIP

4dbaf53

Fixes

869c277

viswajithiii requested review from misberner, connorgorman and mtesseract September 18, 2020 04:26

viswajithiii marked this pull request as ready for review September 18, 2020 04:30

viswajithiii added 2 commits September 17, 2020 21:32

Move dir?

0cc252b

Fix

5e71a2d

misberner previously requested changes Sep 18, 2020

View reviewed changes

PR comments

622e55e

viswajithiii requested a review from misberner September 18, 2020 19:05

Sort check names for deterministic output

bccc555

misberner reviewed Sep 18, 2020

View reviewed changes

PR comments

2928cfe

viswajithiii requested a review from misberner September 18, 2020 20:52

connorgorman reviewed Sep 19, 2020

View reviewed changes

connorgorman approved these changes Sep 19, 2020

View reviewed changes

PR comments

80ef6cb

viswajithiii merged commit 51143ce into main Sep 19, 2020

viswajithiii deleted the viswa/readme branch September 19, 2020 01:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add README and documentation #5

Add README and documentation #5

viswajithiii commented Sep 17, 2020 •

edited

misberner Sep 18, 2020

misberner Sep 18, 2020

viswajithiii Sep 18, 2020

misberner Sep 18, 2020

connorgorman Sep 18, 2020

viswajithiii Sep 18, 2020

misberner Sep 18, 2020

viswajithiii Sep 18, 2020

misberner Sep 18, 2020

viswajithiii Sep 18, 2020

misberner Sep 18, 2020

viswajithiii Sep 18, 2020

misberner Sep 18, 2020

misberner Sep 18, 2020

viswajithiii Sep 18, 2020

misberner Sep 18, 2020

viswajithiii Sep 18, 2020

misberner Sep 18, 2020

viswajithiii Sep 18, 2020

misberner Sep 18, 2020

viswajithiii Sep 18, 2020

misberner Sep 18, 2020

viswajithiii Sep 18, 2020

connorgorman Sep 19, 2020

connorgorman Sep 19, 2020

connorgorman Sep 19, 2020

connorgorman Sep 19, 2020

viswajithiii Sep 19, 2020

connorgorman left a comment

Add README and documentation #5

Add README and documentation #5

Conversation

viswajithiii commented Sep 17, 2020 • edited

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

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

connorgorman left a comment

Choose a reason for hiding this comment

viswajithiii commented Sep 17, 2020 •

edited