Skip to content

Added autoboot configuration option and supporting implementation #20

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 17, 2019
Merged

Added autoboot configuration option and supporting implementation #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1710a14
Added configuration option and supporting implementation to autoboot …
Nov 7, 2018
64573c6
Fixed sketchy tests for autobooting
Nov 7, 2018
83643a7
Fixed config cleaning clearing autoboot setting
Nov 8, 2018
10a2620
Clean up auto-boot config retainment
Nov 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions config/redis-sentinel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
$password = env('REDIS_SENTINEL_PASSWORD', env('REDIS_PASSWORD', null));
$database = env('REDIS_SENTINEL_DATABASE', env('REDIS_DATABASE', 0));
$service = env('REDIS_SENTINEL_SERVICE', 'mymaster');
$autoBoot = env('REDIS_SENTINEL_AUTO_BOOT', false);

return [

Expand Down Expand Up @@ -70,6 +71,8 @@

'clean_config' => true,

'auto_boot' => $autoBoot,

/*
|--------------------------------------------------------------------------
| Redis Sentinel Database Driver
Expand Down
11 changes: 11 additions & 0 deletions src/Configuration/Loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,14 @@ class Loader
*/
public $shouldIntegrateHorizon;


/**
* Indicates whether this should auto-boot the provider immediately.
*
* @var bool
*/
public $shouldAutoBoot;

/**
* The current Laravel or Lumen application instance that provides context
* and services used to load the appropriate configuration.
Expand Down Expand Up @@ -203,6 +211,8 @@ public function loadConfiguration()

$this->shouldIntegrateHorizon = $this->horizonAvailable
&& $this->config->get('horizon.driver') === 'redis-sentinel';

$this->shouldAutoBoot = $this->config->get('redis-sentinel.auto_boot', false);
}

/**
Expand Down Expand Up @@ -475,6 +485,7 @@ 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.',
]);
}
Expand Down
20 changes: 20 additions & 0 deletions src/RedisSentinelServiceProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@
*/
class RedisSentinelServiceProvider extends ServiceProvider
{
/**
* Records whether this provider has already been booted (eg. via auto-boot)
*
* @var boolean
*/
private $isBooted = false;

/**
* Loads the package's configuration and provides configuration values.
*
Expand All @@ -41,6 +48,11 @@ class RedisSentinelServiceProvider extends ServiceProvider
*/
public function boot()
{
// Don't double boot the provider
if ($this->isBooted) {
return;
}

$this->bootComponentDrivers();

// If we want Laravel's Redis API to use Sentinel, we'll remove the
Expand All @@ -54,6 +66,8 @@ public function boot()
$horizon->register();
$horizon->boot();
}

$this->isBooted = true;
}

/**
Expand All @@ -74,6 +88,12 @@ public function register()
if ($this->config->shouldOverrideLaravelRedisApi) {
$this->registerOverrides();
}

// We may need to boot this provider immediately to work around other
// providers calling Cache too early or Lumen 5.7 issues.
if ($this->config->shouldAutoBoot) {
$this->boot();
}
}

/**
Expand Down
12 changes: 12 additions & 0 deletions tests/Unit/Configuration/LoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,18 @@ public function testSkipsCleaningPackageConfigurationWhenDisabled()
}
}

public function testSetsAutoBootConfiguration()
{
// Test that it sets the boot value correctly
$this->config->set('redis-sentinel.auto_boot', false);
$this->loader->loadConfiguration();
$this->assertFalse($this->loader->shouldAutoBoot);

$this->config->set('redis-sentinel.auto_boot', true);
$this->loader->loadConfiguration();
$this->assertTrue($this->loader->shouldAutoBoot);
}

public function testNormalizesSentinelConnectionHosts()
{
$this->startTestWithConfiguredApplication();
Expand Down
20 changes: 20 additions & 0 deletions tests/Unit/RedisSentinelServiceProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,24 @@ public function testBootExtendsSessionHandlers()
$this->assertNotNull($this->app->session->driver('redis-sentinel'));
}
}

public function testWaitsForBoot()
{
$this->app->config->set('redis-sentinel.auto_boot', false);
$this->provider->register();

$this->setExpectedException(\InvalidArgumentException::class);

// It didn't auto boot
$this->assertNull($this->app->cache->store('redis-sentinel'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cyrossignol cyrossignol Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cyrossignol cyrossignol Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cyrossignol cyrossignol Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

public function testAutoBoots()
{
$this->app->config->set('redis-sentinel.auto_boot', true);
$this->provider->register();

// Make sure it booted
$this->assertNotNull($this->app->cache->store('redis-sentinel'));
}
}