Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore directives inconsistency #803

Closed
nejec opened this issue Jun 6, 2024 · 1 comment · Fixed by #809
Closed

ignore directives inconsistency #803

nejec opened this issue Jun 6, 2024 · 1 comment · Fixed by #809
Labels
bug Something isn't working

Comments

@nejec
Copy link

nejec commented Jun 6, 2024

I am a bit puzzled on the configuration of ignored rules. The documentation states that inline directive are the highest and ignoring all rules in the config is the lowest ones.

For now, we were always using regal in CI with CLI exclusions (e.g. --disable-rule todo-comments) and it worked pretty well. However, we wanted to change things around and found out some inconsistencies, which I can't find any explanation for it.

Setup

$ regal version
Version:    0.22.0
Go Version: go1.22.0
Platform:   darwin/arm64
Commit:     unknown
Timestamp:  2024-05-22T11:29:10Z
Hostname:   github.actions.local
$ find opa -type f
opa/bundle/product/service/tests/http_test.rego
opa/bundle/product/service/http.rego
opa/bundle/.manifest
$ cat opa/bundle/product/service/http.rego
package product.service

# regal ignore:unresolved-import
import data.provider.hub
import rego.v1

default allow := false

# METADATA
# entrypoint: true
response := {
	"allow": allow,
	"parameters": parameters,
}

Default regal lint output

Default run of regal lint with direct file reference without any configuration:

$ regal lint opa/bundle/product/service/tests/http_test.rego opa/bundle/product/service/http.rego
Rule:         	unresolved-import
Description:  	Unresolved import
Category:     	imports
Location:     	opa/bundle/product/service/http.rego:4:1
Text:         	data.provider.hub
Documentation:	https://docs.styra.com/regal/rules/imports/unresolved-import

Default run of regal lint with directory reference without any configuration:

$ regal lint opa/
Rule:         	unresolved-import
Description:  	Unresolved import
Category:     	imports
Location:     	opa/bundle/product/service/http.rego:4:1
Text:         	data.provider.hub
Documentation:	https://docs.styra.com/regal/rules/imports/unresolved-import

2 files linted. 1 violation found in 1 file.

If I run regal lint on each file, no issues are detected:

$ regal lint opa/bundle/product/service/tests/http_test.rego
1 file linted. No violations found.
regal lint opa/bundle/product/service/http.rego
1 file linted. No violations found.

Neither of these runs detected inline ignore configuration.

Inconsistency?

Other information:

  • if I disable the rule in the CLI, the rule is ignored and exit status is 0
  • if I create .regal/config.yaml and configure the unresolved-import rule as ignored, $ regal lint opa/ detects the exception and rule does not trigger.

Question

I would really prefer the inline ignore directives but I really don't know how to use it, since they do not trigger in any case. So, what am I doing wrong?

@anderseknert
Copy link
Member

Hi @nejec, and thanks for filing this issue!

There's a couple of things going on here.

First of all — that rule is an aggregate rule, but that's clearly missing from the header in the documentation of that rule. Compare to the docs of e.g. impossible-not. Will have that fixed as soon as possible! So what's an aggregate rule, and what makes them special? A section in the README (under the rules table) explains that, but since it wasn't marked as one, it's no wonder that you wouldn't find it!

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 linting. These rules are called aggregate rules, and will only be run when there is more than one file to lint, such as when linting a directory or a whole policy repository. One example of such a rule is the prefer-package-imports rule, which will aggregate package names and imports from all provided policies in order to determine if any imports are pointing to rules or functions rather than packages. You normally won't need to care about this distinction other than being aware of the fact that some linter rules won't be run when linting a single file.

Since unresolved-import is an aggregate rule — meaning it can never resolve imports with only a single file provided — it is not executed when you run regal lint with only a single file to lint.

However, the issue you're seeing where the ignore directive isn't honored is certainly a bug, as even the documentation suggests that as a way to have it ignored. I'm a little confused myself about that, as this was a known issue around aggregate rules, as you can read more about in #344. We'll have this fixed in the next release.

In the meantime, a configuration file is likely your best option.

if I create .regal/config.yaml and configure the unresolved-import rule as ignored,

Don't do that though! The rule has a special except-imports attribute that'll allow you to list the imports that should be excepted, so there's no need to ignore the rule entirely:

.regal/config.yaml

rules:
  imports:
    unresolved-import:
      level: error
      except-imports:
        - data.provider.parameters

@anderseknert anderseknert added the bug Something isn't working label Jun 6, 2024
anderseknert added a commit that referenced this issue Jun 8, 2024
Also some light refactoring, but should hopefully not
be too distracting.

Fixes #803

Signed-off-by: Anders Eknert <[email protected]>
anderseknert added a commit that referenced this issue Jun 8, 2024
Also some light refactoring, but should hopefully not
be too distracting.

Fixes #803

Signed-off-by: Anders Eknert <[email protected]>
anderseknert added a commit that referenced this issue Jun 8, 2024
Also some light refactoring, but should hopefully not
be too distracting.

Fixes #803

Signed-off-by: Anders Eknert <[email protected]>
anderseknert added a commit that referenced this issue Jun 10, 2024
* Support ignore directives in aggregate rules

Also some light refactoring, but should hopefully not
be too distracting.

Fixes #803

Signed-off-by: Anders Eknert <[email protected]>

* Return `null` in textDocument/diagnostic handler (#812)

While the specification isn't clear here, Neovim apparently had issues
dealing with the empty object response. And since all the other editors
seem to handle a null response too, let's just go with that instead.

Fixes #810

Signed-off-by: Anders Eknert <[email protected]>

---------

Signed-off-by: Anders Eknert <[email protected]>
srenatus pushed a commit to srenatus/regal that referenced this issue Oct 1, 2024
* Support ignore directives in aggregate rules

Also some light refactoring, but should hopefully not
be too distracting.

Fixes StyraInc#803

Signed-off-by: Anders Eknert <[email protected]>

* Return `null` in textDocument/diagnostic handler (StyraInc#812)

While the specification isn't clear here, Neovim apparently had issues
dealing with the empty object response. And since all the other editors
seem to handle a null response too, let's just go with that instead.

Fixes StyraInc#810

Signed-off-by: Anders Eknert <[email protected]>

---------

Signed-off-by: Anders Eknert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants