opa
opa copied to clipboard
open-policy-agent
change input type for concat
Why the changes in this PR are needed?
This is to improve the concat function mentioned here [https://github.com/open-policy-agent/opa/issues/6911](ISSUE 6911)
What are the changes in this PR?
Changed concat to accept both string & number
Notes to assist PR review:
#6911
Further comments:
Deploy Preview for openpolicyagent ready!
| Name | Link |
|---|---|
| Latest commit | b729b832a8c33f05db58b0deffbd83181a8c70f4 |
| Latest deploy log | https://app.netlify.com/sites/openpolicyagent/deploys/66f251fe54b22f000847b043 |
| Deploy Preview | https://deploy-preview-7054--openpolicyagent.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
Deploy Preview for openpolicyagent ready!
| Name | Link |
|---|---|
| Latest commit | e3f58ad6b5b8e6c1fa6d8eee8304f5626ea1c82f |
| Latest deploy log | https://app.netlify.com/sites/openpolicyagent/deploys/66f252277f37dd00087b6b18 |
| Deploy Preview | https://deploy-preview-7054--openpolicyagent.netlify.app |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Hi @anderseknert looks like it passed all the checks, please help review it, thanks!
Code looks good. An extra test in the yaml test cases would be a nice touch. I'm also afraid this won't work as-is with wasm 🤔
Code looks good. An extra test in the yaml test cases would be a nice touch. I'm also afraid this won't work as-is with wasm 🤔
Thanks @srenatus , I will try to add an extra test case for this. Since I'm not using wasm, not familiar with it, would you mind share some docs about how to verify that in wasm? Thanks!
So as soon as we have a yaml tests case, the CI suite will run that yaml test case with standard eval (topdown) and wasm; and both tests need to succeed for the PR to go green.
Concretely, we'll need the same change you've done here for topdown in the wasm "base lib", which is a few C functions here:
- https://github.com/open-policy-agent/opa/blob/main/wasm/src/strings.c#L228-L233
- https://github.com/open-policy-agent/opa/blob/main/wasm/src/strings.c#L252
So as soon as we have a yaml tests case, the CI suite will run that yaml test case with standard eval (topdown) and wasm; and both tests need to succeed for the PR to go green.
Concretely, we'll need the same change you've done here for topdown in the wasm "base lib", which is a few C functions here:
- https://github.com/open-policy-agent/opa/blob/main/wasm/src/strings.c#L228-L233
- https://github.com/open-policy-agent/opa/blob/main/wasm/src/strings.c#L252
Thanks for the comments. Working on it.
@sunjh thanks for working on this. Just checking in to see if you had a chance to address Stephan's comments. We can then get this in the next release.
@sunjh closing this PR for now. Feel free to re-open when you get a chance to address the PR comments and we can get this in.
Sorry for the late response @ashutosh-narkar , lots of things going on here recently. And based on the policy of my organization, I cannot continue working on this any more. Please feel free to assign this task to others.
Thanks.