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

Refactor creation of prepared queries #604

Merged

Conversation

mjungsbluth
Copy link
Contributor

Before this refactoring only the PreparedQuery options could be passed when using this plugin as a library. There are two more options (prepare options and evaluation options) that can be passed when using the OPA library directly.

This change breaks the interface but also allows for extensibility of creating the prepared queries in the future.

@mjungsbluth mjungsbluth force-pushed the expose_eval_prepare_opts branch 2 times, most recently from 9d006a5 to 8c49c01 Compare October 14, 2024 15:31
@mjungsbluth mjungsbluth marked this pull request as ready for review October 15, 2024 15:28
Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

The changes look good. We should add a note in the change log about the interface change so that we can include it in the release notes. Also do the existing tests exercise the new changes?

@mjungsbluth
Copy link
Contributor Author

I will add a note to the changelog

As for tests: they cover all changes, however there never was coverage for passing options to the Eval method, I will add a minimal test.

We‘ll also integrate this in Skipper to do partial evaluation on prepare as an optimization. So we‘ll know how well the changed API works design wise…

Before this refactoring only the PreparedQuery options could be passed when using this plugin as a library. There are two more options (prepare options and evaluation options) that can be passed when using the OPA library directly.

This change breaks the interface but also allows for extensibility of creating the prepared queries in the future.

Signed-off-by: Magnus Jungsbluth <magnus.jungsbluth@zalando.de>
@mjungsbluth mjungsbluth force-pushed the expose_eval_prepare_opts branch from 3e3484a to 1e06685 Compare October 21, 2024 13:00
@mjungsbluth
Copy link
Contributor Author

@ashutosh-narkar I added an item to the changelog and also added a test that verifies a user of a library can modify the existing behaviour.

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @mjungsbluth!

@ashutosh-narkar ashutosh-narkar merged commit 765ad70 into open-policy-agent:main Oct 21, 2024
8 checks passed
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.

2 participants