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.

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

Synchronize option, limits and defaults with the API #192

Merged
merged 3 commits into from Oct 12, 2017
Merged

Conversation

chrisamin
Copy link
Contributor

Synchronize option, limits and defaults with the API where relevant,and include a new dev script for checking the tools against the OpenAPI endpoint.

This means adding a few more options that are supported by the API but not the tools, and aligning the minimums and maximums with current reality. In general the behaviour of the CLI has not changed, except that server-side validation errors are less likely and certain values are now possible.

and include a new dev script for checking the tools against the OpenAPI
endpoint.
@astrikos
Copy link
Contributor

The other day i tried to use resolve_on_probe option and I was surprised we hadn't expose it here. Was that on purpose (I don't remember honestly)? Can you maybe add it since you are touching that now? or I can also do a PR later :)

@@ -50,7 +50,7 @@ def add_arguments(self):
)
specific.add_argument(
"--port",
type=ArgumentType.integer_range(minimum=1, maximum=2**16),
type=ArgumentType.integer_range(minimum=1, maximum=65535),
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to allow HTTP measurements to every possible port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know actually, I guess it's okay? But if we restrict it we should do it on the API side first.

Copy link
Member

@Jterbeest Jterbeest 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 as far as I can tell ;)

@chrisamin
Copy link
Contributor Author

@astrikos you're right, resolve_on_probe was missed by my checker because I was conflating it with use_probe_resolver. I'll add it with this PR.

@chrisamin chrisamin merged commit e6c69cf into master Oct 12, 2017
@chrisamin chrisamin deleted the sync-options branch October 12, 2017 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants