-
Notifications
You must be signed in to change notification settings - Fork 729
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
pdctl: support filter json output by calling external jq #1126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a reference of related documentation PR.
@@ -42,7 +43,7 @@ type regionInfo struct { | |||
// NewRegionCommand return a region subcommand of rootCmd | |||
func NewRegionCommand() *cobra.Command { | |||
r := &cobra.Command{ | |||
Use: "region <region_id>", | |||
Use: `region <region_id> [-jq="<query string>"]`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only support some commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Only support APIs that query region/store list. The results returned by other APIs are simple and easy to see with the naked eye.
@@ -82,6 +84,11 @@ func showRegionCommandFunc(cmd *cobra.Command, args []string) { | |||
fmt.Printf("Failed to get region: %s\n", err) | |||
return | |||
} | |||
if flag := cmd.Flag("jq"); flag != nil && flag.Value.String() != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we provide a Print interface for jq or standard output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible but I don't think it's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seem better to extract to a function like isJQUsed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to over-abstract, --jq
is not a general parameter. these two commands just happen to both support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* pdctl: support filter json output by calling external jq * update vendor * support jq for store command
What have you changed? (required)
shellwords
to split command line stringsjq
be installed in the system. We may want to include ajq
binary in our release package.What are the type of the changes (required)?
How has this PR been tested (required)?
Does this PR affect documentation (docs/docs-cn) update? (optional)
Yes. See pingcap/docs-cn#790