Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Adding a redis:flushall command #74

Merged
merged 2 commits into from Mar 24, 2013

Conversation

Projects
None yet
2 participants
Contributor

Sgoettschkes commented Dec 2, 2012

For development/testing, I created this command in addition to the redis:flushdb command.

It flushes all redis databases, using the native flushall command.

@snc snc commented on an outdated diff Dec 2, 2012

Command/RedisFlushallCommand.php
+ * @return boolean true if either no-interaction was chosen or the user wants to proceed
+ */
+ private function proceedingAllowed() {
+ if ($this->input->getOption('no-interaction')) {
+ return true;
+ }
+
+ return $this->getHelper('dialog')->askConfirmation($this->output, '<question>Are you sure you wish to flush the whole database? (y/n)</question>', false);
+ }
+
+ /**
+ * Flushing all redis databases
+ */
+ private function flushAll() {
+ try {
+ $redis = $this->getContainer()->get('snc_redis.default');
@snc

snc Dec 2, 2012

Owner

There is a chance that the default client does not exist. Another scenario would be that someone uses more than one redis instance so it would be better to let the user specify the client.

Owner

snc commented Dec 2, 2012

Regarding my last comment, I think it would be nice to refactor the commands to share some code (abstract class RedisCommand extends ContainerAwareCommand).

Contributor

Sgoettschkes commented Dec 10, 2012

I'll do this (it's not forgotten). I tried to get some tests up and running (mainly because I don't like doing bigger refactorings without tests), but struggled with dependencies. I'll see if I can set up something this week and then refactor the code!

Contributor

Sgoettschkes commented Dec 24, 2012

It took a while, but I got it. I added some tests and extracted common code into a base class.

The redis:flushall command now takes a client option!

I guess there will be some discussion on the code, so send me feedback :)

Owner

snc commented Jan 13, 2013

Sorry for the delay, one thing should be changed, currently it looks like only predis clients would work, but phpredis clients should work, too. Could you adjust the docblocks etc.?

Contributor

Sgoettschkes commented Jan 15, 2013

I only found one docblock (Sgoettschkes/SncRedisBundle@129cbb0#L0R39) which I changed. I looked up the Phpredis client but found it to be identical to the Predis client for my use cases, so I did not change anything else.

I also improved the CommandTestCase to provide both clients for testing. I did not write any more tests as both clients have the exact same way they work, so everything but the class which get's mocked wouldn't change. Also the code does not change depending on the client, so to me it makes no sense to add those tests.

Owner

snc commented Feb 16, 2013

Sorry for the delay... I just checked the TravisCI output. The tests now fail. Can you fix them?

Contributor

Sgoettschkes commented Mar 23, 2013

Now I have to apologize for the delay ;) I just checked the travis output at https://travis-ci.org/snc/SncRedisBundle/jobs/4173042, which fails downloading some dependency (looks like a temporary issue). I run phpunit locally with all tests passing.

As repository owner, you can restart tests using the small icon on the right (beside the tabs "Current", "Build History"). Could you please do that and see if it works this time? Thanks!

Owner

snc commented Mar 24, 2013

Done. There is one related issue:
1) Snc\RedisBundle\Tests\Command\RedisFlushallCommandTest::testWithDefaultClientAndNoInteraction Declaration of Mock_Client_e31ccf37::setRedis() should be compatible with Snc\RedisBundle\Client\Phpredis\Client::setRedis(Redis $redis)

Contributor

Sgoettschkes commented Mar 24, 2013

Thanks! I am not able to reproduce this error on my linux vm. I am also not sure what it means.

The error occurs when I try to mock \Snc\RedisBundle\Client\Phpredis\Client because the declaration of setRedis on the Mock is different from the original one. I played around a bit (see my commits which I'll stash once everything is stable) and it seems that PHPUnit has a problem to create the Mock of the Phpredis Client correctly. I am not sure why this works on my box and not on travis but I think one would need to install PHPRedis to create a Mock successfully.

I am trying how to get travis to the point where it installs phpredis correctly! Till then, please consider this WIP.

Contributor

Sgoettschkes commented Mar 24, 2013

All right, travis tests are passing. As said above, the problem was that PHPRedis was not installed on the virtual machine, causes PHPUnit to not be able to create a Mock which takes an argument of class \Redis, because it's unknown.

I added some steps to the travis.yml before_script.

I think this PR is ready to merge.

Owner

snc commented Mar 24, 2013

Yes, me too :-) Can you squash your commits?

Contributor

Sgoettschkes commented Mar 24, 2013

Done. I split it up to keep context but can squash those two if you like.

@snc snc added a commit that referenced this pull request Mar 24, 2013

@snc snc Merge pull request #74 from Sgoettschkes/flushall_command
Adding a redis:flushall command
54ed3ba

@snc snc merged commit 54ed3ba into snc:master Mar 24, 2013

1 check passed

default The Travis build passed
Details
Owner

snc commented Mar 24, 2013

Merged! Thanks :-)

@Sgoettschkes Sgoettschkes deleted the unknown repository branch Mar 24, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment