Skip to content

Commit

Permalink
Add markdownlint for linting docs (#493)
Browse files Browse the repository at this point in the history
Disable some overly pedantic rules, but good to help keep
our docs consistent.

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
anderseknert authored Nov 28, 2023
1 parent f073c74 commit 98afac8
Show file tree
Hide file tree
Showing 25 changed files with 117 additions and 53 deletions.
1 change: 1 addition & 0 deletions .github/workflows/build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ jobs:
- uses: open-policy-agent/setup-opa@v2
with:
version: edge
- run: npm install -g markdownlint-cli
- run: go install git.sr.ht/~charles/rq/cmd/rq@latest
- run: build/do.rq pull_request
- uses: golangci/golangci-lint-action@v3.7.0
Expand Down
16 changes: 9 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ Rego magnificent!
\- [Merriam Webster](https://www.merriam-webster.com/dictionary/regal)


## Goals

- Identify common mistakes, bugs and inefficiencies in Rego policies, and suggest better approaches
Expand Down Expand Up @@ -101,7 +100,7 @@ import future.keywords
default allow = false
deny if {
"admin" != input.user.roles[_]
"admin" != input.user.roles[_]
}
allow if not deny
Expand All @@ -112,6 +111,8 @@ Next, run `regal lint` pointed at one or more files or directories to have them
```shell
regal lint policy/
```
<!-- markdownlint-capture -->
<!-- markdownlint-disable MD010 -->
```text
Rule: not-equals-in-loop
Description: Use of != in loop
Expand All @@ -136,6 +137,7 @@ Documentation: https://docs.styra.com/regal/rules/style/use-assignment-operator
1 file linted. 3 violations found.
```
<!-- markdownlint-restore -->
<br />

> **Note**
Expand Down Expand Up @@ -217,7 +219,7 @@ The following rules are currently available:
| imports | [prefer-package-imports](https://docs.styra.com/regal/rules/imports/prefer-package-imports) | Prefer importing packages over rules |
| imports | [redundant-alias](https://docs.styra.com/regal/rules/imports/redundant-alias) | Redundant alias |
| imports | [redundant-data-import](https://docs.styra.com/regal/rules/imports/redundant-data-import) | Redundant import of data |
| style | [avoid-get-and-list-prefix](https://docs.styra.com/regal/rules/style/avoid-get-and-list-prefix) | Avoid get_ and list_ prefix for rules and functions |
| style | [avoid-get-and-list-prefix](https://docs.styra.com/regal/rules/style/avoid-get-and-list-prefix) | Avoid `get_` and `list_` prefix for rules and functions |
| style | [chained-rule-body](https://docs.styra.com/regal/rules/style/chained-rule-body) | Avoid chaining rule bodies |
| style | [default-over-else](https://docs.styra.com/regal/rules/style/default-over-else) | Prefer default assignment over fallback else |
| style | [detached-metadata](https://docs.styra.com/regal/rules/style/detached-metadata) | Detached metadata annotation |
Expand Down Expand Up @@ -247,7 +249,7 @@ The following rules are currently available:

By default, all rules except for those in the `custom` category are currently **enabled**.

#### Aggregate Rules
**Aggregate Rules**

Most Regal rules will use data only from a single file at a time, with no consideration for other files. A few rules
however require data from multiple files, and will therefore collect, or aggregate, data from all files provided for
Expand Down Expand Up @@ -453,8 +455,8 @@ The format of an ignore directive is `regal ignore:<rule-name>,<rule-name>...`,
rule to ignore. Multiple rules may be added to the same ignore directive, separated by commas.

Note that at this point in time, Regal only considers the same line or the line following the ignore directive, i.e. it
does not apply to entire blocks of code (like rules, functions or even packages). See [configuration](#configuration) if you want
to ignore certain rules altogether.
does not apply to entire blocks of code (like rules, functions or even packages). See [configuration](#configuration)
if you want to ignore certain rules altogether.

## Output Formats

Expand Down Expand Up @@ -482,7 +484,7 @@ but also for OPA [strict mode](https://www.openpolicyagent.org/docs/latest/polic
> of a future OPA 1.0 release, at which point they'd be made immediately obsolete as part of Regal. There are a few
> strict mode checks that likely will remain optional in OPA, and we may choose to integrate them into Regal in the
> future.

>
> Until then, the recommendation is to run both `opa check --strict` and `regal lint` as part of your policy build
> and test process.

Expand Down
3 changes: 3 additions & 0 deletions build/do.rq
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ job.check_readme {
build(true) {
run("go build")
}

build(false) {
not binary_present
run("go build")
Expand All @@ -143,11 +144,13 @@ e2e {
lint {
run("opa check --strict --capabilities build/capabilities.json bundle")
run("./regal lint --format pretty bundle")
run("markdownlint --config docs/.markdownlint.yaml README.md docs/")
}

lint_ci {
run("opa check --strict --capabilities build/capabilities.json bundle")
run_quiet("./regal lint --format github bundle")
run("markdownlint --config docs/.markdownlint.yaml README.md docs/")
}

check_readme {
Expand Down
2 changes: 1 addition & 1 deletion bundle/regal/rules/style/avoid_get_and_list_prefix.rego
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# METADATA
# description: Avoid get_ and list_ prefix for rules and functions
# description: Avoid `get_` and `list_` prefix for rules and functions
package regal.rules.style["avoid-get-and-list-prefix"]

import rego.v1
Expand Down
4 changes: 2 additions & 2 deletions bundle/regal/rules/style/avoid_get_and_list_prefix_test.rego
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ test_fail_rule_name_starts_with_get if {
r := rule.report with input as ast.policy(`get_foo := 1`)
r == {{
"category": "style",
"description": "Avoid get_ and list_ prefix for rules and functions",
"description": "Avoid `get_` and `list_` prefix for rules and functions",
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/avoid-get-and-list-prefix", "style"),
Expand All @@ -25,7 +25,7 @@ test_fail_function_name_starts_with_list if {
r := rule.report with input as ast.policy(`list_users(datasource) := ["we", "have", "no", "users"]`)
r == {{
"category": "style",
"description": "Avoid get_ and list_ prefix for rules and functions",
"description": "Avoid `get_` and `list_` prefix for rules and functions",
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/avoid-get-and-list-prefix", "style"),
Expand Down
39 changes: 39 additions & 0 deletions docs/.markdownlint.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
default: true

# MD013/line-length
# https://github.com/DavidAnson/markdownlint/blob/v0.32.1/doc/md013.md
MD013:
line_length: 120
heading_line_length: 80
code_blocks: false
tables: false
headings: true

# MD024/no-duplicate-heading
# https://github.com/DavidAnson/markdownlint/blob/v0.32.1/doc/md024.md
MD024:
siblings_only: true

# MD026/no-trailing-punctuation
# https://github.com/DavidAnson/markdownlint/blob/v0.32.1/doc/md026.md
MD026:
# Punctuation characters
punctuation: ".,;:,;:!"

# MD031/blanks-around-fences
# https://github.com/DavidAnson/markdownlint/blob/v0.32.1/doc/md031.md
MD031: false

# MD033/no-inline-html
# https://github.com/DavidAnson/markdownlint/blob/v0.32.1/doc/md033.md
MD033:
allowed_elements:
- br
- details
- img
- strong
- summary

# MD036/no-emphasis-as-heading
# https://github.com/DavidAnson/markdownlint/blob/v0.32.1/doc/md036.md
MD036: false
2 changes: 1 addition & 1 deletion docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ When running Regal against a directory, like `regal lint my-policies/`, Regal do
policy files and tests, the cost may be prohibitive. To alleviate this, Regal is implemented to process files
concurrently to minimize the impact of IO bound tasks, and to make use of multiple cores when available.
- The result of linting each file is eventually collected and compiled into a linter report, which is presented to the
user in one of the available [output formats](https://docs.styra.com/regal#output-formats).
user in one of the available [output formats](https://docs.styra.com/regal#output-formats).

## Rego Rules Evaluation

Expand Down
31 changes: 22 additions & 9 deletions docs/development.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Development

If you'd like to contribute to Regal, here are some pointers to help get you started.
If you'd like to contribute to Regal, here are some pointers to help get you started.

Before you start, the [architecture](./architecture) guide provides a useful overview of how Regal works, so you might
want to read that before diving into the code!
Expand All @@ -13,6 +13,7 @@ The following tools are required to build, test and lint Regal:
- The [golangci-lint](https://golangci-lint.run/usage/install/#local-installation) linter
- The [gci](https://github.com/daixiang0/gci) import formatter
- The [gofumpt](https://github.com/mvdan/gofumpt) formatter
- The [markdownlint](https://github.com/DavidAnson/markdownlint) linter

Recommended, but not required:

Expand All @@ -37,7 +38,7 @@ Regal.
### Guiding principles for new built-in rules

- All rules should have succinct, descriptive names which are unique - even across categories
- A rule that misses a few cases is better than no rule at all, but it's good to document any known edge cases
- A rule that misses a few cases is better than no rule at all, but it's good to document any known edge cases
- False positives should however always be avoided
- Add tests for as many cases as you can think of
- Any new rule should have an example violation added in `e2e/testada/violations/most_violations.rego`
Expand Down Expand Up @@ -110,6 +111,14 @@ gci write \
-s dot .
```

In order to ensure consistent formatting in our markdown docs, we use the
[markdownlint](https://github.com/DavidAnson/markdownlint) tool in CI. To run it yourself before submitting a PR,
install it (`brew install markdownlint-cli`) and run:

```shell
markdownlint --config docs/.markdownlint.yaml README.md docs/
```

## Preparing a pull request

Using `rq`, run all the required steps with:
Expand All @@ -133,16 +142,20 @@ go run main.go table --write-to-readme bundle

Build with

GOOS=wasip1 GOARCH=wasm go build -o regal.wasm .
```shell
GOOS=wasip1 GOARCH=wasm go build -o regal.wasm .
```

Run with wasmtime regal.wasm and the like:

$ curl https://wasmtime.dev/install.sh -sSf | bash
# ...
$ wasmtime --version
wasmtime-cli 13.0.0
$ wasmtime --dir $(pwd) regal -- lint bundle
90 files linted. No violations found.
```shell
$ curl https://wasmtime.dev/install.sh -sSf | bash
# ...
$ wasmtime --version
wasmtime-cli 13.0.0
$ wasmtime --dir $(pwd) regal -- lint bundle
90 files linted. No violations found.
```

## Community

Expand Down
3 changes: 2 additions & 1 deletion docs/editor-support.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

## Neovim via none-ls

[none-ls](https://github.com/nvimtools/none-ls.nvim) - Use Neovim as a language server to inject LSP diagnostics, code actions, and more via Lua.
[none-ls](https://github.com/nvimtools/none-ls.nvim) - Use Neovim as a language server to inject LSP diagnostics,
code actions, and more via Lua.

Minimal installation via [VimPlug](https://github.com/junegunn/vim-plug)

Expand Down
2 changes: 1 addition & 1 deletion docs/integration.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ regoText := `package foo...`

input, err := rules.InputFromText("policy.rego", regoText)
if err != nil {
// handle error
// handle error
}
```

Expand Down
8 changes: 4 additions & 4 deletions docs/pre-commit-hooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ To use Regal with pre-commit, add this to your `.pre-commit-config.yaml`
# - id: ...
```

### Hooks Available
## Hooks Available

#### `regal-lint`
### `regal-lint`

![commit-msg hook](https://img.shields.io/badge/hook-pre--commit-informational?logo=git)

Expand All @@ -25,15 +25,15 @@ Runs Regal against all staged `.rego` files, aborting the commit if any fail.
- will build and install the tagged version of Regal in an isolated `GOPATH`
- ensures compatibility between versions

#### `regal-lint-use-path`
### `regal-lint-use-path`

![commit-msg hook](https://img.shields.io/badge/hook-pre--commit-informational?logo=git)

Runs Regal against all staged `.rego` files, aborting the commit if any fail.

- requires the `regal` package is already installed and available on `$PATH`.

#### `regal-download`
### `regal-download`

![commit-msg hook](https://img.shields.io/badge/hook-pre--commit-informational?logo=git)

Expand Down
7 changes: 6 additions & 1 deletion docs/rego-style-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ for Regal to check, and should be targeted by other means.
- [ ] Consider using JSON schemas for type checking

### Notes

Both `opa fmt` and `strict mode` checks can be implemented by tapping into
the corresponding features of OPA, and simply report errors as linter violations.
Why not just use OPA for that? Mainly because we want a single command (and config)
Expand All @@ -37,10 +38,11 @@ certainly have to be configurable to be usable. Same goes for JSON schemas.
- [ ] Use helper rules and functions
- [ ] Use negation to handle undefined
- [x] ~~Consider partial helper rules over comprehensions in rule bodies~~
- [x] Avoid prefixing rules and functions with get_ or list_
- [x] Avoid prefixing rules and functions with `get_` or `list_`
- [x] Prefer unconditional assignment in rule head over rule body

### Notes

Three quite complex rules in the top here. While it's not going to be very
scientific, we could try to determine whether helper rules are used to a
satisfying degree by checking the dependencies of rules vs the number of
Expand All @@ -59,6 +61,7 @@ determine whether the author has done, and both have valid use cases.
- [x] ~~Prefer sets over arrays (where applicable)~~

### Notes

Almost all of these should be doable, with some possibly being quite challenging.
One that could be very hard to implement is the `every` rule, as that would
require us to determine what **other** method was used and that `every` is a
Expand All @@ -75,6 +78,7 @@ none of the above rules can be enforced using the AST alone.
- [x] Use raw strings for regex patterns

### Notes

Can only be done by scanning the original code, as this is lost in the AST.

## Imports
Expand All @@ -84,6 +88,7 @@ Can only be done by scanning the original code, as this is lost in the AST.
- [x] Avoid importing input

### Notes

Checking for package imports requires a view of all modules. We may assume that
anything not found there are base documents to be provided at runtime.

Expand Down
4 changes: 2 additions & 2 deletions docs/rules/bugs/not-equals-in-loop.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ deny if {
```

The body of the `deny` rule above roughly translates to "for any item in `input.user.roles`, return true if the item is
not `admin`". This is almost never what the policy author intended. What the policy author likely intended was
not `admin`". This is almost never what the policy author intended. What the policy author likely intended was
"deny if `admin` is not in `input.user.roles`". The above policy would thus **not** deny a user with the roles
`["user", "admin"]` since the first item in the array is not "admin". This is almost never what the policy author
intended.
Expand All @@ -61,7 +61,7 @@ currently only checks for wildcard iteration (`[_]`).
This linter rule provides the following configuration options:

```yaml
rules:
rules:
bugs:
not-equals-in-loop:
# one of "error", "warning", "ignore"
Expand Down
2 changes: 1 addition & 1 deletion docs/rules/custom/prefer-value-in-head.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ deny contains "user attribute missing from input" if not input.user

Rules that return the value assigned in the last expression of the rule body may have the value, or the function
returning the value, moved directly to the rule head. This creates more succinct rules, and often allows for rules to be
expressed as "one-liners". This is not a general recommendation, but a style preference that a team or organization
expressed as "one-liners". This is not a general recommendation, but a style preference that a team or organization
might want to standardize on. As such, it is placed in the custom category, and must be explicitly enabled in
configuration.

Expand Down
Loading

0 comments on commit 98afac8

Please sign in to comment.