Rule: prefer-package-imports
#349
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anderseknert
force-pushed
the
prefer-package-imports
branch
2 times, most recently
from
b0c38fc
to
35dce6d
Compare
charlieegan3
approved these changes
Sep 27, 2023
| @@ -565,15 +571,76 @@ func (l Linter) lintWithRegoRules( | |||
| } | |||
| } | |||
|
|
|||
| func (l Linter) lintWithRegoAggregateRules( | |||
There was a problem hiding this comment.
Does it makes sense to cover this in linter_test too?
There was a problem hiding this comment.
I've added a simple test 👍 And as it's covered by an e2e test too it's probably good enough for the time being.
| aggregate_report contains violation if { | ||
| all_package_paths := {pkg | | ||
| some entry in input.aggregate | ||
| pkg := entry.aggregate_data.package_path |
There was a problem hiding this comment.
Even if a file has no imports, its package path will still be set here right? I think this is the case, but just wanted to check that all_package_paths will be set to all packages, rather than all packages with imports.
This builds upon the design laid out in #282, and on @sesponda's excellent work on that. My approach for testing this was to implement an actual "real world" linter rule from the backlog, and `prefer-package-imports` seemed like a good choice. The rule aggregates package names (or paths) from each input file, along with any import paths. The report rule then checks each import collected against the list of package paths to determine if the import matches a package. If not, the rule will consider that a violation. As is often the case when real world meets the original design, "some" modifications proved to be necessary. The collection of aggregate data is now run alongside the usual linter rule evaluation, as we conveniently may use the same input files for aggregating data. Once that's done, we now send another eval call using only the data aggregated as input. Each aggregate rule is now provided only the data aggregated by _that rule_, so there's no need for policy authors to traverse through all of the aggregates, but one may simply refer to the `input.aggregate` array for the same set of entries as one collected previously. I've also had to add an alternative schema for the input when these rules are evaluated (currently only enforced by `regal test`). Perhaps `anyOf` would be a better option than two separate schemas, but I'll explore that at a later point. Fixes #326 Fixes #343 Signed-off-by: Anders Eknert <anders@styra.com>
anderseknert
force-pushed
the
prefer-package-imports
branch
from
35dce6d
to
9a6994e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This builds upon the design laid out in #282, and on @sesponda's excellent work on that. My approach for testing this was to implement an actual "real world" linter rule from the backlog, and
prefer-package-importsseemed like a goodchoice. The rule aggregates package names (or paths) from each input file, along with any import paths.
The report rule then checks each import collected against the list of package paths to determine if the import matches a package. If not, the rule will consider that a violation.
As is often the case when real world meets the original design, "some" modifications proved to be necessary.
The collection of aggregate data is now run alongside the usual linter rule evaluation, as we conveniently may use the same input files for aggregating data. Once that's done, we now send another eval call using only the data aggregated as input. Each aggregate rule is now provided only the data aggregated by that rule, so there's no need for policy authors to traverse through all of the aggregates, but one may simply refer to the
input.aggregatearray for the same set of entries as one collected previously.I've also had to add an alternative schema for the input when these rules are evaluated (currently only enforced by
regal test). PerhapsanyOfwould be a better option than two separate schemas, but I'll explore that at a later point.Fixes #326
Fixes #343