Redefines experiamental RPCAPI batch_status#1304
Conversation
|
Looks great. It would be nice if we could also use the terminology from zonemaster/zonemaster-backend#960 to the extent that it's not too much trouble. |
24867e1 to
46456bf
Compare
46456bf to
b88d189
Compare
mattias-p
left a comment
There was a problem hiding this comment.
I had a closer look at this. Nothing is wrong but I have a few suggestions on how it could be improved. Ultimately I think this is update fills an important gap in the API.
There was a problem hiding this comment.
Looks good.
I suggest that we address that comment about the type of the batch_id parameter in a different PR. I’ll handle that.
Never mind, the documentation already states: “When a method expects an integer arguments (sic), numbers encoded in strings are also accepted and used transparently, and numbers with fractions are rounded to the nearest integer.”
| * [API method: add_batch_job](#api-method-add_batch_job) | ||
| * [API method: get_batch_job_result](#api-method-get_batch_job_result) | ||
| * [API method: get_test_params](#api-method-get_test_params) | ||
| * [API method: batch_status](#api-method-batch_status) |
There was a problem hiding this comment.
Why not get_batch_status instead? To be consistent with (most) other methods.
There was a problem hiding this comment.
I decided to reuse the already defined name batch_status. It is there, but experimental. I felt that we had already decided on those names.
At the end of the current version of the document you find
## Experimental API methods
There are also some experimental API methods documented only by name:
* system_versions
* conf_profiles
* conf_languages
* lookup_address_records
* lookup_delegation_data
* job_create
* job_status
* job_results
* job_params
* domain_history
* user_create
* batch_create
* batch_status
mattias-p
left a comment
There was a problem hiding this comment.
This looks good to me.
It seems I forgot to mention a couple of minor nits in my last review, so I'm adding them here. Sorry about that. But I don't want to stall this PR so I'm approving anyways.
Co-authored-by: Mattias Päivärinta <mattias@paivarinta.se>
73ac0c6
* Makes API get_batch_job_result documented as deprecated. See PR zonemaster/zonemaster#1304 * Redefines API batch_status. See the same PR. zmb() is updated to be able to use the updated batch_status().
* Clarifies JSON examples * Corrects that empty lists of test IDs are never included in a response. * Clarifies that empty lists of test IDs are never included in responses. * No change of logic.
|
|
@mattias-p, please re-review. |
Purpose
Current
get_batch_job_resultis not optimal for repeated checking the status of a large batch since always included the test ID of finished tests. This PR redefines experimentalbatch_statuswhich is only an alias forget_batch_job_result. With the new definition the response is by default minimal and only including the numbers.This is a document change only. When this has been reviewed it will also be implemented in Zonemaster-Backend.
Context
zonemaster/zonemaster-backend/issues/960
zonemaster/zonemaster-backend#1215 (implementation)
How to test this PR
Review.