Make the CLI follow the nextKey for app and users #762
Conversation
XVincentX
force-pushed
the
bugfix/app-user-list-follow-pointer
branch
2 times, most recently
from
1068e73
to
3089255
Compare
Codecov Report
@@ Coverage Diff @@
## master #762 +/- ##
==========================================
- Coverage 89.13% 89.11% -0.03%
==========================================
Files 131 132 +1
Lines 3591 3574 -17
==========================================
- Hits 3201 3185 -16
+ Misses 390 389 -1
Continue to review full report at Codecov.
|
XVincentX
force-pushed
the
bugfix/app-user-list-follow-pointer
branch
from
cb0e7dc
to
c7bab66
Compare
There was a problem hiding this comment.
Just a few changes suggested for descriptions in the CLI help text.
All in all, this whole thing is a bit weird. We paginate the results, but we don't have any parameters like --start-key or --limit. I wouldn't hold up this PR for that, but it's worth mentioning.
bin/generators/apps/list.js
Outdated
| @@ -10,12 +10,13 @@ module.exports = class extends eg.Generator { | |||
| builder: yargs => | |||
| yargs | |||
| .usage(`Usage: $0 ${process.argv[2]} list [options]`) | |||
| .boolean('a').alias('a', 'all').describe('a', 'Show all the elements instead of stopping to the fist page') | |||
There was a problem hiding this comment.
Suggest change in description:
List all apps
bin/generators/users/list.js
Outdated
| @@ -10,11 +10,12 @@ module.exports = class extends eg.Generator { | |||
| builder: yargs => | |||
| yargs | |||
| .usage(`Usage: $0 ${process.argv[2]} list [options]`) | |||
| .boolean('a').alias('a', 'all').describe('a', 'Show all the elements instead of stopping to the fist page') | |||
There was a problem hiding this comment.
Suggest change in description:
List all users.
XVincentX
force-pushed
the
bugfix/app-user-list-follow-pointer
branch
from
c7bab66
to
f42d8d1
Compare
|
I agree with your points — the CLI is way behind what the API offers. I do hope we'll have some time to modernize the CLI and catch up with the API features. |
|
@kevinswiber I addressed the comments. If there's anything else here, we could bring this in. |
|
@DrMegavolt @kevinswiber Ping |
| list (params = {}) { | ||
| let results = []; | ||
|
|
||
| const fetchNext = (res) => { |
There was a problem hiding this comment.
why do you need this page by page loader if you can request all users\apps at once
by passing count parameter as something big like Number.MAX_SAFE_INTEGER
There was a problem hiding this comment.
Using a large number in theory guarantees that I get all the users — in practice we can't never be completely sure. We do not know how large the data base is.
Also — if I recall correctly the count is about the number of fields, not the number of records.
I do not feel really comfortable in making a single call with a large number and cross the fingers that all will work accordingly. If it's a paginated resource, we should handle pagination.
@kevinswiber Thoughts?
XVincentX
force-pushed
the
bugfix/app-user-list-follow-pointer
branch
from
f42d8d1
to
b1f5a70
Compare
…user-list-follow-pointer Make the CLI follow the nextKey for app and users
This PR introduces an additional flag for
eg apps listandeg users listcalled-aor--all. When this is provided, the cli will navigate through the paged results from Redis in order to show all the users and not just the first page.Connect #749
Closes #749
P.S: The CLI and the tests for the CLI have a lot of copy/paste code that could be removed and reused across the whole part of the system. However doing this now would be a significant amount of time. I still want to do that, but maybe in another iteration.