Skip to content

Redefines batch_status to replace get_batch_job_result #1215

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

Merged
merged 12 commits into from
Jun 11, 2025

Conversation

matsduf
Copy link
Contributor

@matsduf matsduf commented May 12, 2025

Purpose

This PR implements the RPCAPI update in zonemaster/zonemaster/pull/1304.

Changes

batch_status was defined as an alias to get_batch_job_result. Now it redefined to be a smarter replacement of the old API.

How to test this PR

Prepare a small batch (10-15 domain names) in a text file. Set down number_of_processes_for_frontend_testing and number_of_processes_for_batch_testing to a low value (e.g. 2). Start the batch with zmb and read the batch status with zmb. Read the status with various values of the optional options.

* 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().
@matsduf matsduf added this to the v2025.1 milestone May 12, 2025
@matsduf matsduf added the V-Minor Versioning: The change gives an update of minor in version. label May 12, 2025
Copy link
Contributor

@marc-vanderwal marc-vanderwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK. Some minor suggestions.

Co-authored-by: Marc van der Wal <[email protected]>
@matsduf matsduf requested a review from marc-vanderwal May 20, 2025 11:01
marc-vanderwal
marc-vanderwal previously approved these changes May 20, 2025
@matsduf
Copy link
Contributor Author

matsduf commented May 28, 2025

@mattias-p, you approved zonemaster/zonemaster#1304 (the specification). Can you please review/approve the PR (the implementation) too?

Copy link
Member

@mattias-p mattias-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. I have some comments.

Co-authored-by: Mattias Päivärinta <[email protected]>
@matsduf
Copy link
Contributor Author

matsduf commented Jun 4, 2025

@mattias-p and @marc-vanderwal, could you review?

* Adds documentation on "no-" prefixed options.
* Creates and uses a new function for conversion into JSON
  boolean.
@matsduf
Copy link
Contributor Author

matsduf commented Jun 4, 2025

@mattias-p, I have reverted the change of the json_tern function and created a new one instead. Please review.

marc-vanderwal
marc-vanderwal previously approved these changes Jun 5, 2025
@matsduf
Copy link
Contributor Author

matsduf commented Jun 5, 2025

@marc-vanderwal, based on feedback from @mattias-p I changed the options for the new method. Can you rereview?

@mattias-p, please rereview.

marc-vanderwal
marc-vanderwal previously approved these changes Jun 5, 2025
Copy link
Member

@mattias-p mattias-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some comments about tightening up things, but otherwise this looks good.

Co-authored-by: Mattias Päivärinta <[email protected]>
Copy link
Contributor

@MichaelTimbert MichaelTimbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and working.

@matsduf matsduf merged commit 3f0877d into zonemaster:develop Jun 11, 2025
5 checks passed
@matsduf matsduf deleted the redefine-batch_status-api branch June 11, 2025 12:10
@MichaelTimbert MichaelTimbert added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing V-Minor Versioning: The change gives an update of minor in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants