Regression: regal test can't test itself

[anderseknert]

Running regal test bundle (or go run main.go test bundle) now seems to lead to the same files getting loaded twice. I'm guessing that since #197 we now include the bundled files in the test command, and we load them from the filesystem, this dupication is what's triggering the condition.

11 errors occurred:
/regal/regal.rego:1: rego_type_error: subpackages annotation redeclared: bundle/regal/regal.rego:1
/regal/rules/bugs/constant_condition.rego:1: rego_type_error: package annotation redeclared: bundle/regal/rules/bugs/constant_condition.rego:1
/regal/rules/bugs/invalid_metadata_attribute.rego:1: rego_type_error: package annotation redeclared: bundle/regal/rules/bugs/invalid_metadata_attribute.rego:1
/regal/rules/bugs/not_equals_in_loop.rego:1: rego_type_error: package annotation redeclared: bundle/regal/rules/bugs/not_equals_in_loop.rego:1
/regal/rules/bugs/rule_named_if.rego:1: rego_type_error: package annotation redeclared: bundle/regal/rules/bugs/rule_named_if.rego:1
/regal/rules/bugs/rule_shadows_builtin.rego:1: rego_type_error: package annotation redeclared: bundle/regal/rules/bugs/rule_shadows_builtin.rego:1
/regal/rules/bugs/top_level_iteration.rego:1: rego_type_error: package annotation redeclared: bundle/regal/rules/bugs/top_level_iteration.rego:1
/regal/rules/bugs/unused_return_value.rego:1: rego_type_error: package annotation redeclared: bundle/regal/rules/bugs/unused_return_value.rego:1
/regal/rules/idiomatic/custom_has_key_construct.rego:1: rego_type_error: package annotation redeclared: bundle/regal/rules/idiomatic/custom_has_key_construct.rego:1
/regal/rules/idiomatic/custom_in_construct.rego:1: rego_type_error: package annotation redeclared: bundle/regal/rules/idiomatic/custom_in_construct.rego:1
rego_compile_error: error limit reached
exit status 1

Not sure what the best approach would be for fixing it, while still having the command work for custom rules.

[srenatus]

There's a module loader mechanism in the ast/compile.go. Perhaps that could be used to bring in regal's own rules...? Not sure at a glance if it'll help 🤞

[anderseknert]

That's interesting, thanks! I took a look but it's not clear to me how that function works, despite having looked at the tests that make use of it 😅 Seems like it'll just keep getting called forever until the modules map is empty or an error is returned. If I run regal test bundle I do see that the "input map" contains both the modules from the Regal bundle as well as the duplicates from the bundle directory. But what am I supposed to do here? Remove the duplicates, and next invocation an empty map, is that it?

[srenatus]

That test is weird, granted. The module loader allows you to load extra modules out of band, like, when they are needed, because a certain import directive is present. There test asserts that it works like that until it reaches a fix point: no further extra modules are needed.

[kristiansvalland]

As I understand the code, the ModuleLoader should(?) not edit the received map (i.e. not remove duplicates), but return the lazy loaded modules after inspecting the input.

Then, the compiler will add them if the length is non-zero: https://github.com/open-policy-agent/opa/blob/main/ast/compile.go#L1631

So perhaps the solution would be to instead of unconditionally loading the bundle into the compiler as I did, add a ModuleLoader that checks if they are already present in the input map and if not, return them, as @srenatus suggested.

[anderseknert]

I think that's what I tried doing, but evidently unsuccessfully. If you'd like to have a shot at it, feel free to do so! 😃

[kristiansvalland]

I am also on vacation at the moment, so my capacity is a bit limited 🌴 But if the weather takes a turn for the worse, I might be able to take a look. No promises though.

[anderseknert]

Sounds good! This isn't annoying anyone but me, most likely 😆 And I can work around it. Enjoy your vacation! 🏖️

[srenatus]

I've been thinking about this some more, I think the ModuleLoader approach won't help us here. Sorry for muddying the waters. I'll have a look at the code, too 😃

[anderseknert]

This was fixed in #286

Thanks again, Stephan!