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

Added Twilio Contact #77

Open
wants to merge 15 commits into
from

Conversation

Projects
None yet
4 participants

Hi all, I wrote a Twilio contact lib so that you can get notified by SMS when a monitoring event occurs (like if your website goes down!) - tested it out and it all works as expected. Let me know if anything looks wonky in the merge and I'll be happy to correct it

Owner

mojombo commented Jan 13, 2012

This is looking good. Would you mind adding a few test cases for this?

Added test for Twilio contact
I made the twilio info message more verbose and added a test

Should be solid now, let me know if you need anything else from my end

@mojombo mojombo and 1 other commented on an outdated diff Jan 14, 2012

test/test_twilio.rb
@@ -0,0 +1,21 @@
+require File.dirname(__FILE__) + '/helper'
+
+class TestTwilio < Test::Unit::TestCase
+ def setup
+ @twilio = God::Contacts::Twilio.new
+ end
+
+ def test_exists
+ God::Contacts::Twilio
+ end
+
+ def test_notify
+ @twilio.account_sid = 'AC6d969d749c7e4e0bb5eb8968b85fa759'
+ @twilio.auth_token = '1c43e9e653a181a3180083297ca6cf81'
@mojombo

mojombo Jan 14, 2012

Owner

These appear to be real credentials. I'm guessing you don't want the world to see and potentially abuse these. A better approach is probably to mock these out so no real call takes place and just make sure the proper calls are happening on the Twilio API.

@jonmarkgo

jonmarkgo Jan 14, 2012

Its a demo account made specifically for god though I'll change it to mock later today

@mojombo

mojombo Jan 14, 2012

Owner

Ok, cool. Mock is still better. It'll run faster, gives nearly as much protection against errors, and doesn't send someone a text message as a side effect. =)

twilio-ruby will be a runtime dependancy

Hey @mojombo - got proper tests in there now

Owner

mojombo commented Jan 21, 2012

@jonmarkgo I'm getting a test failure on this, can you check it out?

  1) Failure:
test_notify(TestTwilio) [test/test_twilio.rb:19]:
not all expectations were satisfied
unsatisfied expectations:
- expected exactly once, not yet invoked: #<Mock:0x7fe8fbacdb90>.create(:from => '1234567890', :to => '9876543210', :body => 'msg')
satisfied expectations:
- expected exactly once, invoked once: #<Mock:0x7fe8fbacd2d0>.messages(any_parameters)
- expected exactly once, invoked once: #<Mock:0x7fe8fbacc8d0>.sms(any_parameters)
- expected exactly once, invoked once: #<Mock:0x7fe8fbacbef8>.account(any_parameters)
- expected exactly once, invoked once: Twilio::REST::Client.new('ACXXXXX', 'YYYYYY')
Owner

mojombo commented Jan 21, 2012

Also, the dependency should be set as development like the others. God takes care of telling you what gems to install if you do indeed use that contact. The idea is that God won't install a bunch of deps you don't need unless you actually use that functionality.

"Development dependencies are not installed by default, and are not activated when the gem is."

@mojombo i definitely understand and agree with wanting to keep requiring possibly unused libraries to a minimum, but what is the point of them being in the gemspec then? is it to merely indicate that it's a dependency, but it's on the user to satisfy it manually?

Owner

mojombo commented Jan 22, 2012

Development dependencies are needed in order to develop the software and run the test suite.

yes that's obviously what a development dependency is for, but what happens when someone wants to use god with the twilio contact? there is no dependency, so it will blow up with a loaderror unless they already have the twilio-ruby gem installed. may i suggest adding gem 'twilio-ruby' to give a more descriptive error, if you want to keep runtime dependencies lean?

Hey @mojombo - are there still issues with this?

Owner

mojombo commented Feb 13, 2012

@stevegraham God has a loading mechanism that catches LoadError and informs the user that a dependency needs to be installed. This has been designed so that upon installing God you don't need to also install 20 gems that you'll never use.

See https://github.com/mojombo/god/blob/master/lib/god.rb#L367-375

solid. we'll take it out then. cheers @mojombo :)

Should be all good now @mojombo

Hey guys, this looks really useful. Any reason not to merge?

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