feat: Add codejail_service app for transition to containerized codejail #530
Conversation
timmc-edx
force-pushed
the
timmc/codejail-service
branch
2 times, most recently
from
934e7ee to
449b6e1
Compare
timmc-edx
force-pushed
the
timmc/codejail-service
branch
from
449b6e1 to
ecc21eb
Compare
| jsonschema.validate(params, payload_schema) | ||
|
|
||
| complete_code = params['code'] # includes standard prolog | ||
| input_globals_dict = params.get('globals_dict') or {} |
There was a problem hiding this comment.
Do we have to use or here because params.get('globals_dict', {}) will return None if None is the actual value of the key?
There was a problem hiding this comment.
Oh, hmm. That was the idea, but looking again at safe_exec.py in edx-platform (and the calling code in ../capa_problem.py) I see that I had misread; globals_dict is not defaulted to None, and will always be a dictionary (because it's functionally a return argument, and is updated in-place).
I've also set the schema to disallow an explicit None for globals_dict, so this line can already be safely changed to params['globals_dict']. I'll make that change and I'll add globals_dict to the required-fields list in the schema.
| # There's no good reason to have a chain of >2 services passing | ||
| # codejail requests along, so only allow execution here if we | ||
| # aren't going to pass it along to someone else. | ||
| if getattr(settings, 'ENABLE_CODEJAIL_REST_SERVICE', False): |
There was a problem hiding this comment.
If we move this up a few lines we can avoid doing unnecessary work
There was a problem hiding this comment.
Nice, will do.
I should probably also change it to log + return a 500 rather than throwing an exception.
- Add dependency on ddt - Report json errors cleanly
|
Ready for re-review. I added a bunch of unit tests and made a few fixes (see individual commits), and have manually restested. |
| Arguments: | ||
| user: User to authenticate as when calling view (None for unauthenticated) | ||
| exp_status: Assert that the response HTTP status code is this value | ||
| exp_body: Assert that the response body JSON is this value |
There was a problem hiding this comment.
missing a few arguments here
There was a problem hiding this comment.
Thanks, fixed. And one of the lines was a lie, too.
- Move `files` kwarg to later in list to match usage - Correct the `user` arg documentation (None does not mean unauthenticated) - Update changelog date
Merge checklist:
Check off if complete or not applicable: