Add "Note" in the CLI if API has default query "limit=50" and update rule endpoint help #3514
Conversation
LGTM, but we should also do the change @armab suggested aka printing out "there are more results" when using CLI in user-friendly (table) mode (we don't want to print that when using --json or --yaml flag). |
resource.get_plural_display_name().lower())) | ||
self.parser.add_argument('--iftt', action='store_true', | ||
help='Show trigger and action in display list.') | ||
self.parser.add_argument('-p', '--pack', type=str, |
Kami
Jun 26, 2017
Member
Perhaps we should add changelog entry for this change.
Perhaps we should add changelog entry for this change.
humblearner
Jun 27, 2017
Author
Contributor
Yes, once all the changes are ok, I'll update the changelog.
Yes, once all the changes are ok, I'll update the changelog.
@@ -34,20 +34,26 @@ def __init__(self, description, app, subparsers, parent_parser=None): | |||
self.commands['disable'] = RuleDisableCommand(self.resource, self.app, self.subparsers) | |||
|
|||
|
|||
class RuleListCommand(resource.ContentPackResourceListCommand): | |||
class RuleListCommand(resource.ResourceTableCommand): |
Kami
Jun 26, 2017
Member
Just curious why the inherit class change was needed.
Just curious why the inherit class change was needed.
True, without explicit note that only first 50 results are showed up, - it's not user friendly and doesn't solve the confusion. |
Besides rules, the default limit=50 is set for: |
Yeah, that note works for me, but maybe we could also make it more dynamic and only display the note if there are in fact more results. We could do that by utilizing X- headers returned by the API, bit it could also make it a bit more complex and require more refactoring, because headers and raw response objects are not available in those methods right now. |
attributes=args.attr, widths=args.width, | ||
json=args.json, yaml=args.yaml, | ||
attribute_transform_functions=self.attribute_transform_functions) | ||
if args.json or args.yaml: |
Kami
Jun 28, 2017
Member
It looks like this is repeated in quite some places so eventually it will need some refactor, but it should be fine for now.
It looks like this is repeated in quite some places so eventually it will need some refactor, but it should be fine for now.
@@ -274,11 +274,14 @@ def query(self, **kwargs): | |||
self.handle_error(response) | |||
items = response.json() | |||
instances = [self.resource.deserialize(item) for item in items] | |||
return instances | |||
if 'X-Total-Count' in response.headers: |
Kami
Jun 28, 2017
•
Member
It looks like query is used in quite some places, were all the affected places updated?
https://gist.github.com/Kami/365f58a69f90594ce970dcd2c2069a8e
It looks like query is used in quite some places, were all the affected places updated?
https://gist.github.com/Kami/365f58a69f90594ce970dcd2c2069a8e
humblearner
Jun 28, 2017
Author
Contributor
Yes, I have updated all the places, where query was called. However, unit tests are not happy, as we are mocking the response w/o headers.
Yes, I have updated all the places, where query was called. However, unit tests are not happy, as we are mocking the response w/o headers.
json=args.json, | ||
yaml=args.yaml, | ||
attribute_transform_functions=self.attribute_transform_functions) | ||
if args.last >= self.default_limit and int(count) > args.last: |
Kami
Jun 28, 2017
Member
It looks like query
method returns Noneif response doesn't contain X-Total header so this
int(count)` would fail.
So probably need to do something along the lines of and count is not None and int(count)
...
It looks like query
method returns Noneif response doesn't contain X-Total header so this
int(count)` would fail.
So probably need to do something along the lines of and count is not None and int(count)
...
humblearner
Jun 28, 2017
Author
Contributor
Agree, however, is it safe to assume if args.last
is present X-Total-Count
will be present in the header?
Agree, however, is it safe to assume if args.last
is present X-Total-Count
will be present in the header?
Kami
Jun 28, 2017
Member
I believe so, but would need to confirm.
And perhaps better to be on the safe side :)
I believe so, but would need to confirm.
And perhaps better to be on the safe side :)
note = PrettyTable([""]) | ||
note.header = False | ||
note.add_row([message]) | ||
print note |
Kami
Jun 28, 2017
Member
Minor API consistency thing - all other formatter class methods return a value instead of printing it so it would be better for this one to do that as well.
Minor API consistency thing - all other formatter class methods return a value instead of printing it so it would be better for this one to do that as well.
note = PrettyTable([""]) | ||
note.header = False | ||
note.add_row([message]) | ||
return sys.stderr.write(str(note)) |
self.print_output(reversed(instances), table.MultiColumnTable, | ||
attributes=args.attr, widths=args.width, | ||
attribute_transform_functions=self.attribute_transform_functions) | ||
if args.last >= self.default_limit and count and int(count) > args.last: |
Kami
Jun 29, 2017
Member
It looks like it doesn't correctly handle a scenario where you pass in -n 1
for example but there are more results.
So this logic needs to be changed to handle that scenario as well.
It looks like it doesn't correctly handle a scenario where you pass in -n 1
for example but there are more results.
So this logic needs to be changed to handle that scenario as well.
humblearner
Jun 29, 2017
Author
Contributor
Hmm, it is working for me. If n > default limit then show the note, otherwise, just show the result.
$ st2 rule-enforcement list -n 1
+--------------------------+----------------------+----------------------+----------------------+----------------------+
| id | rule.ref | trigger_instance_id | execution_id | enforced_at |
+--------------------------+----------------------+----------------------+----------------------+----------------------+
| 5952ab31909a5027b65c037b | st2ci.st2_pkg_test_a | 5952ab30909a5027b65c | 5952ab30909a5027b65c | 2017-06-27T19:00:00. |
| | nd_promote_unstable_ | 036e | 037a | 858196Z |
| | u16 | | | |
+--------------------------+----------------------+----------------------+----------------------+----------------------+
$ st2 execution list -n 1
+--------------------------+------------+--------------+------------------------+------------------+---------------+
| id | action.ref | context.user | status | start_timestamp | end_timestamp |
+--------------------------+------------+--------------+------------------------+------------------+---------------+
| 595467b3909a5015dab1c8b9 | core.local | stanley | succeeded (1s elapsed) | Thu, 29 Jun 2017 | Thu, 29 Jun |
| | | | | 02:36:35 UTC | 2017 02:36:36 |
| | | | | | UTC |
+--------------------------+------------+--------------+------------------------+------------------+---------------+
$ st2 trigger-instance list -n 1
+--------------------------+---------------------------+---------------------------+-----------+
| id | trigger | occurrence_time | status |
+--------------------------+---------------------------+---------------------------+-----------+
| 59546ab5909a5045fc0d0882 | core.st2.key_value_pair.c | Thu, 29 Jun 2017 02:49:25 | processed |
| | reate | UTC | |
+--------------------------+---------------------------+---------------------------+-----------+
Hmm, it is working for me. If n > default limit then show the note, otherwise, just show the result.
$ st2 rule-enforcement list -n 1
+--------------------------+----------------------+----------------------+----------------------+----------------------+
| id | rule.ref | trigger_instance_id | execution_id | enforced_at |
+--------------------------+----------------------+----------------------+----------------------+----------------------+
| 5952ab31909a5027b65c037b | st2ci.st2_pkg_test_a | 5952ab30909a5027b65c | 5952ab30909a5027b65c | 2017-06-27T19:00:00. |
| | nd_promote_unstable_ | 036e | 037a | 858196Z |
| | u16 | | | |
+--------------------------+----------------------+----------------------+----------------------+----------------------+
$ st2 execution list -n 1
+--------------------------+------------+--------------+------------------------+------------------+---------------+
| id | action.ref | context.user | status | start_timestamp | end_timestamp |
+--------------------------+------------+--------------+------------------------+------------------+---------------+
| 595467b3909a5015dab1c8b9 | core.local | stanley | succeeded (1s elapsed) | Thu, 29 Jun 2017 | Thu, 29 Jun |
| | | | | 02:36:35 UTC | 2017 02:36:36 |
| | | | | | UTC |
+--------------------------+------------+--------------+------------------------+------------------+---------------+
$ st2 trigger-instance list -n 1
+--------------------------+---------------------------+---------------------------+-----------+
| id | trigger | occurrence_time | status |
+--------------------------+---------------------------+---------------------------+-----------+
| 59546ab5909a5045fc0d0882 | core.st2.key_value_pair.c | Thu, 29 Jun 2017 02:49:25 | processed |
| | reate | UTC | |
+--------------------------+---------------------------+---------------------------+-----------+
``execution``, ``rule-enforcment``, ``trace`` and ``trigger-instance`` as they are not displayed. | ||
Default limit is 50. (improvement) | ||
|
||
Reported by Eugen C. #3488 |
armab
Jul 1, 2017
•
Member
Just #3514 #3488
is enough there, without "Reported by".
I think it's good idea to include "reported by" only if there was an external contribution.
Just #3514 #3488
is enough there, without "Reported by".
I think it's good idea to include "reported by" only if there was an external contribution.
value = transform_function(value=value) | ||
value = strutil.unescape(value) | ||
values.append(value) | ||
table.add_row(values) | ||
|
||
# width for the note | ||
global table_width |
Kami
Jul 3, 2017
Member
I will test it and go ahead and merge it if everything look OK so it will end up in v2.3.1, but we really need to refactor and fix this global variable thing since it's not robust and could cause issues.
I will test it and go ahead and merge it if everything look OK so it will end up in v2.3.1, but we really need to refactor and fix this global variable thing since it's not robust and could cause issues.
I'll go ahead and merge this so we can start testing changes for v2.3.1 release, but please look into getting rid of the global variable as soon as you get a chance. |
@humblearner It would be great if you can also work on automated robot tests for this CLI functionality - so testing all the common scenarios + edge cases. CLI is one of the places where we lack tests and regressions are easy to introduce so we should invest some time in more automated tests. |
result. In StackStorm v2.3.0, breaking client change has been introduced for query method as part of StackStorm/st2#3514.
Changes:
CLI Updated:
Functionality:
Addresses: #3488