opa
opa copied to clipboard
open-policy-agent
fixes #4958
Fixes https://github.com/open-policy-agent/opa/issues/4958
Signed-off-by: Martin Johansen [email protected]
Yeah, was a bit unsure about whether to add one or not. However, as mentioned in the issue it is pretty simple to break the tests by adding a default rule to one of the modules in the tests. I think it could make sense to just change those, since this issue would have been caught if the tests covered bundles with default rules as well. I'm not sure if it makes sense to add completely new tests. But I am happy to discuss the best approach here
I tend to prefer new tests, but if you think there are existing tests that could be retrofitted to test this while still testing what they currently do, that should be fine too :)
Makes sense :) I'm not 100% sure what the best approach for a new test here would be... Maybe @ashutosh-narkar has a good suggestion because he implemented the initial tests for the lazymode feature?
@martinjoha thanks for reporting the issue and adding a fix. I don't think need to update the interface with a new ID field. If we trim the policy path for the scenario where the file being processed contains policy in the truncate methods for both the disk and in-memory store this should fix the issue.
@ashutosh-narkar Ok, makes sense. What are your thoughts regarding test cases here? My initial thought is that it should be enough to modify one or some of the tests by adding a default rule to a policy as well as ensuring the same amount of modules before and after activating the delta bundle. WDYT?
@ashutosh-narkar Ok, makes sense. What are your thoughts regarding test cases here? My initial thought is that it should be enough to modify one or some of the tests by adding a default rule to a policy as well as ensuring the same amount of modules before and after activating the delta bundle. WDYT?
That works or you could add a new case which basically does what the old ones do with the added default rule and checks.
I'm not completely sure how you would do that. From my understanding storage.Path can't be made without starting with /, and that's not what we need. Do you have a suggestion on how to do it?
However, I guess we could still change the Truncate methods in the disk and in-memory stores, as you first suggested. That would still require us to change a couple of the tests in inmem_test and disk_test. If you're fine with that, the change is pretty quick.
However, I guess we could still change the Truncate methods in the disk and in-memory stores, as you first suggested. That would still require us to change a couple of the tests in inmem_test and disk_test. If you're fine with that, the change is pretty quick.
I think doing something like strings.TrimLeft(update.Path.String(), "/") in the disk and in-mem truncate calls and fixing existing tests that break and adding the new one you already have should be good.
Thanks @martinjoha ! If you can rebase and squash your commits we can merge this.