feat: add --aggregate flag to regal new rule (#341)#1966
Conversation
cd3341e to
9918642
Compare
|
Thanks! Busy day with the v0.40.0 release going out, but I'll review some time this week 👍 |
|
Thanks @anderseknert - no rush, happy to wait until after the v0.40.0 release settles. |
anderseknert
left a comment
There was a problem hiding this comment.
Looks good to me, with only a minor remark! If you could take a look at that and let me know what works, this is good to merge 🙂
| test_aggregate_reports_violation if { | ||
| agg := rule.aggregate with input as ast.policy("foo := true") | ||
|
|
||
| r := rule.aggregate_report with input.aggregates_internal as util.with_source_files( |
There was a problem hiding this comment.
I think you were right first and Codex set you off in the wrong direction here :) Anything with "internal" in the name should be avoided for code that'll live outside of the project, as those APIs change frequently. In fact, just this one changed just a few releases back!
If there's something you aren't able to do without using this, let me know as we should fix that right away. But even though the non-internal option is much less efficient at this point, let's go with that until we have a better alternative that we can consider stable. OK?
|
Reverted in 415663c — back to One thing worth flagging: the reason I'd changed it in the first place is that scaffolded aggregate rules generated from this template produced tests that passed but never actually emitted violations during normal linting, because runtime evaluation reads from |
Aggregate rules need a different Rego shape (`aggregate` +
`aggregate_report` instead of a single `report`), and scaffolding
that by hand is friction. Adding `--aggregate` to `regal new rule`
pairs the existing per-type templates with an aggregate variant for
both custom and builtin types, and updates the test template to
exercise the aggregate_report path.
Without the flag, behavior is unchanged.
Dogfood:
$ regal new rule -t custom -c naming -n my_rule --aggregate -o $TMPDIR
produces a `.rego` with `aggregate contains entry if { ... }` +
`aggregate_report contains violation if { ... }` and a test file
that feeds a parsed module through `aggregate` and asserts the
`aggregate_report` emits a violation.
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Addresses codex-review feedback: bundled aggregate_report rules are
evaluated with entries under input.aggregates_internal["cat/name"],
not the custom-rule compatibility shape input.aggregate. The prior
builtin template would produce a rule whose scaffolded test passed
(because it injected input.aggregate directly) but which never emitted
violations during normal linting.
Switch the builtin template to read from input.aggregates_internal
keyed by "{{.Category}}/{{.NameOriginal}}" and update the generated
test to exercise the same shape via util.with_source_files.
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
This reverts commit 9918642.
415663c to
c5e5fd8
Compare
|
@mvanhorn that should not be the case, as the internal format is converted here regal/bundle/regal/main/main.rego Lines 227 to 244 in 0d095a6 As noted in the metadata comment it is a legacy approach, but there are tests that verify that it works... not to mention of course a few known deployments where custom rules are used :) If you're not getting that to work, let's look into it. |
|
You're right, sorry for the wrong-direction speculation. Built this branch and ran The conversion at |
|
@mvanhorn I had forgotten about this. The CI failure looks like a flake, so I'm sure it'll be gone if you rebase this. Before you to, can you make sure the DCO check passes? Likely that same issue with diverging e-mail addresses in commuter vs signoff we saw before. |
Summary
Adds
--aggregatetoregal new ruleso aggregate rules are as easy to scaffold as single-module ones. Both--type customand--type builtinare supported.Closes #341
Why this matters
Aggregate rules need a different Rego shape (an
aggregate contains entry if { ... }collector plus anaggregate_report contains violation if { ... }reducer) and, for builtin rules, a different input shape at runtime (input.aggregates_internal["<category>/<name>"]rather thaninput.aggregate). Authors currently have to hand-port all of that from an existing aggregate rule likebundle/regal/rules/imports/unresolved-import/unresolved_import.rego.Changes
cmd/new.go:newRuleCommandParamsgains anaggregate bool. New--aggregateflag.renderTemplatespickstemplates/{type}/{type}_aggregate{,_test}.rego.tplwhen the flag is set, otherwise the original{type}.rego.tpl/{type}_test.rego.tplpair (no behavior change without the flag).internal/embeds/templates/custom/custom_aggregate.rego.tpl+custom_aggregate_test.rego.tpl: scaffold a custom aggregate rule that readsinput.aggregate(the custom-rule shape).internal/embeds/templates/builtin/builtin_aggregate.rego.tpl+builtin_aggregate_test.rego.tpl: scaffold a builtin aggregate rule that readsinput.aggregates_internal["<cat>/<name>"](the bundled runtime shape) and drives its test viautil.with_source_files.createBuiltinDocsandaddToDataYAMLare unchanged — aggregate/non-aggregate doesn't affect those paths.Dogfood
Custom:
Builtin:
Without
--aggregate, both subcommands produce exactly the same output as before.Testing
go build ./...cleango vet ./cmd/...cleango test ./cmd/...(no cmd tests exist today)--type custom --aggregateand--type builtin --aggregateNotes
Two commits to preserve review history:
input.aggregatetoinput.aggregates_internal["category/name"]so bundled aggregate rules actually report during normal linting, not just in the scaffolded test.This contribution was developed with AI assistance (Claude Code + Codex).