Closed Akamatsu21 closed 5 months ago
Hey @Akamatsu21 - We're taking a look at the PR - but maybe it'll be worth to add some tests here :) Thanks for the contribution !
Hey, @Akamatsu21, do you have an estimate of when you can add tests?
Hey, @Akamatsu21, do you have an estimate of when you can add tests?
Hello and big apologies, I have been away for the past two weeks on a long holiday. I will have a look at adding some tests, you can expect them by the end of this week. Appreciate your patience on this!
Hello @obsd @RazcoDev, I just pushed some unit tests, looking forward to your feedback.
Thanks @Akamatsu21, I will ask someone to review :)
We currently don't have CI to run the test so we run them manually, please attach a screenshot of the successfully running tests after your changes :)
Correct me if I'm wrong but the PR is missing the DataStore changes - validations
Correct me if I'm wrong but the PR is missing the DataStore changes - validations
Hi @omer9564 thank you so much for the review, I will address your comments today. As mentioned in the discussion the data store validation is the next item on the list, but I wanted to check that the logic thus far has been correct before I attempt it. I am not expecting the PR to be merged before that's finished.
Hey @Akamatsu21 , Overall logic looks very good, besides what I wrote in the comments everything seems ok. Please tag me once you finish with the data store validation and I will re-review everything :)
Hi @omer9564 apologies you had to wait quite long for this fairly small update. I completed all the remaining tasks on my original list, please let me know if this is good to be merged. I am attaching a screenshot of passing tests, this was run on the RustRover IDE with the cargo version 1.76.0 (c84b36747 2024-01-18).
By the way, I merged the main branch into this one in order to make sure any tests added by other people still pass. I appreciate in hindsight that maybe a rebase would have made for a cleaner commit history, hope it's not a big issue.
Hi @omer9564 apologies you had to wait quite long for this fairly small update. I completed all the remaining tasks on my original list, please let me know if this is good to be merged. I am attaching a screenshot of passing tests, this was run on the RustRover IDE with the cargo version 1.76.0 (c84b36747 2024-01-18).
By the way, I merged the main branch into this one in order to make sure any tests added by other people still pass. I appreciate in hindsight that maybe a rebase would have made for a cleaner commit history, hope it's not a big issue.
Thanks for the update, will take a look in the following days :)
Sorry for the delay, had busy couple of weeks. Will hopefully check this in the first week of April.
Looks very good in general,
- left a single comment on how the error handling of the policy store can be simplified with a reference to rust docs
- left a single comment on a missing validation on schema mutation @Akamatsu21 Sorry for taking so long to re-review 😅
No worries, and also sorry for taking a while. I should be able to respond faster now. I added the missing validation, but still think the unpacking of that error type is as good as it's going to be, unless we agree to just overhaul the return types
@Akamatsu21 I will try to push it next week :)
@Akamatsu21 Will merge & release a new version later this week
Fixes the discussion #23
Created the SchemaMemoryStore with get, update and delete operations Implemented creating schema from file Added API endpoints for the three operations Added validation to policy create/update endpoints