Update after review

internal/builds/register.go

@@ -31,15 +31,11 @@ var (
 		},
 		[]string{"state"},
 	)
-)
 
-var (
 	registerHandlerOpenAtReading  = registerHandlerOpen.WithLabelValues("reading")
 	registerHandlerOpenAtProxying = registerHandlerOpen.WithLabelValues("proxying")
 	registerHandlerOpenAtWatching = registerHandlerOpen.WithLabelValues("watching")
-)
 
-var (
 	registerHandlerBodyReadErrors     = registerHandlerHits.WithLabelValues("body-read-error")
 	registerHandlerBodyParseErrors    = registerHandlerHits.WithLabelValues("body-parse-error")
 	registerHandlerMissingValues      = registerHandlerHits.WithLabelValues("missing-values")
@@ -74,19 +70,18 @@ func readRunnerBody(w http.ResponseWriter, r *http.Request) ([]byte, error) {
 	return helper.ReadRequestBody(w, r, maxRegisterBodySize)
 }
 
-func readRunnerRequest(r *http.Request, body []byte) (runnerRequest, error) {
-	var runnerRequest runnerRequest
-
+func readRunnerRequest(r *http.Request, body []byte) (*runnerRequest, error) {
 	if !helper.IsApplicationJson(r) {
-		return runnerRequest, errors.New("invalid content-type received")
+		return nil, errors.New("invalid content-type received")
 	}
 
+	var runnerRequest runnerRequest
 	err := json.Unmarshal(body, &runnerRequest)
 	if err != nil {
-		return runnerRequest, err
+		return nil, err
 	}
 
-	return runnerRequest, nil
+	return &runnerRequest, nil
 }
 
 func proxyRegisterRequest(h http.Handler, w http.ResponseWriter, r *http.Request) {
@@ -141,7 +136,7 @@ func RegisterHandler(h http.Handler, watchHandler WatchKeyHandler, pollingDurati
 
 		switch result {
 		// It means that we detected a change before starting watching on change,
-		// We proxy request to Rails, to see whether we can receive the build
+		// We proxy request to Rails, to see whether we have a build to receive
 		case redis.WatchKeyStatusAlreadyChanged:
 			registerHandlerAlreadyChangedHits.Inc()
 			proxyRegisterRequest(h, w, newRequest)
@@ -150,7 +145,7 @@ func RegisterHandler(h http.Handler, watchHandler WatchKeyHandler, pollingDurati
 		// We could potentially proxy request to Rails, but...
 		// We can end-up with unreliable responses,
 		// as don't really know whether ResponseWriter is still in a sane state,
-		// whether the connection is not dead
+		// for example the connection is dead
 		case redis.WatchKeyStatusSeenChange:
 			registerHandlerSeenChangeHits.Inc()
 			w.WriteHeader(http.StatusNoContent)

internal/builds/register_test.go

@@ -20,8 +20,6 @@ func echoRequest(rw http.ResponseWriter, req *http.Request) {
 
 var echoRequestFunc = http.HandlerFunc(echoRequest)
 
-const applicationJson = "application/json"
-
 func expectHandlerWithWatcher(t *testing.T, watchHandler WatchKeyHandler, data string, contentType string, expectedHttpStatus int, msgAndArgs ...interface{}) {
 	h := RegisterHandler(echoRequestFunc, watchHandler, time.Second)
 
@@ -40,7 +38,7 @@ func expectHandler(t *testing.T, data string, contentType string, expectedHttpSt
 
 func TestRegisterHandlerLargeBody(t *testing.T) {
 	data := strings.Repeat(".", maxRegisterBodySize+5)
-	expectHandler(t, data, applicationJson, http.StatusRequestEntityTooLarge,
+	expectHandler(t, data, "application/json", http.StatusRequestEntityTooLarge,
 		"rejects body with entity too large")
 }
 
@@ -50,20 +48,20 @@ func TestRegisterHandlerInvalidRunnerRequest(t *testing.T) {
 }
 
 func TestRegisterHandlerInvalidJsonPayload(t *testing.T) {
-	expectHandler(t, "{[", applicationJson, http.StatusOK,
+	expectHandler(t, `{[`, "application/json", http.StatusOK,
 		"fails on parsing body and proxies request to upstream")
 }
 
 func TestRegisterHandlerMissingData(t *testing.T) {
-	dataList := []string{"{\"token\":\"token\"}", "{\"last_update\":\"data\"}"}
+	dataList := []string{`{"token":"token"}`, `{"last_update":"data"}`}
 
 	for _, data := range dataList {
-		expectHandler(t, data, applicationJson, http.StatusOK,
+		expectHandler(t, data, "application/json", http.StatusOK,
 			"fails on argument validation and proxies request to upstream")
 	}
 }
 
-func exceptWatcherToBeExecuted(t *testing.T, watchKeyStatus redis.WatchKeyStatus, watchKeyError error,
+func expectWatcherToBeExecuted(t *testing.T, watchKeyStatus redis.WatchKeyStatus, watchKeyError error,
 	httpStatus int, msgAndArgs ...interface{}) {
 	executed := false
 	watchKeyHandler := func(key, value string, timeout time.Duration) (redis.WatchKeyStatus, error) {
@@ -71,33 +69,33 @@ func exceptWatcherToBeExecuted(t *testing.T, watchKeyStatus redis.WatchKeyStatus
 		return watchKeyStatus, watchKeyError
 	}
 
-	parsableData := "{\"token\":\"token\",\"last_update\":\"last_update\"}"
+	parsableData := `{"token":"token","last_update":"last_update"}`
 
-	expectHandlerWithWatcher(t, watchKeyHandler, parsableData, applicationJson, httpStatus, msgAndArgs...)
+	expectHandlerWithWatcher(t, watchKeyHandler, parsableData, "application/json", httpStatus, msgAndArgs...)
 	assert.True(t, executed, msgAndArgs...)
 }
 
 func TestRegisterHandlerWatcherError(t *testing.T) {
-	exceptWatcherToBeExecuted(t, redis.WatchKeyStatusNoChange, errors.New("redis connection"),
+	expectWatcherToBeExecuted(t, redis.WatchKeyStatusNoChange, errors.New("redis connection"),
 		http.StatusOK, "proxies data to upstream")
 }
 
 func TestRegisterHandlerWatcherAlreadyChanged(t *testing.T) {
-	exceptWatcherToBeExecuted(t, redis.WatchKeyStatusAlreadyChanged, nil,
+	expectWatcherToBeExecuted(t, redis.WatchKeyStatusAlreadyChanged, nil,
 		http.StatusOK, "proxies data to upstream")
 }
 
 func TestRegisterHandlerWatcherSeenChange(t *testing.T) {
-	exceptWatcherToBeExecuted(t, redis.WatchKeyStatusSeenChange, nil,
+	expectWatcherToBeExecuted(t, redis.WatchKeyStatusSeenChange, nil,
 		http.StatusNoContent)
 }
 
 func TestRegisterHandlerWatcherTimeout(t *testing.T) {
-	exceptWatcherToBeExecuted(t, redis.WatchKeyStatusTimeout, nil,
+	expectWatcherToBeExecuted(t, redis.WatchKeyStatusTimeout, nil,
 		http.StatusNoContent)
 }
 
 func TestRegisterHandlerWatcherNoChange(t *testing.T) {
-	exceptWatcherToBeExecuted(t, redis.WatchKeyStatusNoChange, nil,
+	expectWatcherToBeExecuted(t, redis.WatchKeyStatusNoChange, nil,
 		http.StatusNoContent)
 }

internal/helper/helpers.go

@@ -197,6 +197,7 @@ func ReadRequestBody(w http.ResponseWriter, r *http.Request, maxBodySize int64)
 func CloneRequestWithNewBody(r *http.Request, body []byte) *http.Request {
 	newReq := *r
 	newReq.Body = ioutil.NopCloser(bytes.NewReader(body))
+	newReq.Header = HeaderClone(r.Header)
 	newReq.ContentLength = int64(len(body))
 	return &newReq
 }