Display error and send failure exit status if redis-cli fails #109
Conversation
| 👍 |
| Nice idea @jsternberg; I have a few comments on the code. |
bin/check Outdated
There was a problem hiding this comment.
I think you can use if system(...) here; no need to use $?.
| @jsternberg if you update your PR according to my comments we can merge this. |
| I've updated the PR to use the return value of system instead of |
| There was also a commit that was made to |
lib/gitlab_update.rb Outdated
There was a problem hiding this comment.
Maybe we can change . " to .".
There was a problem hiding this comment.
I did that to keep it consistent with an earlier line. The other error message in that file has ! ". I thought it was a bit weird, but better to keep it consistent than have different stylings.
There was a problem hiding this comment.
Thanks for being careful; I think the other line is wrong. :) If you want you can remove the space after that !.
| @jsternberg I agree that your redis check in |
bin/check Outdated
There was a problem hiding this comment.
Maybe we can un-nest these ifs?
print 'Testing cli etc' if system(redis, version etc) puts 'OK' else abort 'FAILED' end print 'Sending ping etc' if system(etc) puts 'OK' else abort 'FAILED' end bin/check Outdated
There was a problem hiding this comment.
Maybe we can drop the err: '/dev/null' because the error output may be useful.
bin/check Outdated
There was a problem hiding this comment.
If we drop the err: and out: bits, this will print 'PONG'. That way we would only need
print 'sending ping etc..' unless system(redis, ping, etc) abort 'FAILED' endThere was a problem hiding this comment.
It's actually more simple than that.
abort unless system(redis, ping, etc)Redis prints a message to stderr if it fails with an appropriate error message. I can just replace it with that if that works. Example:
jsternberg@laptop:[~/gitlab-shell] $ ./bin/check Check GitLab API access: OK Check directories and files: /home/jsternberg/repositories: OK /home/jsternberg/.ssh/authorized_keys2: OK Testing redis-cli executable: redis-cli 2.6.15 Sending ping to redis server: Could not connect to Redis at 127.0.0.1:6379: Connection refused There was a problem hiding this comment.
Good idea! Let's also do that with the other system/abort combo.
| Sorry for keeping you so busy @jsternberg with all my comments. :) I think this is very useful but I would also like it to be clean. |
bin/check Outdated
There was a problem hiding this comment.
Wouldn't it be simpler to just let the redis-cli --version fail?
There was a problem hiding this comment.
abort('FAILED') unless system(*config.redis_command, '--version')
There was a problem hiding this comment.
It's in the commit message for that commit. If the redis bin isn't there, system won't print anything. This causes the newline to never be printed. The second check is just to make sure that the executable they specified actually works, or in the case of the redis section missing, env -i redis-cli works.
There was a problem hiding this comment.
Ah, I missed your commit message.
I tried it out independently in IRB and I also noticed the nil returned by system when the command is not found. I propose to address this by adding a message to the abort (see my comment above). I appreciate that the current solution gives more detailed information, but I am not sure if the added detail is worth the extra code and tying this script more tightly to lib/gitlab_config.rb.
| I rebased the previous commit off and added the |
| This looks great @jsternberg . Could you squash or at least reduce the commits? I appreciate the care you take with your commit messages but we would rather avoid merge commits and detours in master. |
| Don't pull those in yet. It looks like I rebased some of the commits into the wrong place. |
This should help prevent a misconfigured redis causing silent failures and fix issue #108.
The bin/check script now checks if Redis is configured properly.
| Those commits should be good now. |
Display error and send failure exit status if redis-cli fails
| Awesome. Thanks @jsternberg ! |
5 participants
This should help prevent a misconfigured redis causing silent failures
and fix issue #108.