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

feat: support []string and ast.Value in ast.InterfaceToValue #7306

Merged

Conversation

regeda
Copy link
Contributor

@regeda regeda commented Jan 23, 2025

Why the changes in this PR are needed?

  1. ast.InterfaceToValue walks through json encode/decode for the native ast.Value type. It doesn't make sense as far as the function returns ast.Value.
  2. Regarding []string case, it reduces the number of iterations

What are the changes in this PR?

Updates in ast.InterfaceToValue function.

Notes to assist PR review:

Further comments:

@regeda regeda force-pushed the adjust-interface-to-value branch from 354b800 to 3280531 Compare January 23, 2025 14:52
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.

Seems alright to me. I’m curious though — where do we ever pass this a Value?

@regeda
Copy link
Contributor Author

regeda commented Jan 23, 2025

Seems alright to me. I’m curious though — where do we ever pass this a Value?

I'm thinking that could be helpful in opa-envoy-plugin where "check request" should be converted into ast.Value.

@anderseknert
Copy link
Member

anderseknert commented Jan 23, 2025

It would be good if you could provide an example where any of these two new paths are hit. I just tried this addition in Regal, and none of them are in that context. But OTOH we don't hit that function much at all since we use an optimized version for that use case. Let's just make sure that this isn't just dead code in any real project integration.

EDIT: heh, I just noticed that we do have []string implemented there, so I guess it is hit after all, just not in OPA in that integration. Still, something to show that would be great.

@anderseknert anderseknert force-pushed the adjust-interface-to-value branch from 3280531 to e2cbc03 Compare January 31, 2025 19:25
Copy link

netlify bot commented Jan 31, 2025

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit e2cbc03
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/679d23a7477ce10008f56b9c
😎 Deploy Preview https://deploy-preview-7306--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@anderseknert anderseknert merged commit 7ddaff2 into open-policy-agent:main Jan 31, 2025
28 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