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

Add OPAProperties to structure properties #24

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ali-jalaal
Copy link

Implements #21

Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

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

Thank you! Looks like a solid imrovement to me. This is backwards compatible, right? Anything we need to the docs, or update? Looks like some tests are failing, so that'd be good to look into.

@ali-jalaal
Copy link
Author

Hi @anderseknert, thanks for your review.

This is backwards compatible, right?

Yes, I suppose these changes are backward compatible since public constructors/methods of OPAAuthorizationManager are kept as before and behavior should be the same.

Anything we need to the docs, or update?

I'd suggest that we can update the docs after implementing #22 since after that we could use the features of auto-configuration and configuration-properties.

Looks like some tests are failing

The new tests were passing, however the old tests were failing. By enabling more logs, it seems OPA v1 + nginx have some problems and OPAClient does not return any response (exception: java.io.IOException: HTTP/1.1 header parser received no bytes). In the 3rd commit, I reverted OPA docker image to the 0.70.0 version and all tests were 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