cli: use Cobra's positional argument validation #24399
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
nvanbenschoten
force-pushed
the
nvanbenschoten/argVal
branch
from
df8d2fd
to
5d21edd
Compare
knz
approved these changes
Apr 2, 2018
pkg/cli/cert.go
Outdated
| @@ -84,6 +85,12 @@ Requires a CA cert in "<certs-dir>/ca.crt" and matching key in "--ca-key". | |||
| If "ca.crt" contains more than one certificate, the first is used. | |||
| Creation fails if the CA expiration time is before the desired certificate expiration. | |||
| `, | |||
| Args: func(cmd *cobra.Command, args []string) error { | |||
| if len(args) == 0 { | |||
| return errors.Errorf("no hosts specified. Need at least one") | |||
There was a problem hiding this comment.
create-node requires at least one host name or address, none was specified
| @@ -55,11 +55,12 @@ import ( | |||
| ) | |||
|
|
|||
| var debugKeysCmd = &cobra.Command{ | |||
| Use: "keys [directory]", | |||
| Use: "keys <directory>", | |||
There was a problem hiding this comment.
omg thank you thank you this has been bugging me for ages
Positional argument validation was added to spf13/cobra in spf13/cobra#284, but our cli still relies on custom validation in each of the "run" functions. This validation is unstandardized and overly permissive in certain cases. This change fixes this by replacing our custom logic with validation expressed using Cobra's `PositionalArgs` validators. Release note: None
Required args should not be put in square brackets. Release note: None
Done. TFTR! |
nvanbenschoten
force-pushed
the
nvanbenschoten/argVal
branch
from
5d21edd
to
5f21e76
Compare
|
bors r=knz |
craig bot
pushed a commit
that referenced
this pull request
Apr 12, 2018
24529: distsql: collect tableReader stats and output to trace r=asubiotto a=asubiotto
This PR is split into two commits. The first commit lays the groundwork for stat collection code. The stats are modeled as a proto to be sent over the wire in the future (we will be sending these stats as metadata). The second commit modifies the tableReader to use this InputStatCollector to count the number of rows it reads. Example usage:
```
root@:26257/> SELECT COUNT(*) FROM test.xyz;
+-------+
| count |
+-------+
| 7 |
+-------+
(1 row)
Time: 257.249037ms
root@:26257/> SET tracing = ON;
SET
Time: 362.575µs
root@:26257/> SELECT x,y,z FROM test.xyz ORDER BY x;
+---+---+------+
| x | y | z |
+---+---+------+
| 1 | 1 | NULL |
| 1 | 1 | 2 |
| 1 | 1 | 2 |
| 1 | 2 | 1 |
| 2 | 2 | 3 |
| 4 | 5 | 6 |
| 4 | 1 | 6 |
+---+---+------+
(7 rows)
Time: 207.334037ms
root@:26257/> SHOW TRACE FOR SESSION;
... rest of trace elided ...
| 2018-04-05 20:53:01.571467+00:00 | 21s794ms540µs951ns | waiting for read lock | [n1,s1,r23/1:/Table/5{2-3}] | | /cockroach.roachpb.Internal/Batch | 13 |
| 2018-04-05 20:53:01.777272+00:00 | 22s345µs429ns | read completed | [n1,s1,r23/1:/Table/5{2-3}] | | /cockroach.roachpb.Internal/Batch | 13 |
| 2018-04-05 20:53:01.777435+00:00 | 22s508µs797ns | stat summary for tableReader input | [client=[::1]:64047,user=root,n1,TableReader] | | table reader | 11 |
| | | | | | | |
| | | rows read 7 | | | | |
... rest of trace elided ...
```
And then through jaeger:
<img width="1427" alt="screen shot 2018-04-05 at 4 53 55 pm" src="https://user-images.githubusercontent.com/10560359/38392141-740ae6d8-38f4-11e8-9406-2c167142eba6.png">
Note that we can't currently run `SHOW TRACE FOR <QUERY>` in distsql mode, which I hope to fix soon (#16562)
Any suggestions to improve the output welcome.
cc @knz @jordanlewis
24715: opt: Fix flaky test r=andy-kimball a=andy-kimball
Update the TestNoMallocs to use the SucceedsSoon method in order
to repeatedly retry until zero memory allocations are detected.
Release note: None
24752: cli: Fix number of args expected by `debug gossip-values` r=a-robinson a=a-robinson
Fixes #24751
Release note: None
It looks like the problem is that my original usage string (`gossip-values <directory>`) was badly copied and pasted from other debug commands, and then assumed necessary by #24399 when in reality no positional argument is used by the command.
Co-authored-by: Alfonso Subiotto Marqués <[email protected]>
Co-authored-by: Andrew Kimball <[email protected]>
Co-authored-by: Alex Robinson <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Positional argument validation was added to spf13/cobra in
spf13/cobra#284, but our cli still
relies on custom validation in each of the "run" functions.
This validation is unstandardized and overly permissive in
certain cases. This change fixes this by replacing our custom
logic with validation expressed using Cobra's
PositionalArgsvalidators.
Release note: None