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

bugs/not-equals-in-loop #130

Merged
merged 1 commit into from
Jun 1, 2023
Merged

bugs/not-equals-in-loop #130

merged 1 commit into from
Jun 1, 2023

Conversation

anderseknert
Copy link
Member

Like a few other rules, this won't catch all violations as we don't yet traverse nested structures like comprehensions or every constructs.

Fixes #79

bundle/regal/rules/bugs/bugs.rego Show resolved Hide resolved

some i
neq_term.value[i].type == "var"
startswith(neq_term.value[i].value, "$")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have a "constants" package and put the "wildcardprefix" in it. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah, that would read better. We're doing a lot of these checks in the very hot path of find_vars and such, so I wonder if there'd be any difference in performance using a string literal versus a data reference? 🤔 I guess I'd have to measure. Making a note on that.

Comment on lines +13 to +15
deny if {
"admin" != input.user.roles[_]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also work for this, right?

Suggested change
deny if {
"admin" != input.user.roles[_]
}
deny if "admin" != input.user.roles[_]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I've added a test case for good measure. I think I'll keep the longer version in the docs though as it makes it clearer that the condition is in the rule body. But maybe it's just my old habits.


```rego
package policy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, should we use if here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! Fixed.

Like a few other rules, this won't catch all violations as
we don't yet traverse nested structures like comprehensions
or `every` constructs.

Fixes #79

Signed-off-by: Anders Eknert <anders@styra.com>
@anderseknert anderseknert merged commit 7518be5 into main Jun 1, 2023
@anderseknert anderseknert deleted the not-equals-in-loop branch June 1, 2023 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule: warn on != in iteration
2 participants