From 1c9b9b7a3b0ae2ee60e4cda5d1ac2541c17302f8 Mon Sep 17 00:00:00 2001 From: Ariejan de Vroom Date: Mon, 12 Dec 2011 23:03:38 +0100 Subject: [PATCH 1/7] Added Resque as a dependency --- Gemfile | 1 + Gemfile.lock | 17 +++++++++++++++++ config/routes.rb | 4 ++++ lib/tasks/resque.rake | 1 + 4 files changed, 23 insertions(+) create mode 100644 lib/tasks/resque.rake diff --git a/Gemfile b/Gemfile index 01b14cc0..2faf097b 100644 --- a/Gemfile +++ b/Gemfile @@ -23,6 +23,7 @@ gem "rdiscount" gem "acts-as-taggable-on", "~> 2.1.0" gem "drapper" gem "rchardet19", "~> 1.3.5" +gem "resque" group :assets do gem "sass-rails", "~> 3.1.0" diff --git a/Gemfile.lock b/Gemfile.lock index 54b1e25d..8a45a026 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -143,6 +143,8 @@ GEM rack (>= 0.4) rack-mount (0.8.3) rack (>= 1.0.0) + rack-protection (1.1.4) + rack rack-ssl (1.3.2) rack rack-test (0.6.1) @@ -169,6 +171,14 @@ GEM rdiscount (1.6.8) rdoc (3.11) json (~> 1.4) + redis (2.2.2) + redis-namespace (1.0.3) + redis (< 3.0.0) + resque (1.19.0) + multi_json (~> 1.0) + redis-namespace (~> 1.0.2) + sinatra (>= 0.9.2) + vegas (~> 0.1.2) rspec (2.7.0) rspec-core (~> 2.7.0) rspec-expectations (~> 2.7.0) @@ -220,6 +230,10 @@ GEM multi_json (~> 1.0.3) simplecov-html (~> 0.5.3) simplecov-html (0.5.3) + sinatra (1.3.1) + rack (~> 1.3, >= 1.3.4) + rack-protection (~> 1.1, >= 1.1.2) + tilt (~> 1.3, >= 1.3.3) six (0.2.0) sprockets (2.0.3) hike (~> 1.2) @@ -244,6 +258,8 @@ GEM uglifier (1.1.0) execjs (>= 0.3.0) multi_json (>= 1.0.2) + vegas (0.1.8) + rack (>= 1.0.0) warden (1.1.0) rack (>= 1.0) xpath (0.1.4) @@ -279,6 +295,7 @@ DEPENDENCIES rails-footnotes (~> 3.7.5) rchardet19 (~> 1.3.5) rdiscount + resque rspec-rails ruby-debug19 sass-rails (~> 3.1.0) diff --git a/config/routes.rb b/config/routes.rb index ad8b0b31..df210a20 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,5 +1,9 @@ Gitlab::Application.routes.draw do + # Optionally, enable Resque here + # require 'resque/server' + # mount Resque::Server.new, at: '/info/resque' + get 'tags'=> 'tags#index' get 'tags/:tag' => 'projects#index' diff --git a/lib/tasks/resque.rake b/lib/tasks/resque.rake new file mode 100644 index 00000000..9b30bb0a --- /dev/null +++ b/lib/tasks/resque.rake @@ -0,0 +1 @@ +require 'resque/tasks' From bc0155fbaa1261f348324c1ddf5d1eaba5907f77 Mon Sep 17 00:00:00 2001 From: Ariejan de Vroom Date: Tue, 13 Dec 2011 01:03:26 +0100 Subject: [PATCH 2/7] First attempt at a post-receive hook that posts directly to Resque --- app/models/repository.rb | 11 +++++++++++ app/workers/post_receive.rb | 5 +++++ db/schema.rb | 12 ------------ lib/gitlabhq/gitolite.rb | 4 ++-- lib/post-receive-hook | 12 ++++++++++++ 5 files changed, 30 insertions(+), 14 deletions(-) create mode 100644 app/workers/post_receive.rb create mode 100755 lib/post-receive-hook diff --git a/app/models/repository.rb b/app/models/repository.rb index 0e6f0e9a..61fcf0cb 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -31,6 +31,17 @@ class Repository project.id end + # repo.update_hook('post-receive', File.read('some-hook')) + def update_hook(name, content) + hook_file = File.join(project.path_to_repo, 'hooks', name) + + File.open(hook_file, 'w') do |f| + f.write(content) + end + + File.chmod(0775, hook_file) + end + def repo @repo ||= Grit::Repo.new(project.path_to_repo) end diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb new file mode 100644 index 00000000..01620f78 --- /dev/null +++ b/app/workers/post_receive.rb @@ -0,0 +1,5 @@ +class PostReceive + def self.perform(reponame, oldrev, newrev, ref) + puts "[#{reponame}] #{oldrev} => #{newrev} (#{ref})" + end +end diff --git a/db/schema.rb b/db/schema.rb index 17246a61..613b65cb 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -13,18 +13,6 @@ ActiveRecord::Schema.define(:version => 20111207211728) do - create_table "features", :force => true do |t| - t.string "name" - t.string "branch_name" - t.integer "assignee_id" - t.integer "author_id" - t.integer "project_id" - t.datetime "created_at" - t.datetime "updated_at" - t.string "version" - t.integer "status", :default => 0, :null => false - end - create_table "issues", :force => true do |t| t.string "title" t.integer "assignee_id" diff --git a/lib/gitlabhq/gitolite.rb b/lib/gitlabhq/gitolite.rb index e79afb55..f3d8584f 100644 --- a/lib/gitlabhq/gitolite.rb +++ b/lib/gitlabhq/gitolite.rb @@ -43,14 +43,14 @@ module Gitlabhq def destroy_project(project) FileUtils.rm_rf(project.path_to_repo) - + ga_repo = ::Gitolite::GitoliteAdmin.new(File.join(@local_dir,'gitolite')) conf = ga_repo.config conf.rm_repo(project.path) ga_repo.save end - #update or create + #update or create def update_keys(user, key) File.open(File.join(@local_dir, 'gitolite/keydir',"#{user}.pub"), 'w') {|f| f.write(key.gsub(/\n/,'')) } end diff --git a/lib/post-receive-hook b/lib/post-receive-hook new file mode 100755 index 00000000..ec7c607f --- /dev/null +++ b/lib/post-receive-hook @@ -0,0 +1,12 @@ +#!/bin/bash + +# This file was placed here by Gitlab. It makes sure that your pushed commits +# will be processed properly. + +while read oldrev newrev ref +do + # For every branch or tag that was pushed, create a Resque job in redis. + pwd=`pwd` + reponame=`basename "$pwd" | cut -d. -f1` + env -i redis-cli rpush "resque:queue:post-receive" "{\"class\":\"PostReceive\",\"args\":[\"$reponame\",\"$oldrev\",\"$newrev\",\"$ref\"]}" > /dev/null 2>&1 +done From 56fc53e8d870b70ca66332daeb6da39ab0eb5ce7 Mon Sep 17 00:00:00 2001 From: Ariejan de Vroom Date: Tue, 13 Dec 2011 20:39:02 +0100 Subject: [PATCH 3/7] Automatically write hooks when updating a repository. --- app/models/repository.rb | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 61fcf0cb..ca73f935 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -31,8 +31,13 @@ class Repository project.id end - # repo.update_hook('post-receive', File.read('some-hook')) - def update_hook(name, content) + def write_hooks + %w(post-receive).each do |hook| + write_hook(hook, File.read(File.join(Rails.root, 'lib', "#{hook}-hook"))) + end + end + + def write_hook(name, content) hook_file = File.join(project.path_to_repo, 'hooks', name) File.open(hook_file, 'w') do |f| @@ -58,6 +63,8 @@ class Repository Gitlabhq::GitHost.system.new.configure do |c| c.update_project(path, project) end + + write_hooks end def destroy_repository From edab46e9fa5f568b1423c0021e81d30453d7dc1e Mon Sep 17 00:00:00 2001 From: Ariejan de Vroom Date: Wed, 14 Dec 2011 17:38:52 +0100 Subject: [PATCH 4/7] Added web hooks functionality This commit includes: * Projects can have zero or more WebHooks. * The PostReceive job will ask a project to execute any web hooks defined for that project. * WebHook has a URL, we post Github-compatible JSON to that URL. * Failure to execute a WebHook will be silently ignored. --- Gemfile | 2 + Gemfile.lock | 10 ++ app/models/project.rb | 44 +++++++- app/models/repository.rb | 4 + app/models/web_hook.rb | 20 ++++ app/workers/post_receive.rb | 5 +- db/migrate/20111214091851_create_web_hooks.rb | 9 ++ db/schema.rb | 16 ++- spec/factories.rb | 4 + spec/models/project_spec.rb | 101 ++++++++++++++++++ spec/models/web_hook_spec.rb | 54 ++++++++++ .../requests/projects_tree_perfomance_spec.rb | 1 - spec/spec_helper.rb | 3 + spec/workers/post_receive_spec.rb | 26 +++++ 14 files changed, 295 insertions(+), 4 deletions(-) create mode 100644 app/models/web_hook.rb create mode 100644 db/migrate/20111214091851_create_web_hooks.rb create mode 100644 spec/models/web_hook_spec.rb create mode 100644 spec/workers/post_receive_spec.rb diff --git a/Gemfile b/Gemfile index 2faf097b..3c7b1a4f 100644 --- a/Gemfile +++ b/Gemfile @@ -24,6 +24,7 @@ gem "acts-as-taggable-on", "~> 2.1.0" gem "drapper" gem "rchardet19", "~> 1.3.5" gem "resque" +gem "httparty" group :assets do gem "sass-rails", "~> 3.1.0" @@ -48,6 +49,7 @@ group :development, :test do gem "awesome_print" gem "database_cleaner" gem "launchy" + gem "webmock" end group :test do diff --git a/Gemfile.lock b/Gemfile.lock index 8a45a026..09fecb88 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -87,6 +87,7 @@ GEM execjs coffee-script-source (1.1.3) columnize (0.3.4) + crack (0.3.1) daemons (1.1.4) database_cleaner (0.7.0) devise (1.5.0) @@ -111,6 +112,9 @@ GEM railties (~> 3.0) hashery (1.4.0) hike (1.2.1) + httparty (0.8.1) + multi_json + multi_xml i18n (0.6.0) jquery-rails (1.0.17) railties (~> 3.0) @@ -132,6 +136,7 @@ GEM treetop (~> 1.4.8) mime-types (1.17.2) multi_json (1.0.3) + multi_xml (0.4.1) nokogiri (1.5.0) orm_adapter (0.0.5) polyglot (0.3.3) @@ -262,6 +267,9 @@ GEM rack (>= 1.0.0) warden (1.1.0) rack (>= 1.0) + webmock (1.7.8) + addressable (~> 2.2, > 2.2.5) + crack (>= 0.1.7) xpath (0.1.4) nokogiri (~> 1.3) @@ -286,6 +294,7 @@ DEPENDENCIES gitolite! grit! haml-rails + httparty jquery-rails kaminari launchy @@ -309,3 +318,4 @@ DEPENDENCIES thin turn uglifier + webmock diff --git a/app/models/project.rb b/app/models/project.rb index a5361313..e14c1ebf 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -14,6 +14,7 @@ class Project < ActiveRecord::Base has_many :users, :through => :users_projects has_many :notes, :dependent => :destroy has_many :snippets, :dependent => :destroy + has_many :web_hooks, :dependent => :destroy acts_as_taggable @@ -79,12 +80,53 @@ class Project < ActiveRecord::Base :heads, :commits_since, :fresh_commits, + :commits_between, :to => :repository, :prefix => nil def to_param code end + def web_url + [GIT_HOST['host'], code].join("/") + end + + def execute_web_hooks(oldrev, newrev, ref) + data = web_hook_data(oldrev, newrev, ref) + web_hooks.each { |web_hook| web_hook.execute(data) } + end + + def web_hook_data(oldrev, newrev, ref) + data = { + before: oldrev, + after: newrev, + ref: ref, + repository: { + name: name, + url: web_url, + description: description, + homepage: web_url, + private: private? + }, + commits: [] + } + + commits_between(oldrev, newrev).each do |commit| + data[:commits] << { + id: commit.id, + message: commit.safe_message, + timestamp: commit.date.xmlschema, + url: "http://#{GIT_HOST['host']}/#{code}/commits/#{commit.id}", + author: { + name: commit.author_name, + email: commit.author_email + } + } + end + + data + end + def team_member_by_name_or_email(email = nil, name = nil) user = users.where("email like ? or name like ?", email, name).first users_projects.find_by_user_id(user.id) if user @@ -157,7 +199,7 @@ class Project < ActiveRecord::Base @admins ||= users_projects.includes(:user).where(:project_access => PROJECT_RWA).map(&:user) end - def root_ref + def root_ref default_branch || "master" end diff --git a/app/models/repository.rb b/app/models/repository.rb index ca73f935..9a70b0c9 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -133,4 +133,8 @@ class Repository repo.commits(ref) end.map{ |c| Commit.new(c) } end + + def commits_between(from, to) + repo.commits_between(from, to).map { |c| Commit.new(c) } + end end diff --git a/app/models/web_hook.rb b/app/models/web_hook.rb new file mode 100644 index 00000000..96684d15 --- /dev/null +++ b/app/models/web_hook.rb @@ -0,0 +1,20 @@ +class WebHook < ActiveRecord::Base + include HTTParty + + # HTTParty timeout + default_timeout 10 + + belongs_to :project + + validates :url, + presence: true, + format: { + with: /(^$)|(^(http|https):\/\/[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(([0-9]{1,5})?\/.*)?$)/ix, + message: "should be a valid url" } + + def execute(data) + WebHook.post(url, body: data.to_json) + rescue + # There was a problem calling this web hook, let's forget about it. + end +end diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 01620f78..d79f4599 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -1,5 +1,8 @@ class PostReceive def self.perform(reponame, oldrev, newrev, ref) - puts "[#{reponame}] #{oldrev} => #{newrev} (#{ref})" + project = Project.find_by_path(reponame) + return false if project.nil? + + project.execute_web_hooks(oldrev, newrev, ref) end end diff --git a/db/migrate/20111214091851_create_web_hooks.rb b/db/migrate/20111214091851_create_web_hooks.rb new file mode 100644 index 00000000..c6ba89c1 --- /dev/null +++ b/db/migrate/20111214091851_create_web_hooks.rb @@ -0,0 +1,9 @@ +class CreateWebHooks < ActiveRecord::Migration + def change + create_table :web_hooks do |t| + t.string :url + t.integer :project_id + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 613b65cb..1a31cd8f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20111207211728) do +ActiveRecord::Schema.define(:version => 20111214091851) do create_table "issues", :force => true do |t| t.string "title" @@ -132,4 +132,18 @@ ActiveRecord::Schema.define(:version => 20111207211728) do t.integer "project_access", :default => 0, :null => false end + create_table "web_hook_urls", :force => true do |t| + t.string "url" + t.integer "project_id" + t.datetime "created_at" + t.datetime "updated_at" + end + + create_table "web_hooks", :force => true do |t| + t.string "url" + t.integer "project_id" + t.datetime "created_at" + t.datetime "updated_at" + end + end diff --git a/spec/factories.rb b/spec/factories.rb index 6951ca2c..86e4d1e4 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -54,3 +54,7 @@ Factory.add(:key, Key) do |obj| obj.title = "Example key" obj.key = File.read(File.join(Rails.root, "db", "pkey.example")) end + +Factory.add(:web_hook, WebHook) do |obj| + obj.url = Faker::Internet.url +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index eda20a0c..0afb73f8 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -7,6 +7,7 @@ describe Project do it { should have_many(:issues) } it { should have_many(:notes) } it { should have_many(:snippets) } + it { should have_many(:web_hooks).dependent(:destroy) } end describe "Validation" do @@ -33,6 +34,7 @@ describe Project do it { should respond_to(:repo) } it { should respond_to(:tags) } it { should respond_to(:commit) } + it { should respond_to(:commits_between) } end it "should not allow 'gitolite-admin' as repo name" do @@ -50,6 +52,11 @@ describe Project do project.path_to_repo.should == File.join(Rails.root, "tmp", "tests", "somewhere") end + it "returns the full web URL for this repo" do + project = Project.new(:code => "somewhere") + project.web_url.should == "#{GIT_HOST['host']}/somewhere" + end + describe :valid_repo? do it "should be valid repo" do project = Factory :project @@ -62,6 +69,85 @@ describe Project do end end + describe "web hooks" do + let(:project) { Factory :project } + + context "with no web hooks" do + it "raises no errors" do + lambda { + project.execute_web_hooks('oldrev', 'newrev', 'ref') + }.should_not raise_error + end + end + + context "with web hooks" do + before do + @webhook = Factory(:web_hook) + @webhook_2 = Factory(:web_hook) + project.web_hooks << [@webhook, @webhook_2] + end + + it "executes multiple web hook" do + @webhook.should_receive(:execute).once + @webhook_2.should_receive(:execute).once + + project.execute_web_hooks('oldrev', 'newrev', 'ref') + end + end + + context "when gathering commit data" do + before do + @oldrev, @newrev, @ref = project.fresh_commits(2).last.sha, project.fresh_commits(2).first.sha, 'refs/heads/master' + @commit = project.fresh_commits(2).first + + # Fill nil/empty attributes + project.description = "This is a description" + + @data = project.web_hook_data(@oldrev, @newrev, @ref) + end + + subject { @data } + + it { should include(before: @oldrev) } + it { should include(after: @newrev) } + it { should include(ref: @ref) } + + context "with repository data" do + subject { @data[:repository] } + + it { should include(name: project.name) } + it { should include(url: project.web_url) } + it { should include(description: project.description) } + it { should include(homepage: project.web_url) } + it { should include(private: project.private?) } + end + + context "with commits" do + subject { @data[:commits] } + + it { should be_an(Array) } + it { should have(1).element } + + context "the commit" do + subject { @data[:commits].first } + + it { should include(id: @commit.id) } + it { should include(message: @commit.safe_message) } + it { should include(timestamp: @commit.date.xmlschema) } + it { should include(url: "http://localhost/#{project.code}/commits/#{@commit.id}") } + + context "with a author" do + subject { @data[:commits].first[:author] } + + it { should include(name: @commit.author_name) } + it { should include(email: @commit.author_email) } + end + end + end + + end + end + describe "updates" do let(:project) { Factory :project } @@ -107,6 +193,21 @@ describe Project do it { project.fresh_commits.last.id.should == "0dac878dbfe0b9c6104a87d65fe999149a8d862c" } end + describe "commits_between" do + let(:project) { Factory :project } + + subject do + commits = project.commits_between("a6d1d4aca0c85816ddfd27d93773f43a31395033", + "2fb376f61875b58bceee0492e270e9c805294b1a") + commits.map { |c| c.id } + end + + it { should have(2).elements } + it { should include("2fb376f61875b58bceee0492e270e9c805294b1a") } + it { should include("4571e226fbcd7be1af16e9fa1e13b7ac003bebdf") } + it { should_not include("a6d1d4aca0c85816ddfd27d93773f43a31395033") } + end + describe "Git methods" do let(:project) { Factory :project } diff --git a/spec/models/web_hook_spec.rb b/spec/models/web_hook_spec.rb new file mode 100644 index 00000000..e73e554a --- /dev/null +++ b/spec/models/web_hook_spec.rb @@ -0,0 +1,54 @@ +require 'spec_helper' + +describe WebHook do + describe "Associations" do + it { should belong_to :project } + end + + describe "Validations" do + it { should validate_presence_of(:url) } + + context "url format" do + it { should allow_value("http://example.com").for(:url) } + it { should allow_value("https://excample.com").for(:url) } + it { should allow_value("http://test.com/api").for(:url) } + it { should allow_value("http://test.com/api?key=abc").for(:url) } + it { should allow_value("http://test.com/api?key=abc&type=def").for(:url) } + + it { should_not allow_value("example.com").for(:url) } + it { should_not allow_value("ftp://example.com").for(:url) } + it { should_not allow_value("herp-and-derp").for(:url) } + end + end + + describe "execute" do + before(:each) do + @webhook = Factory :web_hook + @project = Factory :project + @project.web_hooks << [@webhook] + @data = { before: 'oldrev', after: 'newrev', ref: 'ref'} + + WebMock.stub_request(:post, @webhook.url) + end + + it "POSTs to the web hook URL" do + @webhook.execute(@data) + WebMock.should have_requested(:post, @webhook.url).once + end + + it "POSTs the data as JSON" do + json = @data.to_json + + @webhook.execute(@data) + WebMock.should have_requested(:post, @webhook.url).with(body: json).once + end + + it "catches exceptions" do + WebHook.should_receive(:post).and_raise("Some HTTP Post error") + + lambda { + @webhook.execute(@data) + }.should_not raise_error + end + end +end diff --git a/spec/requests/projects_tree_perfomance_spec.rb b/spec/requests/projects_tree_perfomance_spec.rb index 5cf93bf9..93876354 100644 --- a/spec/requests/projects_tree_perfomance_spec.rb +++ b/spec/requests/projects_tree_perfomance_spec.rb @@ -9,7 +9,6 @@ describe "Projects" do before do @project = Factory :project @project.add_access(@user, :read) - end it "should be fast" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 05fd6ca7..f24496ec 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -8,6 +8,7 @@ require 'rspec/rails' require 'capybara/rails' require 'capybara/rspec' require 'capybara/dsl' +require 'webmock/rspec' require 'factories' require 'monkeypatch' @@ -48,6 +49,8 @@ RSpec.configure do |config| end DatabaseCleaner.start + + WebMock.disable_net_connect!(allow_localhost: true) end config.after do diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb new file mode 100644 index 00000000..500a6998 --- /dev/null +++ b/spec/workers/post_receive_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe PostReceive do + + context "as a resque worker" do + it "reponds to #perform" do + PostReceive.should respond_to(:perform) + end + end + + context "web hooks" do + let(:project) { Factory :project } + + it "it retrieves the correct project" do + Project.should_receive(:find_by_path).with(project.path) + PostReceive.perform(project.path, 'sha-old', 'sha-new', 'refs/heads/master') + end + + it "asks the project to execute web hooks" do + Project.stub(find_by_path: project) + project.should_receive(:execute_web_hooks).with('sha-old', 'sha-new', 'refs/heads/master') + + PostReceive.perform(project.path, 'sha-old', 'sha-new', 'refs/heads/master') + end + end +end From 7ffb8fc616a2890cc46924b099c28709e491495e Mon Sep 17 00:00:00 2001 From: Ariejan de Vroom Date: Thu, 15 Dec 2011 10:33:20 +0100 Subject: [PATCH 5/7] Added specs for special cases We don't execute web hooks when: * You create a new branch. Make sure you first create the branch, and then push any commits. This is the way Github works, so its expected behavior. * When tags are pushed. --- app/models/project.rb | 5 +++++ spec/models/project_spec.rb | 23 ++++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index e14c1ebf..ab21f4da 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -92,6 +92,11 @@ class Project < ActiveRecord::Base end def execute_web_hooks(oldrev, newrev, ref) + ref_parts = ref.split('/') + + # Return if this is not a push to a branch (e.g. new commits) + return if ref_parts[1] !~ /heads/ || oldrev == "00000000000000000000000000000000" + data = web_hook_data(oldrev, newrev, ref) web_hooks.each { |web_hook| web_hook.execute(data) } end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 0afb73f8..242461d0 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -91,10 +91,31 @@ describe Project do @webhook.should_receive(:execute).once @webhook_2.should_receive(:execute).once - project.execute_web_hooks('oldrev', 'newrev', 'ref') + project.execute_web_hooks('oldrev', 'newrev', 'refs/heads/master') end end + context "does not execute web hooks" do + before do + @webhook = Factory(:web_hook) + project.web_hooks << [@webhook] + end + + it "when pushing a branch for the first time" do + @webhook.should_not_receive(:execute) + project.execute_web_hooks('00000000000000000000000000000000', 'newrev', 'refs/heads/mster') + end + + it "when pushing tags" do + @webhook.should_not_receive(:execute) + project.execute_web_hooks('oldrev', 'newrev', 'refs/tags/v1.0.0') + end + end + + context "when pushing new branches" do + + end + context "when gathering commit data" do before do @oldrev, @newrev, @ref = project.fresh_commits(2).last.sha, project.fresh_commits(2).first.sha, 'refs/heads/master' From be6e52c2f676927a373a2225f22f63120020f8cd Mon Sep 17 00:00:00 2001 From: Ariejan de Vroom Date: Mon, 26 Dec 2011 10:10:21 +0100 Subject: [PATCH 6/7] Fixed typo in spec mster => master --- spec/models/project_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 242461d0..444e49a6 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -103,7 +103,7 @@ describe Project do it "when pushing a branch for the first time" do @webhook.should_not_receive(:execute) - project.execute_web_hooks('00000000000000000000000000000000', 'newrev', 'refs/heads/mster') + project.execute_web_hooks('00000000000000000000000000000000', 'newrev', 'refs/heads/master') end it "when pushing tags" do From 5ca836048d4f3319d24d2d4580912102eed54618 Mon Sep 17 00:00:00 2001 From: Ariejan de Vroom Date: Mon, 26 Dec 2011 10:12:09 +0100 Subject: [PATCH 7/7] Use URI::regexp for validating WebHook urls --- app/models/web_hook.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/web_hook.rb b/app/models/web_hook.rb index 96684d15..0058bd57 100644 --- a/app/models/web_hook.rb +++ b/app/models/web_hook.rb @@ -9,7 +9,7 @@ class WebHook < ActiveRecord::Base validates :url, presence: true, format: { - with: /(^$)|(^(http|https):\/\/[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(([0-9]{1,5})?\/.*)?$)/ix, + with: URI::regexp(%w(http https)), message: "should be a valid url" } def execute(data)