Merge branch 'hook-docs-behavior' into 'master'

Improve documentation and web test for web hooks

Tips and documentation of actual hook behavior. Improved user feedback
when testing hooks via the web UI.

See merge request !4015

5 files changed, 21 insertions, 6 deletions

diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index dfa9bd259e8..47524b1cf0b 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/app/controllers/projects/hooks_controller.rb
@@ -27,8 +27,10 @@ class Projects::HooksController < Projects::ApplicationController
     if !@project.empty_repo?
       status, message = TestHookService.new.execute(hook, current_user)
 
-      if status
-        flash[:notice] = 'Hook successfully executed.'
+      if status && status >= 200 && status < 400
+        flash[:notice] = "Hook executed successfully: HTTP #{status}"
+      elsif status
+        flash[:alert] = "Hook executed successfully but returned HTTP #{status} #{message}"
       else
         flash[:alert] = "Hook execution failed: #{message}"
       end
diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index fde05f729dc..8b87b6c3d64 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -38,7 +38,7 @@ class WebHook < ActiveRecord::Base
                               basic_auth: auth)
     end
 
-    [(response.code >= 200 && response.code < 300), ActionView::Base.full_sanitizer.sanitize(response.to_s)]
+    [response.code, response.to_s]
   rescue SocketError, OpenSSL::SSL::SSLError, Errno::ECONNRESET, Errno::ECONNREFUSED, Net::OpenTimeout => e
     logger.error("WebHook Error => #{e}")
     [false, e.to_s]
diff --git a/doc/web_hooks/web_hooks.md b/doc/web_hooks/web_hooks.md
index c1c51302e79..45506ac1d7c 100644
--- a/doc/web_hooks/web_hooks.md
+++ b/doc/web_hooks/web_hooks.md
@@ -13,6 +13,19 @@ You can configure webhooks to listen for specific events like pushes, issues or
 
 Webhooks can be used to update an external issue tracker, trigger CI builds, update a backup mirror, or even deploy to your production server.
 
+## Webhook endpoint tips
+
+If you are writing your own endpoint (web server) that will receive
+GitLab webhooks keep in mind the following things:
+
+-   Your endpoint should send its HTTP response as fast as possible. If
+    you wait too long, GitLab may decide the hook failed and retry it.
+-   Your endpoint should ALWAYS return a valid HTTP response. If you do
+    not do this then GitLab will think the hook failed and retry it.
+    Most HTTP libraries take care of this for you automatically but if
+    you are writing a low-level hook this is important to remember.
+-   GitLab ignores the HTTP status code returned by your endpoint.
+
 ## SSL Verification
 
 By default, the SSL certificate of the webhook endpoint is verified based on
diff --git a/features/steps/project/hooks.rb b/features/steps/project/hooks.rb
index b1ffe7f7b4c..13c0713669a 100644
--- a/features/steps/project/hooks.rb
+++ b/features/steps/project/hooks.rb
@@ -59,7 +59,7 @@ class Spinach::Features::ProjectHooks < Spinach::FeatureSteps
   step 'hook should be triggered' do
     expect(current_path).to eq namespace_project_hooks_path(current_project.namespace, current_project)
     expect(page).to have_selector '.flash-notice',
-                              text: 'Hook successfully executed.'
+                              text: 'Hook executed successfully: HTTP 200'
   end
 
   step 'I should see hook error message' do
diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index 37a27d73aab..f9bab487b96 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/spec/models/hooks/web_hook_spec.rb
@@ -95,13 +95,13 @@ describe WebHook, models: true do
     it "handles 200 status code" do
       WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: "Success")
 
-      expect(project_hook.execute(@data, 'push_hooks')).to eq([true, 'Success'])
+      expect(project_hook.execute(@data, 'push_hooks')).to eq([200, 'Success'])
     end
 
     it "handles 2xx status codes" do
       WebMock.stub_request(:post, project_hook.url).to_return(status: 201, body: "Success")
 
-      expect(project_hook.execute(@data, 'push_hooks')).to eq([true, 'Success'])
+      expect(project_hook.execute(@data, 'push_hooks')).to eq([201, 'Success'])
     end
   end
 end