--version-check default should be explicitly "off"
Bug #1069951 reported by
Daniel Nichter
This bug affects 1 person
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Percona Toolkit moved to https://jira.percona.com/projects/PT |
Fix Released
|
High
|
Brian Fraser |
Bug Description
[12:30pm] Daniel: brian|lunch: i just noticed optional_value. it needs to be reverted. optional values are not something we want to do because it confuses the issue. there only 3 types of options: off unless specified (e.g. --version), negatable (on unless negated, e.g. --no-check-slave for --[no]check-slave), then options with values. an optional value crosses the line between the previous three and is not inline with the rest of the tools' options.
[12:30pm] Daniel: --version-check just needs to be type: string; default: https then docu possible values are https, and http
Related branches
lp:~percona-toolkit-dev/percona-toolkit/OptionParser-remove-optional_value
- Daniel Nichter: Approve
-
Diff: 4383 lines (+1264/-677)31 files modifiedbin/pt-archiver (+62/-29)
bin/pt-config-diff (+58/-28)
bin/pt-deadlock-logger (+62/-29)
bin/pt-diskstats (+57/-27)
bin/pt-duplicate-key-checker (+59/-29)
bin/pt-fifo-split (+5/-8)
bin/pt-find (+59/-28)
bin/pt-fingerprint (+5/-8)
bin/pt-fk-error-logger (+62/-29)
bin/pt-heartbeat (+61/-27)
bin/pt-index-usage (+62/-29)
bin/pt-kill (+59/-28)
bin/pt-log-player (+5/-8)
bin/pt-online-schema-change (+60/-29)
bin/pt-query-advisor (+62/-29)
bin/pt-query-digest (+63/-30)
bin/pt-show-grants (+5/-8)
bin/pt-slave-delay (+62/-29)
bin/pt-slave-find (+5/-8)
bin/pt-slave-restart (+59/-28)
bin/pt-table-checksum (+62/-29)
bin/pt-table-sync (+61/-27)
bin/pt-tcp-model (+5/-8)
bin/pt-trend (+5/-8)
bin/pt-upgrade (+59/-28)
bin/pt-variable-advisor (+59/-28)
lib/OptionParser.pm (+5/-8)
lib/Pingback.pm (+42/-11)
t/lib/OptionParser.t (+3/-62)
t/lib/Pingback.t (+18/-0)
t/pt-query-digest/version_check.t (+13/-0)
Changed in percona-toolkit: | |
status: | Confirmed → In Progress |
Changed in percona-toolkit: | |
status: | In Progress → Fix Committed |
Changed in percona-toolkit: | |
milestone: | none → 2.1.6 |
Changed in percona-toolkit: | |
status: | Fix Committed → Fix Released |
To post a comment you must log in.
Here's a solution:
type: string
Possible values: auto, http, https, off*
Right now, with no default value, the feature is effectively off because if ( $o->get( 'version- check') ) will be false. But in the future we're going to change that default to either auto or https. Auto means "try https, if that fails, try http, else give up." Off means off, which will allow users to --version-check off when we set the default to auto or https.
* I thought of maybe "disable(d)" instead of "off", but "off" is a lot shorter; also "none" but that doesn't seem to make as much sense as "off".