Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvs: add flux kvs ls subcommand #1172

Merged
merged 4 commits into from Aug 30, 2017

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Aug 29, 2017

This adds a flux kvs ls subcommand that behaves similar to ls(1), as discussed in #1158.

Usage: flux-kvs ls [-R] [-d] [-w COLS] [-1] [-W] [-C] [key...]
List directory
  -1, --1                Force one entry per line
  -C, --columns          List entries by columns
  -F, --classify         Append indicator (one of .@) to entries
  -R, --recursive        List directory recursively
  -d, --directory        List directory instead of contents
  -h, --help             Display this message.
  -w, --width            Set output width to COLS.  0 means no limit
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 29, 2017

Coverage Status

Coverage increased (+0.02%) to 78.057% when pulling 6f7389d on garlick:kvs_ls into 0c87cfa on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 29, 2017

Restarted three tests; two write errors, one hydra test failure (can't really be related).

@garlick garlick force-pushed the garlick:kvs_ls branch from 6f7389d to 402dc0a Aug 29, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 29, 2017

Codecov Report

Merging #1172 into master will increase coverage by 0.09%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master    #1172      +/-   ##
==========================================
+ Coverage   77.64%   77.73%   +0.09%     
==========================================
  Files         158      158              
  Lines       29088    29265     +177     
==========================================
+ Hits        22586    22750     +164     
- Misses       6502     6515      +13
Impacted Files Coverage Δ
src/common/libkvs/kvs_dir.c 96% <100%> (+3.5%) ⬆️
src/cmd/flux-kvs.c 79.66% <86.53%> (+3.04%) ⬆️
src/common/libflux/request.c 87.17% <0%> (-2.57%) ⬇️
src/common/libutil/dirwalk.c 92.95% <0%> (-0.71%) ⬇️
src/broker/overlay.c 71.67% <0%> (-0.35%) ⬇️
src/common/libcompat/handle.c 85.41% <0%> (+0.27%) ⬆️
src/common/libflux/message.c 81.41% <0%> (+0.35%) ⬆️
src/common/libflux/handle.c 84.15% <0%> (+0.49%) ⬆️
src/common/libflux/future.c 88.61% <0%> (+0.49%) ⬆️
... and 2 more
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 29, 2017

Rebased after merge of #1171

* if -C, or 1 if not.
* N.B. A width of 0 means "unlimited", while a width of 1 effectively
* forces one entry per line since entries are never broken across lines.
*/

This comment has been minimized.

@grondo

grondo Aug 30, 2017

Contributor

To match ls should get_window_width() also check COLUMNS env var? From GNU ls(1):

‘-w cols’
‘--width=cols’
Assume the screen is cols columns wide. The default is taken from the terminal settings if possible; otherwise the environment variable COLUMNS is used if it is set; otherwise the default is 80. With a cols value of ‘0’, there is no limit on the length of the output line, and that single output line will be delimited with spaces, not tabs.

This comment has been minimized.

@garlick

garlick Aug 30, 2017

Author Member

Good catch, I did not read that carefully in the ls(1) page. Will fix.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage increased (+0.05%) to 78.068% when pulling 402dc0a on garlick:kvs_ls into 6da40cb on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 30, 2017

I know the -C option is coming from ls, but is there a use case for it in flux-kvs ls? From the cmdline help and manpage it sounds like a superfluous option. Is it meant to be used when output is not a terminal? If it has some use case a note in the manual might help.

@chu11
Copy link
Contributor

chu11 left a comment

Haven't checked out code and played with it yet, but just a few nits I saw.

@@ -116,6 +143,13 @@ static struct optparse_subcommand subcommands[] = {
0,
dir_opts
},
{ "ls",
"[-R] [-d] [-w COLS] [-1] [-W] [-C] [key...]",
"List directory",

This comment has been minimized.

@chu11

chu11 Aug 30, 2017

Contributor

the -W iss probably supposed to be -F?

This comment has been minimized.

@garlick

garlick Aug 30, 2017

Author Member

Yep, thanks.

@@ -116,6 +143,13 @@ static struct optparse_subcommand subcommands[] = {
0,
dir_opts
},
{ "ls",
"[-R] [-d] [-w COLS] [-1] [-W] [-C] [key...]",
"List directory",

This comment has been minimized.

@chu11

chu11 Aug 30, 2017

Contributor

also noticed the manpage is [-R] [-d] [-C] [-1|-C] [-w COLS] [-F], should this match up together (most notably [-1|-C]).

This comment has been minimized.

@garlick

garlick Aug 30, 2017

Author Member

Indeed.


/* List keys, arranging in columns so that they fit within 'win_width',
* and using a custom column width selected based on the longest entry name.
* The keys are popped off the list and freed we go.

This comment has been minimized.

@chu11

chu11 Aug 30, 2017

Contributor

freed we go, just "freed"? Or had another thought but didn't finish?

This comment has been minimized.

@garlick

garlick Aug 30, 2017

Author Member

Freed as we go, but I'll go with "freed".

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 30, 2017

I know the -C option is coming from ls, but is there a use case for it in flux-kvs ls? From the cmdline help and manpage it sounds like a superfluous option. Is it meant to be used when output is not a terminal? If it has some use case a note in the manual might help.

I may have misrepresented the way columns are selected by ls(1), and thought -C was needed to get columns when output is not a tty. I think we could just drop it actually.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage increased (+0.04%) to 78.054% when pulling edb8526 on garlick:kvs_ls into 6da40cb on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 30, 2017

I may have misrepresented the way columns are selected by ls(1), and thought -C was needed to get columns when output is not a tty. I think we could just drop it actually.

Ah, ok that is what I figured. I see you already removed it, but I would have been fine with keeping -C, however a note in the usage message "preserve column output when stdout is not a tty" I think would have avoided confusion. In the real ls there are so many options that I think the -C just gets lost in the noise, so isn't confusing...

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 30, 2017

One missing kvsitr_destroy in flux-kvs.c:list_kvs_dir():
(Actually, could any iterators created on a kvsdir be automatically destroyed when the dir is destroyed? Not sure if this is bad practice though...)

--- a/src/cmd/flux-kvs.c
+++ b/src/cmd/flux-kvs.c
@@ -976,6 +976,7 @@ static void list_kvs_dir (flux_t *h, const char *key, optpar
                 free (nkey);
             }
         }
+        kvsitr_destroy (itr);
         kvsdir_destroy (dir);
     }
 done:
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 30, 2017

I did poke at this and it seemed to work great!

One thing I did notice is that flux kvs ls resource works, but flux kvs ls resource. returns resource.: No such file or directory. However, the same thing happens with flux-kvs dir so I'm not sure it is a problem

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 30, 2017

Hmm, yeah, ls(1) allows ls foo/ so probably flux-ls should allow flux ls foo.

Good catch on the kvsitr_destroy(). The kvsdir also still needs to be destroyed but not in the conditional. A few patches coming...

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 30, 2017

Hmm, yeah, ls(1) allows ls foo/ so probably flux-ls should allow flux ls foo.

One catch will be that flux kvs ls foo. should fail with Not a directory if foo is not a directory.

Nice work, flux kvs ls feels much more natural than flux kvs dir

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Aug 30, 2017

Hmm, yeah, ls(1) allows ls foo/ so probably flux-ls should allow flux ls foo.

I noticed this a long time ago and just accepted it as the way flux kvs worked, but it does seem like it should be supported. The behavior should also be in flux kvs get.

>flux kvs get resource.
flux-kvs: resource.: No such file or directory
>flux kvs get resource
flux-kvs: resource: Is a directory

Presumably, the error should be identical.

This could be a different issue and maybe we should just open a issue on it. In fact, the solution may have to come inside the flux kvs module as that is what is returning some of these error codes.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 30, 2017

This could be a different issue and maybe we should just open a issue on it. In fact, the solution may have to come inside the flux kvs module as that is what is returning some of these error codes.

I'm fine with fixing this holistically in another PR

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 30, 2017

Good point @chu11 - want to open that issue?

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Aug 30, 2017

One thing I noticed when messing with this branch

>flux kvs ls -FR .
<snip>
resource.hwloc:
by_rank.   xml.       loaded.    

resource.hwloc.by_rank:
2.   0.   1.   3.   

Do we want entries sorted before it is output? Because ls does as well. Dunno if it would come within the kvsdir api or not. It appears that jansson does not sort on iterators, so it'd have to be some wrapper intermediary layer as well. This could be for a future issue and not necessarily in this PR.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 30, 2017

Do we want entries sorted before it is output?

We could just use the JSON_SORT_KEYS flag when creating a kvsdir_t. And the list of loose keys can also be sorted with zlist_sort().

Sounds like a good idea.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage increased (+0.08%) to 78.092% when pulling 638ecad on garlick:kvs_ls into 6da40cb on flux-framework:master.

bool require_directory = false;

if (!(nkey = malloc (strlen (key) + 2))) // room for deocration char + null
log_err_exit ("malloc");

This comment has been minimized.

@chu11

chu11 Aug 30, 2017

Contributor

typo, "decoration" :-)

This comment has been minimized.

@garlick

garlick Aug 30, 2017

Author Member

oops :-/

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 30, 2017

Output is sorted now. Shall I squash this down or is it still useful for review purposes to see the incremental work?

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Aug 30, 2017

I think it's ok to squash now. One nit, the | sort can probably be removed in the tests now.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 30, 2017

Heading out for a bit so having addressed the recent travis failures, I'm going to go ahead and squash.

@garlick garlick force-pushed the garlick:kvs_ls branch from c7af510 to b6d2a5f Aug 30, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage increased (+0.09%) to 78.11% when pulling c7af510 on garlick:kvs_ls into 6da40cb on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage increased (+0.09%) to 78.106% when pulling b6d2a5f on garlick:kvs_ls into 6da40cb on flux-framework:master.

test_expect_success 'kvs: ls key. fails if key is not a directory' '
flux kvs unlink -Rf $DIR &&
flux kvs put $DIR.a=42 &&
! flux kvs ls -d $DIR.a.

This comment has been minimized.

@grondo

grondo Aug 30, 2017

Contributor

Just a general comment, no reason to fix it here and now

Instead of ! command to indicate expected (required) failure, it might be more correct to use either test_must_fail or test_expect_code. See sharness docs here

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 30, 2017

I'll go ahead and address the unnecessary sort and the use of ! in the test.

@garlick garlick force-pushed the garlick:kvs_ls branch from b6d2a5f to 9203bcf Aug 30, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage increased (+0.07%) to 78.081% when pulling 9203bcf on garlick:kvs_ls into 6da40cb on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 30, 2017

Thanks, LGTM.

@chu11
Copy link
Contributor

chu11 left a comment

LGTM, with only 1 nit on a unit test for bad input on kvsitr_create().


if (!dir) {

This comment has been minimized.

@chu11

chu11 Aug 30, 2017

Contributor

Should we add a unit test for NULL input to kvsitr_create() now that it's checking for NULL input?

if (itr->iter) {
name = json_object_iter_key (itr->iter);
itr->iter = json_object_iter_next (itr->dirdata, itr->iter);
if (itr) {

This comment has been minimized.

@chu11

chu11 Aug 30, 2017

Contributor

possibly a unit test for bad input here as well? although it might not be necessary coverage wise for this one.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 30, 2017

Sure, I'll do that.

garlick added some commits Aug 25, 2017

cmd/flux-kvs: add ls subcommand
Add new subcommand:
   flux kvs ls [-R] [-d] [-w COLS] [-1|-C] [-F] [key...]

This subcommand mimics ls(1) behavior and options,
as discussed in #1158.
libkvs/kvs_dir: sort directories by key
Change the kvsitr_t implementation to create a list of keys
from the directory object and sort it, then access the list
in order.

@garlick garlick force-pushed the garlick:kvs_ls branch from 9203bcf to 3854952 Aug 30, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 30, 2017

I added the tests you suggested to the commit that modified the kvsitr_t implementation (squashed in and forced).

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 30, 2017

Coverage Status

Coverage increased (+0.09%) to 78.106% when pulling 3854952 on garlick:kvs_ls into 6da40cb on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Aug 30, 2017

LGTM, if no complaints will hit button.

@chu11 chu11 merged commit 2eb43ea into flux-framework:master Aug 30, 2017

4 checks passed

codecov/patch 88.46% of diff hit (target 77.64%)
Details
codecov/project 77.73% (+0.09%) compared to 6da40cb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 78.106%
Details

@garlick garlick deleted the garlick:kvs_ls branch Sep 6, 2017

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.