Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement zmb get_test_params #768

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Implement zmb get_test_params #768

merged 4 commits into from
Jun 16, 2021

Conversation

mattias-p
Copy link
Member

@mattias-p mattias-p commented May 21, 2021 •
edited
Loading

Purpose

Add another RPC API method to zmb.

Context

N/A

Changes

Add zmb get_test_params.

Rename the --testid parameter to --test-id for the test_progress and get_test_results commands. The old parameter name still works but emits a deprecation warning.

How to test this PR

Start a test and verify that the output from get_test_params looks reasonable:

testid="$(zmb start_domain_test --domain example | jq -r .result)"
zmb get_test_params --test-id "$testid" | jq .

Verify that zmtest still works:

zmtest example.com

@mattias-p mattias-p added this to the v2021.2 milestone May 21, 2021
@mattias-p mattias-p marked this pull request as ready for review June 1, 2021 15:34
@mattias-p mattias-p requested review from matsduf and a user June 1, 2021 15:34
matsduf
matsduf previously approved these changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

It looks fine. It works as expected. I think we should include all API methods into zmb (but not in this PR).

@matsduf
Copy link
Contributor

matsduf commented Jun 4, 2021

@PNAX, can you review this?

ghost
ghost suggested changes Jun 8, 2021
View reviewed changes
script/zmb
zmb [GLOBAL OPTIONS] get_test_params [OPTIONS]

Options:
--test-id TEST_ID
Copy link

Choose a reason for hiding this comment

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

It looks like other methods use --testid (see test_progress and get_test_results). Could you change this one to be consistent with the other ? (Or the other way around).

Copy link
Contributor

Choose a reason for hiding this comment

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

Outside this change I find two --testid and one --test-id. I agree that making them consistent would be good. I might also affect zmtest for which zmb is a backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that this sucks, but I actually made it so in order to be consistent with the RPCAPI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can start by making it consistent in zmb because it is more likely that you do things manually there. Or support both.

Copy link

Choose a reason for hiding this comment

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

Could you then update all the other one to --test-id in this PR, so they all evolve together ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just checked and it's inconsistent all over the place. Maybe we should start migrating to testid for test ids across the board. I'll deprecate all test-id, test_id and id that I can find.

Copy link
Member Author

@mattias-p mattias-p Jun 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I created #790 to better facilitate such a migration. I also created #791 for the migration itself.

@mattias-p
Copy link
Member Author

I renamed the --testid parameters so they're all named --test-id now.

@PNAX @matsduf Please re-review.

@mattias-p
Copy link
Member Author

I'll make another update that does away with the deprecations in zmb. It clearly states that it's interface is unstable so we may well make use of that.

@matsduf
Copy link
Contributor

matsduf commented Jun 15, 2021

@PNAX, are your comments on this PR adressed by the last updates?

ghost
ghost approved these changes Jun 16, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

yes, this looks good to me.

@mattias-p mattias-p merged commit 2a9df58 into zonemaster:develop Jun 16, 2021
@ghost
Copy link

ghost commented Nov 25, 2021

v2021.2 testing

LGTM

@ghost ghost added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Nov 25, 2021
@mattias-p mattias-p deleted the get-test-params branch July 19, 2023 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matsduf matsduf matsduf approved these changes

Assignees
No one assigned
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing
Projects
None yet
Milestone
v2021.2
Development

Successfully merging this pull request may close these issues.
2 participants
@mattias-p @matsduf