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

Misc cleanup & fixes #1636

Merged
merged 7 commits into from Aug 31, 2018

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Aug 31, 2018

Misc minor fixes peeled off of PR #1564

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 31, 2018

Coverage Status

Coverage decreased (-0.007%) to 79.154% when pulling c2ddf7f on chu11:misccleanup13 into bf5af45 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 31, 2018

hit a #1595 , which I doubt anything I did in this PR is the cause. Restarted.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 31, 2018

This looks fine to me. @grondo can merge if he concurs.

@grondo
Copy link
Contributor

grondo left a comment

commit message on ea7aaf4 has an extra /, should just be broker:, admittedly very minor.

Other than that one similarly very minor comment (typo probably) inline. Otherwise, thanks for all the fixes!

@@ -334,7 +334,7 @@ void add_args_list (char **argz, size_t *argz_len, optparse_t *opt, const char *
optparse_getopt_iterator_reset (opt, name);
while ((arg = optparse_getopt_next (opt, name)))
if (argz_add (argz, argz_len, arg) != 0)
log_err_exit ("subprocess_argv_append");
log_err_exit ("argv_add");
}

This comment has been minimized.

@grondo

grondo Aug 31, 2018

Contributor

very minor: s/argv_add/argz_add?

This comment has been minimized.

@chu11

chu11 Aug 31, 2018

Author Contributor

ahh yes

chu11 added some commits Aug 24, 2018

broker/runlevel: Remove duplicate context setting
The runlevel_t context was added to a subprocess structure twice.
Instead, add it only once and add an additional assert to catch
for error.
modules/cron: Handle "Rexec Failure" case
In addition to handling the "Exec Failure" case in
cron_entry_completion_handler, also handle the "Rexec Failure"
case.
t/t0015-cron.t: Fix race between kill and dump
After a cron delete --kill, there is a small race between the kill
and cron knowing it has been killed.

Add a loop that waits for the kill to be confirmed, but gives up
after a long 5 seconds.

@chu11 chu11 force-pushed the chu11:misccleanup13 branch from 41144a4 to c2ddf7f Aug 31, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 31, 2018

just re-pushed with fixes, and also added 1 additional comment typo I saw about 20 mins ago.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 31, 2018

Excellent! Will merge when travis reports back inevitable success!

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 31, 2018

hit a #731, restart builder. And hit a hang after all tests completed successfully (no fails or errors found). Not sure. Maybe cleanup is slow? Restarted builder.

�[0;32mPASS�[m: ../t/t9990-python-tests.t 44 test.request.TestRequestMethods.test_null_payload


No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 31, 2018

Codecov Report

Merging #1636 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #1636      +/-   ##
==========================================
+ Coverage   78.96%   78.97%   +<.01%     
==========================================
  Files         179      179              
  Lines       32482    32515      +33     
==========================================
+ Hits        25651    25680      +29     
- Misses       6831     6835       +4
Impacted Files Coverage Δ
src/broker/broker.c 76.77% <ø> (ø) ⬆️
src/cmd/flux-start.c 80.71% <0%> (ø) ⬆️
src/broker/runlevel.c 85.27% <100%> (ø) ⬆️
src/modules/cron/cron.c 79.39% <33.33%> (-0.33%) ⬇️
src/common/libflux/request.c 88.46% <0%> (-1.29%) ⬇️
src/broker/overlay.c 73.81% <0%> (-0.32%) ⬇️
src/common/libflux/message.c 80.78% <0%> (-0.12%) ⬇️
src/cmd/flux-event.c 77.57% <0%> (ø) ⬆️
src/modules/connector-local/local.c 74.18% <0%> (+0.2%) ⬆️
src/common/libutil/base64.c 95.07% <0%> (+0.42%) ⬆️
... and 1 more
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 31, 2018

diff coverage is unsurprisingly bad, mostly b/c of log and error paths and the fact this is like ~10 lines of code change total.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 31, 2018

The .007% decrease in coverage is suspicious! Merging anyway!

@grondo grondo merged commit 29c0c6c into flux-framework:master Aug 31, 2018

3 of 4 checks passed

codecov/patch 50% of diff hit (target 78.96%)
Details
codecov/project 78.97% (+<.01%) compared to bf5af45
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.007%) to 79.154%
Details
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.