-
Notifications
You must be signed in to change notification settings - Fork 35
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
expose policy info from the engine #255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thedavemarshall Thank you for taking this on!
I understand #254 to want the Engine to provide a way to obtain entire policy file contents. I don't think the PR addresses that yet.
@anakrish I know you've mentioned the ast and some of the internals are unstable and being iterated on, so I'd love you input here if this is a good approach, or if you're not ready to expose these things through the engine.
I have been thinking about exposing the AST. In addition to the AST likely being changed in some time, the other challenge is how we can expose the AST to the bindings. The AST nodes are going to be cumbersome to expose to each language.
Some thoughts:
- We could expose AST publicly via a cargo feature. That will allow it to be used and refined based on feedback.
- We could version the AST.
- Instead of ad-hoc functions should we just define a mapping of AST nodes to json? We could then serialize entire AST to json and make it available to the different bindings as well.
I'd love your feedback since you have a concrete usecase.
@@ -129,6 +129,61 @@ impl Engine { | |||
.collect() | |||
} | |||
|
|||
pub fn get_packages_texts(&self) -> Result<Map<String, String>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that these APIs are public, they would need to be documented as well as tested via doc tests.
@@ -129,6 +129,61 @@ impl Engine { | |||
.collect() | |||
} | |||
|
|||
pub fn get_packages_texts(&self) -> Result<Map<String, String>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding, what is the use of such an that maps filenames to package names?
Ok(packages_text) | ||
} | ||
|
||
pub fn get_packages_imports(&self) -> Result<Map<String, Vec<String>>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there is also a as
part:
import data.x as y
@anakrish thanks for the thoughtful feedback!
ah I think I misunderstood when I first read those requirements, my mistake!
yeah, I think AST exposure is a great candidate for a rust crate feature
I like this idea! Or at least versioning the JSON output?
I think this is similar to how opa does it for their policy HTTP API, docs
I think exposing a JSON representation of the AST is a good solution. Tomorrow I'll take an in-depth look at #266 , which at first glance seems a great fit for my use case! Closing this PR in favor of #266 |
This change is to expose more package data from the engine, potentially addressing #254
@anakrish I know you've mentioned the ast and some of the internals are unstable and being iterated on, so I'd love you input here if this is a good approach, or if you're not ready to expose these things through the engine.
I had some trouble getting a method of
package name -> [rule_name1, rule_name2]
, so I'm curious if there's a good way to do that with the current abstractions, or if that's something I could potentially help out with.