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

Use Hubot for notifications #72

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
9 participants

Initial work on #71.

Should I remove previous chat_services and compatibility code?

The patch on the janky hubot script is coming.

@francois2metz francois2metz referenced this pull request in github/hubot-scripts May 9, 2012

Merged

Add a /janky endpoint for janky notifications. #408

The hubot part is now merged. See github/hubot-scripts#408.

Can I do anything more on this?

Contributor

sr commented Jun 5, 2012

@francois2metz Sorry for huge delay here. I am hoping to spend some time in Janky sometimes this week. Thanks!

@sr sr commented on an outdated diff Jun 23, 2012

README.md
@@ -161,12 +161,22 @@ Required settings:
Installation:
-* Add `require "janky/chat_service/hipchat"` to the `config/environment.rb` file
@sr

sr Jun 23, 2012

Contributor

This is the recommended file where to require things like that. I'd like to keep config/environment.rb here. See https://gist.github.com/1497335.

@sr sr commented on the diff Jun 23, 2012

README.md
**before** the `Janky.setup(ENV)` line.
* `echo 'gem "hipchat", "~>0.4"' >> Gemfile`
* `bundle`
* `git commit -am "install hipchat"`
+#### Hubot
+
+* `JANKY_CHAT=hubot`
+* `JANKY_CHAT_HUBOT_URL`: the url to the hubot instance
@sr

sr Jun 23, 2012

Contributor

An example would be great here.

@sr sr commented on an outdated diff Jun 23, 2012

README.md
**before** the `Janky.setup(ENV)` line.
* `echo 'gem "hipchat", "~>0.4"' >> Gemfile`
* `bundle`
* `git commit -am "install hipchat"`
+#### Hubot
+
+* `JANKY_CHAT=hubot`
+* `JANKY_CHAT_HUBOT_URL`: the url to the hubot instance
+
+Installation:
+
+* Add `require "janky/chat_service/hubot"` to the `config.ru` file
@sr

sr Jun 23, 2012

Contributor

Ditto. I'd like this to be in config/environment.rb instead.

@sr sr commented on an outdated diff Jun 23, 2012

lib/janky/chat_service/hubot.rb
+
+ def request(message, room)
+ uri = @url
+ user = uri.user
+ pass = uri.password
+ path = uri.path
+
+ http = Net::HTTP.new(uri.host, uri.port)
+ if uri.scheme == "https"
+ http.use_ssl = true
+ end
+
+ post = Net::HTTP::Post.new("#{path}/janky")
+ post.basic_auth(user, pass) if user && pass
+ post["Content-Type"] = "application/json"
+ post.body = {message: message, room: room}.to_json
@sr

sr Jun 23, 2012

Contributor

Please use the old Hash syntax here. Janky should work on Ruby 1.8.7, although this may change in later versions.

@sr sr commented on the diff Jun 23, 2012

lib/janky/hubot.rb
@@ -47,22 +47,13 @@ class Hubot < Sinatra::Base
end
end
- # Get a list of available rooms.
- get "/rooms" do
- Yajl.dump(ChatService.room_names)
- end
@sr

sr Jun 23, 2012

Contributor

What's the reason for removing this?. I'd like to keep hubot ci rooms. It's useful for hubot ci set room.

@francois2metz

francois2metz Jun 28, 2012

Ok. I'll keep it for old adapters.

@sr sr commented on the diff Jun 23, 2012

.../database/migrate/1335992968_add_room_drop_room_id.rb
@@ -0,0 +1,17 @@
+class AddRoomDropRoomId < ActiveRecord::Migration
+ def self.up
+ add_column :builds, :room, :text, :null => true
+ remove_column :builds, :room_id
+
+ add_column :repositories, :room, :text, :null => true
+ remove_column :repositories, :room_id
@sr

sr Jun 23, 2012

Contributor

What's the reason for switching to a text column here? We'd have to migrate the data if we go that route.

@francois2metz

francois2metz Jun 28, 2012

The room as integer is very campfire/hipchat specific. It doesn't work with irc adapter. Rooms name are strings. Just forgot about the migration.

Contributor

sr commented Jun 23, 2012

Sorry for taking so long to review this.

Great start! Big 👍 on keeping the existing notifiers around. We might to remove them in later versions but maintaining backward compatibility is definitely a priority.

I am a little skeptical of the room/room_id changes. Why are these changes necessary? The diff looks great otherwise.

Some inline comments replied. I'll work on it later. Thanks for the review.

Just curious if this is something you all are still looking at implementing? I just started using Janky and would love to see it no longer depend on a specific chat service since Hubot handles that already.

The fork is almost up to date. And I use it in production :).

On 02/05/2013 03:32 AM, Erich Menge wrote:

Just curious if this is something you all are still looking at
implementing? I just started using Janky and would love to see it no
longer depend on a specific chat service since Hubot handles that already.


Reply to this email directly or view it on GitHub
#72 (comment).

François
https://stormz.me

+1

francois2metz added some commits May 8, 2012

@francois2metz francois2metz Initial work on hubot notifications.
Mostly remove any reference to room_id. Renamed to room as a string.
See #71.

Signed-off-by: François de Metz <francois@stormz.me>
4fc33cc
@francois2metz francois2metz Add hubot chat_service.
Signed-off-by: François de Metz <francois@stormz.me>
7d50489
@francois2metz francois2metz Some doc about the Hubot adapter.
Signed-off-by: François de Metz <francois@stormz.me>
be6d8f1
@francois2metz francois2metz Ruby 1.8 hash style.
Signed-off-by: François de Metz <francois@stormz.me>
1310943
@francois2metz francois2metz Update install instructions with config/environment.rb.
Signed-off-by: François de Metz <francois@stormz.me>
2a2e479

jvc26 commented May 9, 2013

Any news on this getting pulled into master?

Contributor

atmos commented Sep 20, 2013

Can you guys merge master so we can see about merging this?

Contributor

parkr commented Dec 12, 2013

Would love this.

Contributor

luxflux commented Jan 27, 2014

Whats the state here? Will you merge? Would be cool to have this!

Contributor

calmyournerves commented Feb 5, 2014

:shipit:

Contributor

luxflux commented Apr 4, 2014

@atmos any chance of getting this merged?

Contributor

atmos commented Apr 8, 2014

How does this work with all the people who have it setup with existing campfire stuff. There's no data migration to tranform the columns.

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