Allow 'new_clusters' key in 'jobs reset' option to mimic JSON API pay… #524
Conversation
Codecov Report
@@ Coverage Diff @@
## main #524 +/- ##
==========================================
+ Coverage 60.93% 60.95% +0.02%
==========================================
Files 55 55
Lines 4669 4672 +3
==========================================
+ Hits 2845 2848 +3
Misses 1824 1824
Continue to review full report at Codecov.
|
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
@janvandervegt Thanks for the PR, looks reasonable. Please check out the comments.
| RESET_JSON = '{"job_name": "test_job"}' | ||
|
|
||
| CORRECT_RESPONSE = {'job_id': 1, | ||
| 'new_settings': json.loads(RESET_JSON)} |
There was a problem hiding this comment.
This should be CORRECT_REQUEST_PAYLOAD if I'm reading the test correctly.
| assert jobs_api_mock.reset_job.call_args[0][0] == CORRECT_RESPONSE | ||
|
|
||
|
|
||
| RESET_JSON_NEW_SETTINGS = '{"new_settings": {"job_name": "test_job"}}' |
There was a problem hiding this comment.
Can use RESET_JSON to avoid duplication.
| } | ||
| if 'new_settings' in deser_json: | ||
| request_body = deser_json | ||
| request_body['job_id'] = job_id |
There was a problem hiding this comment.
What about checking for new_clusters as the title implies? If you specify a dict with only that key, it would be wrapped in the new_settings field in the else branch. I suspect you may need to check for more than just the new_settings key here to determine what is actually specified.
|
Resolved by #582. |
3 participants
Added check for 'new_clusters' key so that the regular JSON payload for the reset endpoint is also accepted