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

libkvs/eventlog: drop typedefs that violate RFC 7 #1717

Merged
merged 2 commits into from Oct 11, 2018

Conversation

Projects
None yet
3 participants
@garlick
Copy link
Member

garlick commented Oct 11, 2018

I've been sitting on this for a while, moving it around between pull requests I've been working on. Maybe better to just submit standalone rather than keep withholding it.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 11, 2018

looks good, but just need to update doc/man3/flux_kvs_eventlog_create.adoc too

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 11, 2018

Thanks, I'll fix that.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 11, 2018

Looks good to me. One nit comment now that I read the manpage entry. Should the length macros like MAX_KVS_EVENT_NAME be renamed? It seems weird that the name begins with MAX, should it begin with FLUX_KVS or KVS instead?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 11, 2018

Good point. I'll fix that too - hang on.

@garlick garlick force-pushed the garlick:eventlog_Rfc7 branch from 8d683e4 to 957737c Oct 11, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 11, 2018

OK, that's fixed and squashed. Thanks for catching that stuff @chu11. I was being a little sloppy.

@garlick garlick force-pushed the garlick:eventlog_Rfc7 branch from 957737c to 77cc08a Oct 11, 2018

garlick added some commits Oct 1, 2018

libkvs/eventlog: drop typedefs that violate RFC 7
Problem: the typedefs 'flux_kvs_event_name_t' and
'flux_kvs_event_context_t' are fixed length arrays
that violate RFC 7.

Convert to a (char *buf, int size) interface.

Update eventlog unit test.

Update flux-kvs eventlog sub-command.

Fixes #1702

@garlick garlick force-pushed the garlick:eventlog_Rfc7 branch from 77cc08a to 385efef Oct 11, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 11, 2018

Codecov Report

Merging #1717 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1717      +/-   ##
==========================================
+ Coverage   79.32%   79.33%   +<.01%     
==========================================
  Files         184      184              
  Lines       34548    34550       +2     
==========================================
+ Hits        27405    27409       +4     
+ Misses       7143     7141       -2
Impacted Files Coverage Δ
src/common/libkvs/kvs_eventlog.c 91.78% <100%> (+0.11%) ⬆️
src/cmd/flux-kvs.c 81.8% <100%> (ø) ⬆️
src/modules/kvs/kvs.c 65.81% <0%> (-0.16%) ⬇️
src/cmd/flux-module.c 85.58% <0%> (+0.3%) ⬆️
src/common/libflux/handle.c 84.38% <0%> (+0.69%) ⬆️
@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 11, 2018

LGTM!

@chu11 chu11 merged commit 52b3636 into flux-framework:master Oct 11, 2018

3 checks passed

codecov/patch 100% of diff hit (target 79.32%)
Details
codecov/project 79.33% (+<.01%) compared to 9fd52be
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 11, 2018

Thanks!

@garlick garlick deleted the garlick:eventlog_Rfc7 branch Oct 16, 2018

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.