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

ctclient fails if any unexpected arguments are found #926

Merged
merged 1 commit into from
May 10, 2022

Conversation

mhutchinson
Copy link
Contributor

@mhutchinson mhutchinson commented May 9, 2022

All of the parameterization to the subcommands is performed by flags. However, as the subcommands themselves are selected via normal arguments (no hyphens), this can lead to newcomers (e.g. me) believing that the log to fetch from could also be a bare argument. On something like get-entries, there was no way to know that the logname was ignored. Now, this will explode and the user gets the option to reconsider how they are specifying the parameters.

e.g. go run github.com/google/certificate-transparency-go/client/ctclient@master get-entries submariner --first 35494500 --last 35494590 looks very reasonable and returns data, but it isn't from submariner.

#900

All of the parameterization to the subcommands is performed by flags. However, as the subcommands themselves are selected via normal arguments (no hyphens), this can lead to newcomers (e.g. me) believing that the log to fetch from could also be a bare argument. On something like get-entries, there was no way to know that the logname was ignored. Now, this will explode and the user gets the option to reconsider how they are specifying the parameters.
@mhutchinson mhutchinson requested a review from pav-kv May 9, 2022 15:50
Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

This PR looks good, it reduces possibility of shooting oneself in the foot when using the tool.

Separately, we could think of making the interface more intuitive, by using the non-flag args, sort of like you originally tried to use it.

E.g. ./ctclient get-consistency-proof argon2022 100 200 looks more intuitive than ./ctclient get-consistency-proof --log_name=argon2022 --some-flag-that-I-always-forget-the-name-of=100 --another-flag-which-I-dont-remember=200.

@mhutchinson
Copy link
Contributor Author

Yeah definitely! I think this is the first right step: tighten things up and avoid surprising behaviour. And future work can just make the whole thing feel more natural, without introducing more ambiguity. 👍

@mhutchinson mhutchinson merged commit 81944ab into google:master May 10, 2022
@mhutchinson mhutchinson deleted the blowuponextraargs branch May 10, 2022 09:16
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

2 participants