Refactor/click #107
Refactor/click #107
Conversation
| cls=AdvancedQueryAndSavedSearchIncompatible, | ||
| callback=is_in_filter(Actor), | ||
| help="Filter alerts by including the given actor(s) who triggered the alert. " | ||
| "Args must match actor username exactly.", |
There was a problem hiding this comment.
should this just be actor instead of actor username?
There was a problem hiding this comment.
I'm trying to make clear that when searching for actor you're searching usernames. Although I suppose most people using the CLI to search for alerts will already be familiar with the console and probably don't need that extra help?
| def is_in_filter(filter_cls): | ||
| def callback(ctx, param, arg): | ||
| if arg: | ||
| ctx.obj.search_filters.append(filter_cls.is_in(arg)) |
There was a problem hiding this comment.
do click params that are multiple=True always come through as a list (even if only one item is enterred)?
There was a problem hiding this comment.
yes, and options that allow multiple values have to be called separately, so --option a --option b, you can't do --option a b
There was a problem hiding this comment.
that will be an important breaking change to note
src/code42cli/cmds/alert_rules.py
Outdated
| selected_rule = _get_rule_metadata(state.sdk, rule_id) | ||
| rule_detail = None | ||
| if selected_rule: | ||
| rule_type = selected_rule[0][u"type"] |
There was a problem hiding this comment.
a private _get_rule_type method might tighten this up a bit
| @click.argument("checkpoint-name") | ||
| @sdk_options | ||
| def clear_checkpoint(state, checkpoint_name): | ||
| """Remove the saved alert checkpoint from '--use-checkpoint/-c' mode.""" |
There was a problem hiding this comment.
if --use-checkpoint has changed to --checkpoint-name, this comment should change, ideally we should update the relevant variable to checkpoint_name from use_checkpoint.
There was a problem hiding this comment.
The option to use checkpoint is still --use-checkpoint, this is just the arg name that you pass in when deleting a checkpoint. So the help looks like this:
Usage: code42 alerts clear-checkpoint [OPTIONS] CHECKPOINT_NAME
Remove the saved alert checkpoint from '--use-checkpoint/-c' mode.
…nto _get_rule_type_func
| return super().invoke(ctx) | ||
|
|
||
| except click.UsageError as err: | ||
| self._suggest_cmd(err) |
There was a problem hiding this comment.
nice, i saw that click didnt have suggestions built in and was wondering how that would be handled, thanks for this.
| ) | ||
| if not suggested_commands: | ||
| raise usage_err | ||
| usage_err.message = "No such command '{}'. Did you mean {}?".format( |
There was a problem hiding this comment.
how will this be presented? Ideally I'm thinking one suggestion per line, like:
No such command 'blah'. Did you mean one of the following?
blag
blap
blar
There was a problem hiding this comment.
This is what it looks like when there are multiple options.
PS C:\Users\tabra\PycharmProjects\code42cli> code42 employee
Usage: code42 [OPTIONS] COMMAND [ARGS]...
Try 'code42 -h' for help.
Error: No such command 'employee'. Did you mean high-risk-employee or departing-employee?
I would guess there's very rarely going to be a time when more than two are presented.
There was a problem hiding this comment.
That's a fair point, this isn't exactly core functionality, we can always change this if needed later
|
@timabrmsn it looks like the small legal hold refactor somehow broke a test but otherwise the format of the suggestions is the one last thing I saw and everything looks good. We should come up with a plan to break up the functionality and divide up testing. |
Draft PR for mostly done refactor so people can start getting eyes on it.
Still need to bring in @alanag13's checkpoint and @kiran-chaudhary's smart search changes. Plus refactoring tests.