From 2ff811ed72a1fa3c56fc245c619b973684b8c724 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Thu, 2 Mar 2017 16:58:00 +0100
Subject: [PATCH] Update after review
---
internal/builds/register.go | 19 +++++++------------
internal/builds/register_test.go | 26 ++++++++++++--------------
internal/helper/helpers.go | 1 +
3 files changed, 20 insertions(+), 26 deletions(-)
diff --git a/internal/builds/register.go b/internal/builds/register.go
index 00ea4f1e01375..4c902912c1883 100644
--- a/internal/builds/register.go
+++ b/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)
diff --git a/internal/builds/register_test.go b/internal/builds/register_test.go
index f28068d1b3544..519738a31bbc9 100644
--- a/internal/builds/register_test.go
+++ b/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)
}
diff --git a/internal/helper/helpers.go b/internal/helper/helpers.go
index d039d49bda6f8..72c37c8a1d3ef 100644
--- a/internal/helper/helpers.go
+++ b/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
}
--
GitLab