-
Notifications
You must be signed in to change notification settings - Fork 0
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 code exec functionality from edx-arch-experiments #6
Conversation
Copy of https://github.com/edx/edx-arch-experiments/blob/081c392a49113c77fceb2a59987f5751fecdd84e/edx_arch_experiments/codejail_service/views.py and tests with no modifications. Cleanup happens in the next commit.
This now correctly responds to code exec calls: ``` curl -sS 'http://localhost:8080/api/v0/code-exec' -d 'payload={"code": "x = 6 * 7", "globals_dict":{}}' | jq .globals_dict.x 42 ``` - Remove cookiecutter-generated v1 api - Add url routes for v0 api that LMS and CMS will expect to use - Stop using reverse() in tests because it would have to be `reverse('api:v0:code_exec')` and it would be easier to just write `/api/v0/code_exec'` - Simplify existing routes to be simple path constants instead of regular expressions. - Add dependencies - Strip out DB-dependent bits (auth) - Remove toggle (only made sense when this was an app) - Tweak some comments and log lines - Lint cleanup (**dict form)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you reviewed changes to the eduNEXT service between when you first created edx_arch_experiments.codejail_service
and now?
|
||
app_name = 'api' | ||
urlpatterns = [ | ||
re_path(r'^v1/', include(v1_urls)), | ||
path('v0/', include(v0_urls)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this v1. To me, v0 is when you are trying to work out an API, but the API has already been worked out in the eduNEXT version of the service, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually hardcoded as v0 in edxapp right now.
I don't think their code has changed in a couple years. |
I'd love to hear more confidence in:
Like, I checked, and there is nothing I need to account for. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to know that there are no updates that need to be accounted for, but otherwise, looks good.
I can confirm it -- https://github.com/eduNEXT/codejailservice/tree/main/codejailservice shows no changes beyond the tests dir for 3 years. |
But also, I built the original app mostly by looking at the client code and what it wanted -- I didn't consult the eduNEXT version very much. |
See commit messages for description of changes. Summary:
edx_arch_experiments.codejail_service
Merge checklist:
Check off if complete or not applicable: