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

cmd/flux-kvs: Update dir and ls usage #1444

Merged
merged 4 commits into from Apr 11, 2018

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Apr 10, 2018

With the dir and ls commands in flux-kvs, if a user specifies a
namespace prefix without a key, assume the user desires the root
(i.e. ".") directory within that namespace.

Update/adjust unit tests appropriately.

Fixes #1434

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 10, 2018

Er. not sure this looks right:

$ flux kvs namespace-create foo
$ flux kvs put ns:foo/a=b
$ flux kvs dir ns:foo/
ns:foo/..a = b
&& (ptr = strchr (nkey, '/'))) {
/* No key suffix, decorate with default '.' */
if (*(ptr + 1) == '\0')
strcat (nkey, ".");

This comment has been minimized.

@grondo

grondo Apr 10, 2018

Contributor

Because you decorate with . here, is that resulting in an extra . when the keys are actually printed in dir? e.g. I'm seeing:

$ flux kvs put ns:foo/bar=none
$ flux kvs dir ns:foo/
ns:foo/..bar = none

The /.. might confusing people. I would assume we'd want the same output as we used to put the key in the first place, i.e. ns:foo/bar = none -- leading dots stripped.

This comment has been minimized.

@grondo

grondo Apr 10, 2018

Contributor

As long as you are in here, I wonder if a fix for this issue would also fix #1391? For some reason I find this awkward:

$ flux kvs dir resource. 
resource..hwloc.

Possibly just stripping all trailing . from "key" will help?

This comment has been minimized.

@grondo

grondo Apr 10, 2018

Contributor

Oops sorry, @garlick beat me to it.

This comment has been minimized.

@chu11

chu11 Apr 10, 2018

Author Contributor

Yup, that is issue #1391. I was going to fix it separately.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 10, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@f4e0b7e). Click here to learn what that means.
The diff coverage is 93.75%.

@@            Coverage Diff            @@
##             master    #1444   +/-   ##
=========================================
  Coverage          ?   78.76%           
=========================================
  Files             ?      163           
  Lines             ?    30208           
  Branches          ?        0           
=========================================
  Hits              ?    23792           
  Misses            ?     6416           
  Partials          ?        0
Impacted Files Coverage Δ
src/cmd/flux-kvs.c 81.26% <93.75%> (ø)
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 10, 2018

@garlick The double period is issue #1391, was going to fix that issue separately. But could merge in here if that's what's desired.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 10, 2018

chu11 added some commits Apr 10, 2018

cmd/flux-kvs: Update dir and ls usage
With the dir and ls commands in flux-kvs, if a user specifies a
namespace prefix without a key, assume the user desires the root
(i.e. ".") directory within that namespace.

Update/adjust unit tests appropriately.

Fixes #1434
common/libkvs: Fix period output corner cases
Fix output corner cases where double periods or errant additional
period could be output.

Fixes #1391
t/kvs: Add period output corner case tests
Also adjust older tests to remove double periods.

@chu11 chu11 force-pushed the chu11:issue1434 branch from f6ca59a to 15638eb Apr 11, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 11, 2018

Just pushed update w/ period output corner case fixes. It fixes both the double period output case an an extraneous period output corner case that I found along the way. Lots of unit tests added.

Also added in dumb patch in which I remove a duplicate test.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 11, 2018

hit a

PASS: t4000-issues-test-driver.t 3 - t0821-kvs-segfault.sh

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

which I think is #1311. Restarted builder.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 11, 2018

Better!

$ flux kvs put ns:foo/a=b
$ flux kvs dir ns:foo/
ns:foo/a = b
$ flux kvs dir ns:foo/.
ns:foo/a = b

Hmm, what about this one?

$ flux kvs dir ns:foo/..
ns:foo/..a = b
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 11, 2018

I would say this is a different bug, where we have to decide if we want to "normalize" a users input before output. We didn't do that before.

>flux kvs ls -R resource......hwloc...xml
resource......hwloc...xml:
0    1    2    3

Is this a change we want to do? I think that it should be different issue though.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 11, 2018

I'm going to create a new issue for the above case, as I feel its separate from this PR and has a few things to consider.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 11, 2018

Sounds good. I'll go ahead and merge this. Thanks!

@garlick garlick merged commit 81eef20 into flux-framework:master Apr 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 79.029%
Details

@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.