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

app/controllers/projects/hooks_controller.rb

@@ -27,8 +27,10 @@ def test
     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

app/models/hooks/web_hook.rb

@@ -38,7 +38,7 @@ def execute(data, hook_name)
                               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]

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

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

spec/models/hooks/web_hook_spec.rb

@@ -95,13 +95,13 @@
     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