Remove primary Geo system hook

When a primary Geo node is created, repository events are added to the system
hook. With each push to primary, the primary also receives an event,
which causes unnecessary work.

Closes #1752

app/models/geo_node.rb

@@ -130,8 +130,7 @@ class GeoNode < ActiveRecord::Base
 
   def build_dependents
     unless persisted?
-      self.build_geo_node_key if geo_node_key.nil?
-      update_system_hook!
+      self.build_geo_node_key unless geo_node_key.present?
     end
   end
 
@@ -141,8 +140,6 @@ class GeoNode < ActiveRecord::Base
     if self.primary?
       self.oauth_application = nil
       update_clone_url
-      self.system_hook.push_events = false
-      self.system_hook.tag_push_events = false
     else
       update_oauth_application!
       update_system_hook!
@@ -169,6 +166,8 @@ class GeoNode < ActiveRecord::Base
   end
 
   def update_system_hook!
+    return if self.primary?
+
     self.build_system_hook if system_hook.nil?
     self.system_hook.token = SecureRandom.hex(20) unless self.system_hook.token.present?
     self.system_hook.url = geo_events_url if uri.present?

db/migrate/20170419063751_remove_push_events_from_geo_primary_system_hook.rb → db/migrate/20170620063751_remove_geo_primary_system_hook.rb

-class RemovePushEventsFromGeoPrimarySystemHook < ActiveRecord::Migration
+class RemoveGeoPrimarySystemHook < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
   DOWNTIME = false
 
-  # Older version of Geo added push and tag push events to the
-  # primary system hook. This would cause unnecessary hooks to be
-  # fired.
+  # Older versions of GitLab added a system hook for the primary Geo node.
+  # This would cause all events to be fired for the primary as well.
   def up
     return unless geo_enabled?
 
     execute <<-SQL
-      UPDATE web_hooks
-      SET push_events = false, tag_push_events = false WHERE
+      DELETE FROM web_hooks WHERE
       type = 'SystemHook' AND
-      id = (
+      id IN (
       SELECT system_hook_id FROM geo_nodes WHERE
-        host = #{quote(Gitlab.config.gitlab.host)} AND
-        port = #{quote(Gitlab.config.gitlab.port)} AND
-        relative_url_root = #{quote(Gitlab.config.gitlab.relative_url_root)});
+        "primary" = #{true_value}
+      );
     SQL
   end
 

spec/models/geo_node_spec.rb

 require 'spec_helper'
 
 describe GeoNode, type: :model do
-  subject(:new_node) { described_class.new(schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab') }
-  subject(:new_primary_node) { described_class.new(schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab', primary: true) }
+  subject(:new_node) { create(:geo_node, schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab', primary: false) }
+  subject(:new_primary_node) { create(:geo_node, schema: 'https', host: 'localhost', port: 3000, relative_url_root: 'gitlab', primary: true) }
   subject(:empty_node) { described_class.new }
-  subject(:primary_node) { FactoryGirl.create(:geo_node, :primary) }
-  subject(:node) { FactoryGirl.create(:geo_node) }
+  subject(:primary_node) { create(:geo_node, :primary) }
+  subject(:node) { create(:geo_node) }
 
   let(:dummy_url) { 'https://localhost:3000/gitlab' }
   let(:url_helpers) { Gitlab::Application.routes.url_helpers }
@@ -47,16 +47,15 @@ describe GeoNode, type: :model do
   end
 
   context 'system hooks' do
-    it 'primary creates a system hook with no push events' do
-      hook = primary_node.system_hook
-      expect(hook.push_events).to be_falsey
-      expect(hook.tag_push_events).to be_falsey
+    it 'primary does not create a system hook' do
+      expect(primary_node.system_hook).to be_nil
     end
 
-    it 'secondary creates a system hook with push events' do
+    it 'secondary creates a system hook with repository update events' do
       hook = new_node.system_hook
-      expect(hook.push_events).to be_truthy
-      expect(hook.tag_push_events).to be_truthy
+      expect(hook.push_events).to be_falsey
+      expect(hook.tag_push_events).to be_falsey
+      expect(hook.repository_update_events).to be_truthy
     end
   end
 
@@ -165,6 +164,10 @@ describe GeoNode, type: :model do
     end
 
     context 'when required fields are not filled' do
+      it 'returns an initialized Geo node key' do
+        expect(empty_node.geo_node_key).not_to be_nil
+      end
+
       it 'returns an URI object' do
         expect(empty_node.uri).to be_a URI
       end

spec/requests/api/geo_spec.rb

@@ -6,12 +6,13 @@ describe API::Geo, api: true do
   let(:admin) { create(:admin) }
   let(:user) { create(:user) }
   let!(:primary_node) { create(:geo_node, :primary) }
+  let!(:secondary_node) { create(:geo_node) }
   let(:geo_token_header) do
-    { 'X-Gitlab-Token' => primary_node.system_hook.token }
+    { 'X-Gitlab-Token' => secondary_node.system_hook.token }
   end
 
   before(:each) do
-    allow(Gitlab::Geo).to receive(:current_node) { primary_node }
+    allow(Gitlab::Geo).to receive(:current_node) { secondary_node }
   end
 
   describe 'POST /geo/receive_events authentication' do
@@ -30,7 +31,7 @@ describe API::Geo, api: true do
 
   describe 'POST /geo/refresh_wikis disabled node' do
     it 'responds with forbidden' do
-      primary_node.enabled = false
+      secondary_node.enabled = false
 
       post api('/geo/refresh_wikis', admin), nil
 
@@ -40,7 +41,7 @@ describe API::Geo, api: true do
 
   describe 'POST /geo/receive_events disabled node' do
     it 'responds with forbidden' do
-      primary_node.enabled = false
+      secondary_node.enabled = false
 
       post api('/geo/receive_events'), nil, geo_token_header