Skip to content
This repository has been archived by the owner on Feb 28, 2019. It is now read-only.

Initial main #1

Merged
merged 8 commits into from
Jun 7, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,4 @@ env:
sudo: required
dist: trusty
script:
- make lint
- make test-ci-unit
- make test-ci-integration
- make m3ctl
- make all
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a newline here?

3 changes: 3 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ services-linux-amd64:

$(foreach SERVICE,$(SERVICES),$(eval $(SERVICE_RULES)))

all: lint test-ci-unit services
@echo Made all successfully
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll fail fairly loudly if any of the build targets fail so don't think you actually need the echo here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do this in other places, its nice positive reenforcement.


lint:
@which golint > /dev/null || go get -u github.com/golang/lint/golint
$(lint_check)
Expand Down
160 changes: 160 additions & 0 deletions glide.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions glide.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package: github.com/m3db/m3ctl

import:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe copy the packages (and their corresponding versions) that are in glide.lock but aren't in glide.yaml into glide.yaml so they don't change when someone runs glide update?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for direct dependencies, not transitive ones, yeah?

- package: github.com/m3db/m3cluster
version: d64606edb68599677fd335ceed86d990d13d2991
- package: github.com/m3db/m3metrics
version: 18bc01a7057d55b78e528236e0e9db297bcd4fa3
- package: github.com/m3db/m3x
version: bd6be18ffb184691512eb442561b16a59c15d797
- package: github.com/stretchr/testify
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could mark this under TestImports. For example:

testImport:
- package: github.com/stretchr/testify
  version: 6fe211e493929a8aac0469b93f28b1d0688a9a3a

version: 6fe211e493929a8aac0469b93f28b1d0688a9a3a
60 changes: 60 additions & 0 deletions handler/common.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright (c) 2017 Uber Technologies, Inc.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE

package handler

import (
"encoding/json"
"fmt"
"net/http"
)

const (
mimeTypeApplicationJSON = "application/json"
)

// NotSupportdResponse is data for a method not supported response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/NotSupportdResponse/NotSupportedResponse

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments should be sentences and end with period.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also should it be called MethodNotSupportedResponse?

type NotSupportdResponse struct {
Error string
MethodName string
URI string
SupportedMethods []string
}

// MethodNotSupportedResponse writes a method not supported response to the writer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps add periods to the end of these comments. It's the comment convention recommended in the Go wiki and we've been trying to migrate code to use it, though we're definitely far from that mark :) Can start clean here at least

func MethodNotSupportedResponse(w http.ResponseWriter, r *http.Request, supported []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like this function should be broken into two parts, first part is constructing the response, and second part is to write out the response, which IMO is a clearer separation of concerns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing the response is just w.Write(body). You want that in its own function?

w.WriteHeader(http.StatusMethodNotAllowed)
response := NotSupportdResponse{
Error: "Method not supported",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it a const?

URI: r.RequestURI,
MethodName: r.Method,
SupportedMethods: supported}
body, err := json.Marshal(response)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps just return the error itself or include it in the error message?

return fmt.Errorf("Could not generate MethodNotSupportedResponse for %s - %s", r.Method, r.RequestURI)
}
w.Write(body)
return nil
}

// InitJSONResponse sets common headers for m3ctl on a ResponseWriter
func InitJSONResponse(w http.ResponseWriter) {
w.Header().Set("Content-Type", mimeTypeApplicationJSON)
}
62 changes: 62 additions & 0 deletions handler/common_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE

package handler

import (
"encoding/json"
"net/http"
"net/http/httptest"
"testing"

"github.com/stretchr/testify/require"
)

func TestMethodNotSupportedResponse(t *testing.T) {
rr := httptest.NewRecorder()
goodMethods := []string{http.MethodDelete, http.MethodConnect}

// Create a request to pass to our handler. We don't have any query parameters for now, so we'll
// pass 'nil' as the third parameter.
req := http.Request{Method: "GET", RequestURI: "/badMethod"}

handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
MethodNotSupportedResponse(w, r, goodMethods)
})

// Our handlers satisfy http.Handler, so we can call their ServeHTTP method
// directly and pass in our Request and ResponseRecorder.
handler.ServeHTTP(rr, &req)

rawResult := make([]byte, rr.Body.Len())
_, err := rr.Body.Read(rawResult)
require.NoError(t, err, "Encountered error parsing reponse")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/reponse/response


var actualResult NotSupportdResponse
json.Unmarshal(rawResult, &actualResult)

expectedResult := NotSupportdResponse{
Error: "Method not supported",
URI: "/badMethod",
MethodName: "GET",
SupportedMethods: goodMethods,
}

require.Equal(t, rr.Code, http.StatusMethodNotAllowed)
require.Equal(t, expectedResult, actualResult)
}
29 changes: 29 additions & 0 deletions handler/controller.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright (c) 2017 Uber Technologies, Inc.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE

package handler

import "net/http"

// Controller defines functions around a controllable entity
type Controller interface {
// Registers the handlers for this controll with the given ServeMux
Copy link
Contributor

@jeromefroe jeromefroe Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/controll/contoller

nit: comments should ideally start with the name of the thing that is being described. So perhaps "RegisterHandlers registers the handlers for this controller with the given ServeMux"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if Router might be a better name?

RegisterHandlers(*http.ServeMux) error
}
Loading