Skip to content

Commit c08d6e0

Browse files
stephenfinstephenfin
committed
parseactions: Use ArgumentError, not ArgumentTypeError
If you use the former, you get a pretty error message when there's a failure. If you use the latter, you get an ugly traceback when used with the '--debug' flag. Without this change: $ openstack flavor create ... --property '' foo ... Traceback (most recent call last): File "/tmp/venv/lib/python3.11/site-packages/cliff/app.py", line 402, in run_subcommand parsed_args = cmd_parser.parse_args(sub_argv) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.11/argparse.py", line 1862, in parse_args args, argv = self.parse_known_args(args, namespace) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.11/argparse.py", line 1895, in parse_known_args namespace, args = self._parse_known_args(args, namespace) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.11/argparse.py", line 2107, in _parse_known_args start_index = consume_optional(start_index) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib64/python3.11/argparse.py", line 2047, in consume_optional take_action(action, args, option_string) File "/usr/lib64/python3.11/argparse.py", line 1971, in take_action action(self, namespace, argument_values, option_string) File "/tmp/venv/lib/python3.11/site-packages/osc_lib/cli/parseractions.py", line 45, in __call__ raise argparse.ArgumentTypeError(msg % str(values)) argparse.ArgumentTypeError: Expected 'key=value' type, but got: clean_up CreateFlavor: Expected 'key=value' type, but got: With this change: $ openstack flavor create ... --property '' foo ... usage: openstack flavor create [-h] [-f {json,shell,table,value,yaml}] [-c COLUMN] [--noindent] [--prefix PREFIX] [--max-width <integer>] [--fit-width] [--print-empty] [--id <id>] [--ram <size-mb>] [--disk <size-gb>] [--ephemeral <size-gb>] [--swap <size-mb>] [--vcpus <vcpus>] [--rxtx-factor <factor>] [--public | --private] [--property <key=value>] [--project <project>] [--description <description>] [--project-domain <project-domain>] <flavor-name> openstack flavor create: error: argument --property: Expected 'key=value' type, but got: clean_up CreateFlavor: Change-Id: I9e78b35ad9d016d7a33655141ec579397c5344c0 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
1 parent 3057997 commit c08d6e0

10 files changed

Lines changed: 133 additions & 98 deletions

File tree

‎openstackclient/compute/v2/server.py‎

Lines changed: 54 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -841,7 +841,7 @@ def __call__(self, parser, namespace, values, option_string=None):
841841
"Invalid argument %s; characters ',' and '=' are not "
842842
"allowed"
843843
)
844-
raise argparse.ArgumentTypeError(msg % values)
844+
raise argparse.ArgumentError(self, msg % values)
845845

846846
values = '='.join([self.key, values])
847847
else:
@@ -869,7 +869,7 @@ def __call__(self, parser, namespace, values, option_string=None):
869869
"'net-id=net-uuid,port-id=port-uuid,v4-fixed-ip=ip-addr,"
870870
"v6-fixed-ip=ip-addr,tag=tag'"
871871
)
872-
raise argparse.ArgumentTypeError(msg % values)
872+
raise argparse.ArgumentError(self, msg % values)
873873

874874
info[k] = v
875875

@@ -878,7 +878,7 @@ def __call__(self, parser, namespace, values, option_string=None):
878878
'Invalid argument %s; either network or port should be '
879879
'specified but not both'
880880
)
881-
raise argparse.ArgumentTypeError(msg % values)
881+
raise argparse.ArgumenteError(self, msg % values)
882882

883883
getattr(namespace, self.dest).append(info)
884884

@@ -896,7 +896,7 @@ def __call__(self, parser, namespace, values, option_string=None):
896896
"Invalid argument %s; argument must be of form "
897897
"'dev-name=id[:type[:size[:delete-on-terminate]]]'"
898898
)
899-
raise argparse.ArgumentTypeError(msg % values)
899+
raise argparse.ArgumentError(self, msg % values)
900900

901901
mapping = {
902902
'device_name': dev_name,
@@ -913,7 +913,7 @@ def __call__(self, parser, namespace, values, option_string=None):
913913
"Invalid argument %s; 'type' must be one of: volume, "
914914
"snapshot, image"
915915
)
916-
raise argparse.ArgumentTypeError(msg % values)
916+
raise argparse.ArgumentError(self, msg % values)
917917

918918
mapping['source_type'] = dev_map[1]
919919

@@ -966,12 +966,13 @@ def validate_keys(self, keys):
966966
"Invalid keys %(invalid_keys)s specified.\n"
967967
"Valid keys are: %(valid_keys)s"
968968
)
969-
raise argparse.ArgumentTypeError(
969+
raise argparse.ArgumentError(
970+
self,
970971
msg
971972
% {
972973
'invalid_keys': ', '.join(invalid_keys),
973974
'valid_keys': ', '.join(valid_keys),
974-
}
975+
},
975976
)
976977

977978
missing_keys = [k for k in self.required_keys if k not in keys]
@@ -980,12 +981,13 @@ def validate_keys(self, keys):
980981
"Missing required keys %(missing_keys)s.\n"
981982
"Required keys are: %(required_keys)s"
982983
)
983-
raise argparse.ArgumentTypeError(
984+
raise argparse.ArgumentError(
985+
self,
984986
msg
985987
% {
986988
'missing_keys': ', '.join(missing_keys),
987989
'required_keys': ', '.join(self.required_keys),
988-
}
990+
},
989991
)
990992

991993
def __call__(self, parser, namespace, values, option_string=None):
@@ -2086,11 +2088,48 @@ def _show_progress(progress):
20862088
raise exceptions.CommandError(msg)
20872089

20882090

2089-
def percent_type(x):
2090-
x = int(x)
2091-
if not 0 < x <= 100:
2092-
raise argparse.ArgumentTypeError("Must be between 0 and 100")
2093-
return x
2091+
class PercentAction(argparse.Action):
2092+
def __init__(
2093+
self,
2094+
option_strings,
2095+
dest,
2096+
nargs=None,
2097+
const=None,
2098+
default=None,
2099+
type=None,
2100+
choices=None,
2101+
required=False,
2102+
help=None,
2103+
metavar=None,
2104+
):
2105+
if nargs == 0:
2106+
raise ValueError(
2107+
'nargs for store actions must be != 0; if you '
2108+
'have nothing to store, actions such as store '
2109+
'true or store const may be more appropriate'
2110+
)
2111+
2112+
if const is not None:
2113+
raise ValueError('const does not make sense for PercentAction')
2114+
2115+
super().__init__(
2116+
option_strings=option_strings,
2117+
dest=dest,
2118+
nargs=nargs,
2119+
const=const,
2120+
default=default,
2121+
type=type,
2122+
choices=choices,
2123+
required=required,
2124+
help=help,
2125+
metavar=metavar,
2126+
)
2127+
2128+
def __call__(self, parser, namespace, values, option_string=None):
2129+
x = int(values)
2130+
if not 0 < x <= 100:
2131+
raise argparse.ArgumentError(self, "Must be between 0 and 100")
2132+
setattr(namespace, self.dest, x)
20942133

20952134

20962135
class ListServer(command.Lister):
@@ -2243,7 +2282,7 @@ def get_parser(self, prog_name):
22432282
)
22442283
parser.add_argument(
22452284
'--progress',
2246-
type=percent_type,
2285+
action=PercentAction,
22472286
default=None,
22482287
help=_(
22492288
'Search by progress value (%%) '

‎openstackclient/network/v2/port.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ def __call__(self, parser, namespace, values, option_string=None):
124124
"%(option)s, but encountered JSON parsing error: "
125125
"%(error)s"
126126
) % {"option": option_string, "error": e}
127-
raise argparse.ArgumentTypeError(msg)
127+
raise argparse.ArgumentError(self, msg)
128128

129129

130130
def _get_attrs(client_manager, parsed_args):
@@ -409,7 +409,7 @@ def _validate_port_hints(hints):
409409
{'openvswitch': {'other_config': {'tx-steering': 'hash'}}},
410410
):
411411
msg = _("Invalid value to --hints, see --help for valid values.")
412-
raise argparse.ArgumentTypeError(msg)
412+
raise exceptions.CommandError(msg)
413413

414414

415415
# When we have multiple hints, we'll need to refactor this to expand aliases

‎openstackclient/tests/unit/compute/v2/test_server.py‎

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,7 @@
1111
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
1212
# License for the specific language governing permissions and limitations
1313
# under the License.
14-
#
15-
import argparse
14+
1615
import collections
1716
import copy
1817
import getpass
@@ -33,11 +32,11 @@
3332
from openstackclient.tests.unit.compute.v2 import fakes as compute_fakes
3433
from openstackclient.tests.unit.image.v2 import fakes as image_fakes
3534
from openstackclient.tests.unit.network.v2 import fakes as network_fakes
36-
from openstackclient.tests.unit import utils
35+
from openstackclient.tests.unit import utils as test_utils
3736
from openstackclient.tests.unit.volume.v3 import fakes as volume_fakes
3837

3938

40-
class TestPowerStateColumn(utils.TestCase):
39+
class TestPowerStateColumn(test_utils.TestCase):
4140
def test_human_readable(self):
4241
self.assertEqual(
4342
'NOSTATE', server.PowerStateColumn(0x00).human_readable()
@@ -1128,7 +1127,7 @@ def test_server_add_volume_with_disable_and_enable_delete_on_termination(
11281127
('disable_delete_on_termination', True),
11291128
]
11301129
ex = self.assertRaises(
1131-
utils.ParserException,
1130+
test_utils.ParserException,
11321131
self.check_parser,
11331132
self.cmd,
11341133
arglist,
@@ -1373,7 +1372,7 @@ def test_server_create_no_options(self):
13731372
]
13741373

13751374
self.assertRaises(
1376-
utils.ParserException,
1375+
test_utils.ParserException,
13771376
self.check_parser,
13781377
self.cmd,
13791378
arglist,
@@ -2193,7 +2192,7 @@ def test_server_create_with_invalid_network_options(self):
21932192
self.new_server.name,
21942193
]
21952194
self.assertRaises(
2196-
argparse.ArgumentTypeError,
2195+
test_utils.ParserException,
21972196
self.check_parser,
21982197
self.cmd,
21992198
arglist,
@@ -2212,7 +2211,7 @@ def test_server_create_with_invalid_network_key(self):
22122211
self.new_server.name,
22132212
]
22142213
self.assertRaises(
2215-
argparse.ArgumentTypeError,
2214+
test_utils.ParserException,
22162215
self.check_parser,
22172216
self.cmd,
22182217
arglist,
@@ -2231,7 +2230,7 @@ def test_server_create_with_empty_network_key_value(self):
22312230
self.new_server.name,
22322231
]
22332232
self.assertRaises(
2234-
argparse.ArgumentTypeError,
2233+
test_utils.ParserException,
22352234
self.check_parser,
22362235
self.cmd,
22372236
arglist,
@@ -2250,7 +2249,7 @@ def test_server_create_with_only_network_key(self):
22502249
self.new_server.name,
22512250
]
22522251
self.assertRaises(
2253-
argparse.ArgumentTypeError,
2252+
test_utils.ParserException,
22542253
self.check_parser,
22552254
self.cmd,
22562255
arglist,
@@ -3302,7 +3301,7 @@ def test_server_create_with_block_device_mapping_invalid_format(self):
33023301
self.new_server.name,
33033302
]
33043303
self.assertRaises(
3305-
argparse.ArgumentTypeError,
3304+
test_utils.ParserException,
33063305
self.check_parser,
33073306
self.cmd,
33083307
arglist,
@@ -3320,7 +3319,7 @@ def test_server_create_with_block_device_mapping_invalid_format(self):
33203319
self.new_server.name,
33213320
]
33223321
self.assertRaises(
3323-
argparse.ArgumentTypeError,
3322+
test_utils.ParserException,
33243323
self.check_parser,
33253324
self.cmd,
33263325
arglist,
@@ -3338,7 +3337,7 @@ def test_server_create_with_block_device_mapping_no_uuid(self):
33383337
self.new_server.name,
33393338
]
33403339
self.assertRaises(
3341-
argparse.ArgumentTypeError,
3340+
test_utils.ParserException,
33423341
self.check_parser,
33433342
self.cmd,
33443343
arglist,
@@ -3759,7 +3758,7 @@ def test_server_create_with_ephemeral_missing_key(self):
37593758
self.new_server.name,
37603759
]
37613760
self.assertRaises(
3762-
argparse.ArgumentTypeError,
3761+
test_utils.ParserException,
37633762
self.check_parser,
37643763
self.cmd,
37653764
arglist,
@@ -3777,7 +3776,7 @@ def test_server_create_with_ephemeral_invalid_key(self):
37773776
self.new_server.name,
37783777
]
37793778
self.assertRaises(
3780-
argparse.ArgumentTypeError,
3779+
test_utils.ParserException,
37813780
self.check_parser,
37823781
self.cmd,
37833782
arglist,
@@ -3796,7 +3795,7 @@ def test_server_create_invalid_hint(self):
37963795
self.new_server.name,
37973796
]
37983797
self.assertRaises(
3799-
argparse.ArgumentTypeError,
3798+
test_utils.ParserException,
38003799
self.check_parser,
38013800
self.cmd,
38023801
arglist,
@@ -3814,7 +3813,7 @@ def test_server_create_invalid_hint(self):
38143813
self.new_server.name,
38153814
]
38163815
self.assertRaises(
3817-
argparse.ArgumentTypeError,
3816+
test_utils.ParserException,
38183817
self.check_parser,
38193818
self.cmd,
38203819
arglist,
@@ -5187,7 +5186,7 @@ def test_server_list_with_progress_invalid(self):
51875186
]
51885187

51895188
self.assertRaises(
5190-
utils.ParserException,
5189+
test_utils.ParserException,
51915190
self.check_parser,
51925191
self.cmd,
51935192
arglist,
@@ -5446,7 +5445,7 @@ def test_server_list_with_locked_and_unlocked(self):
54465445
verifylist = [('locked', True), ('unlocked', True)]
54475446

54485447
ex = self.assertRaises(
5449-
utils.ParserException,
5448+
test_utils.ParserException,
54505449
self.check_parser,
54515450
self.cmd,
54525451
arglist,
@@ -6548,7 +6547,7 @@ def test_rebuild_with_keypair_name_and_unset(self):
65486547
('key_name', self.server.key_name),
65496548
]
65506549
self.assertRaises(
6551-
utils.ParserException,
6550+
test_utils.ParserException,
65526551
self.check_parser,
65536552
self.cmd,
65546553
arglist,
@@ -6652,7 +6651,11 @@ def test_rebuild_with_user_data_and_unset(self):
66526651
'--no-user-data',
66536652
]
66546653
self.assertRaises(
6655-
utils.ParserException, self.check_parser, self.cmd, arglist, None
6654+
test_utils.ParserException,
6655+
self.check_parser,
6656+
self.cmd,
6657+
arglist,
6658+
None,
66566659
)
66576660

66586661
def test_rebuild_with_trusted_image_cert(self):
@@ -7847,7 +7850,7 @@ def test_server_set_with_invalid_state(self):
78477850
('server', 'foo_vm'),
78487851
]
78497852
self.assertRaises(
7850-
utils.ParserException,
7853+
test_utils.ParserException,
78517854
self.check_parser,
78527855
self.cmd,
78537856
arglist,
@@ -8251,7 +8254,7 @@ def test_show_no_options(self):
82518254
verifylist = []
82528255

82538256
self.assertRaises(
8254-
utils.ParserException,
8257+
test_utils.ParserException,
82558258
self.check_parser,
82568259
self.cmd,
82578260
arglist,
@@ -8915,7 +8918,7 @@ def test_unshelve_with_no_az_and_az_conflict(self):
89158918
]
89168919

89178920
ex = self.assertRaises(
8918-
utils.ParserException,
8921+
test_utils.ParserException,
89198922
self.check_parser,
89208923
self.cmd,
89218924
arglist,

0 commit comments

Comments
 (0)