torproject/onionbalance

Feature: Reload configuration upon receiving SIGHUP

asn-d6 opened this issue · 12 comments

asn-d6 commented

Identical to DonnchaC/onionbalance#24

Patches welcome.

gitcoinbot commented

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 200.0 DAI (200.0 USD @ $1.0/DAI) attached to it.

gitcoinbot commented

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 1 week from now.
Please review their action plans below:

1) touhonoob has started work.

reload configuration on SIGHUP

Learn more on the Gitcoin Issue Details page.

touhonoob commented

Does v2 need to be supported as well?

asn-d6 commented

Does v2 need to be supported as well?

Nope.

gitcoinbot commented

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 200.0 DAI (200.0 USD @ $1.0/DAI) has been submitted by:

  1. @touhonoob

@ceresstation please take a look at the submitted work:

asn-d6 commented

Hello, this looks really good! Great work!
I left a comment that should be easy to fix.

Also, can you think of any unittests that would help with coverage here? Either checking that the config exceptions are thrown correctly, or that the SIGHUP handler works as expected?

Thanks! :)

asn-d6 commented

Hello. Thanks for your work. As a final thing before we wrap this up, is it possible for you to disable the functional unittests in CI/tox so that they stop breaking Travis? Since they are unstable I guess we should disable them for now until we fix them. As a matter of fact, I thought they were disabled, but I see this PR failing on the functional tests so I'm not sure if I had actually disabled them.

touhonoob commented

@asn-d6 Yes, functional tests were disabled on Travis-CI as the $TEST environment variable was never set. The CI/testing stuff indeed deserves its own pull request I think.

asn-d6 commented

ACK. But then why did PR#31 fail initially if the TEST envvar was not set? Also, I see that your last commit (3005dab) enabled the TEST envvar, so perhaps we should disable it again?

asn-d6 commented

Merged! Thanks a lot!! :)

gitcoinbot commented

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 200.0 DAI (200.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @touhonoob.