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: doc updates, consistency fixes, new convenience options #896

Merged
merged 10 commits into from Nov 4, 2016

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Nov 4, 2016

Wanted to add a simple option to list kvs entries without the values since some entries are gigantic and flood stdout. Sometimes just want to know what entries were there. Lead to a lot of cleanup along the way.

The new -d option I added to several subcommands was chosen b/c I likened the option to the -d option of ls. I know that may just be my weird way of thinking about it and maybe most won't think of it that way.

The subcommand option parsing could be cleaned up, but I decided to leave it be, let the code have one more "strike" before cleanup.

chu11 added some commits Nov 3, 2016

doc/flux-kvs(1): Add missing -r options
Document -r recursive option for dir and dirat subcommands.
cmd/flux-kvs: Add count to watch subcommand
For consistency to watch-dir command, offer a 'count' option
to the watch subcommand.
doc/test: Add new words to dictionary
Add dirsize and subdirectories
cmd/flux-kvs: Add option to not output key value
Support -d option in dir, dirat, and watch-dir subcommands
to not output values of keys.  This option may be useful when
values are extremely long and the user is only interested in
listing the keys or knowing that key values changed.
doc/flux-kvs(1): Document -d subcommand options
Documentation -d option for dir, dirat, and watch-dir subcommands.
test/kvs: Test -d subcommand options
Add tests for -d option in dir, dirat, and watch-dir subcommands.

@garlick garlick added the review label Nov 4, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 4, 2016

Current coverage is 72.31% (diff: 96.00%)

Merging #896 into master will increase coverage by 0.08%

@@             master       #896   diff @@
==========================================
  Files           157        157          
  Lines         27012      27041    +29   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19511      19556    +45   
+ Misses         7501       7485    -16   
  Partials          0          0          
Diff Coverage File Path
••••••••• 96% src/cmd/flux-kvs.c

Powered by Codecov. Last update e8f8968...c31123d

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 4, 2016

Coverage Status

Coverage increased (+0.09%) to 75.919% when pulling c31123d on chu11:kvsdirkeyonly into e8f8968 on flux-framework:master.

@garlick garlick referenced this pull request Nov 4, 2016

Closed

flux-kvs needs cleanup #897

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 4, 2016

Oh, nice idea!

I wonder if we should open an issue to re-evaluate the command line interface to kvs in flux-kvs at some point, perhaps after pending rework done by @garlick? e.g. no longopts as you notice, perhaps dir could keep its current behavior and ls could just list keys, consider use of --at=treeobj instead of separate *at commands to reduce duplication of option parsing, etc. Just ideas for now...
The -d option seems fine to me, but I do wonder what the corresponding longopt would be.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 4, 2016

Oh, @garlick beat me to it with #897!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 4, 2016

Sorry just getting ready to hit the road so I don't have time to review, but feel free to make these changes in my absence. I like the general idea where @chu11 is headed here. I would like to get the bigger problem in #897 solved (and this does contribute to the mess slightly) but as I've been putting it off for like a dozen subcommands I won't insist it be fixed in this PR :-)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 4, 2016

Sounds good to me. Since @chu11 has a good use case and we can kick the cleanup can down the road, I see no reason not to merge this.

@grondo grondo merged commit da42f80 into flux-framework:master Nov 4, 2016

4 checks passed

codecov/patch 96.00% of diff hit (target 72.23%)
Details
codecov/project 72.31% (+0.08%) compared to e8f8968
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 75.919%
Details

@grondo grondo removed the review label Nov 4, 2016

@grondo grondo referenced this pull request Nov 28, 2016

Closed

Create 0.6.0 release notes #916

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.