Added autoboot configuration option and supporting implementation

[dave-lang]

This is to address the issue in #17

UPDATE: Added ENV & config vars 'REDIS_SENTINEL_AUTO_BOOT' & 'auto_boot'
UPDATE: Configuration loader checks autoboot setting, provider will autoboot if this is set

[cyrossignol]

Looks like tests fail here. We can $this->setExpectedException(InvalidArgumentException::class);. We better split this into two tests because of how this old version of PHPUnit handles exeptions. Maybe testWaitsForBoot() and testAutoBoots(). Otherwise looks good!

[dave-lang]

Good idea, I'm getting some other test failures on my local environment (horizon related) and I missed that one. I've just pushed an update through with it.

[cyrossignol]

Hmm, we missed the import for InvalidArgumentException in the latest commit. The PHPUnit command for testing in Lumen without Horizon installed is:

vendor/bin/phpunit --exclude-group laravel-only,horizon

That may help your local tests pass. We can also add --testsuite unit to avoid running the integration tests (if you don't have the full Redis/Sentinel setup running locally). If you're running unit tests locally against Lumen only, remove laravel/framework from the package's require-dev in composer.json and then composer update. I needed to put both in there so Scrutinizer can do the static analysis properly. My fault...need to add that note to the documentation for contributors.

[dave-lang]

Pre-coffee commits are never a good idea.. The unit tests run, however my local PHP install's missing Horizon related dependencies so there's a lot of white noise and my sleepy brain didn't pick up that one exeption was mine..

I've fixed the import issue and tracked auto boot failing down to the cache cleaning - it removes the auto_boot option. I've added in an explicit check to retain it, debating whether I add in a whitelist of config options to retain to future-proof it a little.

[cyrossignol]

Agreed...coffee before code :)

Something like this will probably work as a "whitelist" for the time being:

protected function cleanPackageConfiguration()
{
    ...
    if ($this->config->get('redis-sentinel.clean_config', true) === true) {
        $this->config->set('redis-sentinel', [
            'auto_boot' => $this->config->get('redis-sentinel.auto_boot', false),
            'Config merged. Set redis-sentinel.clean_config=false to keep.',
        ]);
    }
    ...
}

I don't foresee adding a lot of package initialization options, and the config loader is due for a refactor...it grew enough that it's hard to work with now.

Great work so far. Some of the Horizon tests on Travis fail occasionally when the site is under higher load (the timeout parameters in the integration tests are too tight--I need to tweak those). Testing is the most frustrating part of working with this otherwise simple package--it aims to support so many use cases and framework versions at this point. Let me know if you want to keep plugging away. I can merge and finish up the tests for this feature if you prefer.

[dave-lang]

I've just merged that in, much cleaner than my original code. The test suite is impressively comprehensive, I normally only have time to cover the bare minimum.

It looks like there's issues with the Laravel 5.7 tests, although they're passing locally and it looks unrelated to this commit, I won't have time to check why until tomorrow though.

[cyrossignol]

Thanks--the strong test suite is important to prove the behavior of a library that claims to support high-availability. A bug in this package that causes downtime would defeat the purpose of using Sentinel for services designed to endure outages. If you think of more test cases, I'm happy to add them.

Looks like Laravel added queueable closures in version 5.7.7 (as of now, Travis installs 5.7.13). The tests need a tweak in the app double bootstrap routine. If I remember correctly, you're running 5.7.4, so this shouldn't impact your local tests. The 5.7 test error isn't related to this change. I'll push a fix for the tests tonight to the 2.x branch--the PR should pass if you merge it in.

[dave-lang]

Looks like we never got around to closing this off, the root issue was address in lumen-framework but it would be nice to get this cleaned up as well.

[cyrossignol]

Thanks for following up, Dave. I'm glad you found a solution in Lumen. With regard to my last message: in my haste, it looks like I accidentally pushed the fix for the tests to my backup repo instead of to Github, and then a bunch of year-end stuff came up at work so I failed to notice. Sorry to keep you waiting!

I'll merge this in and finish up the change over the next few days so you don't need to devote any more time to it. Thanks again for implementing this feature!

[cyrossignol]

@davoaust Thanks again for your time on this one. Sorry it took me so long to circle back!