Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MYST-99 bootstrap client api #52

Merged
merged 12 commits into from Dec 6, 2017

Conversation

Projects
None yet
4 participants
@tadovas
Copy link
Member

commented Nov 30, 2017

No description provided.

@tadovas tadovas requested review from ignasbernotas, interro and Waldz Nov 30, 2017

@Waldz Waldz changed the title Feature/myst 99 bootstrap client api MYST-99 bootstrap client api Dec 1, 2017


func doGet(t *testing.T, path string) *http.Response {

resp, err := http.Get("http://localhost" + serverBindAddress + "/" + path)

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 1, 2017

Member

I would use 127.0.0.1 instead of localhost since some environments may not have localhost -> 127.0.0.1 mapping.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 1, 2017

Author Member

This is just a test - server is started on localhost, and client acess it during part of testing.

)

func TestHealthCheckReturnsExpectedJsonObject(t *testing.T) {

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 1, 2017

Member

Unneeded whitespace?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 5, 2017

Author Member

spaces removed

writer.Header().Add("Content-type", "charset=utf-8")
_, writeErr := writer.Write(serialized)
if writeErr != nil {
http.Error(writer, "Unable write to writer (funny :D)", http.StatusInternalServerError)

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 1, 2017

Contributor

doesn't this actually send the http response?
if it does, error should say 'failed to send http response'

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 5, 2017

Author Member

fixed

@@ -68,6 +69,8 @@ func (cmd *CommandRun) Run(options CommandOptions) (err error) {
return err
}

go client_local_api.Bootstrap(options.LocalApiAddress)

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 1, 2017

Contributor

for me Bootstrap relates to a configuration routine that runs once before actual business logic could run.
so I'd rather have client_local_api.Boostrap() and go client_local_api.Run(options.LocalApiAddress) here.

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

Yeah could be some DI approach:

  • factory factories/bootstraps TequilapiServer to be sure that everything is OK
  • here just TequilapiServer.Start()

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

I am kinda missing control if of subprocess status:

  • Start() error
  • Wait() error
  • main programs combines waiting of 2 subroutines. see cmd/mysterium_fake/mysterium_fake.go.

func TestMain(m *testing.M) {
startServer() //start a single server before all tests
os.Exit(m.Run())

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

I cant understand.. What is it?

}

func TestHealthCheckReturnsExcpectedResponse(t *testing.T) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

What are these empty lines for?

return resp
}

func expectJsonAndStatus(t *testing.T, resp *http.Response, httpStatus int, v interface{}) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

I would have 2 functions for different responsibilities:

  • expectJsonAndStatus(t, response, 200)
  • expectJsonBody(t, response, '{"status":"ok"}') <-- maybe httptest library already has JSON assertions

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

I had such helper:

func assertResponseJson(t *testing.T, response *httptest.ResponseRecorder, expectedJsonFile string) {
	expectedResponse, err := filesystem_helper.ReadFile(expectedJsonFile)
	if err != nil {
		t.Fatalf("Error reading file: %s. %s", expectedJsonFile, err.Error())
	}

	assert.JSONEq(t, string(expectedResponse), response.Body.String())
}
@@ -0,0 +1,38 @@
package client_local_api

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

client_local_api -> tequilapi

Bootstrap function starts http server on specified binding address, which format conforms to what is expeted by
http.ListenAndServe function. This function IS BLOCKING, that means - it should be run on goroutine to resume with normal flow
*/
func Bootstrap(bindAddress string) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

Are You sure it's enough to have global functions in package.
Having wrapper like would make testing easier:

TekilaServer {
    host string
    port int
    routes []Handler
} 

HealthCheckEndpoint(resp, req)

assert.JSONEq(t, `

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

Lets have similar JSON formatting convention.

See openvpn/service_discovery/dto/service_definition_test.go Currently it was:

assert.JSONEq(
    t,
    `{
        "uptime" : "1m0s"
    }`,
    resp.Body.String(),
)
*/
func HealthCheckEndpoint(writer http.ResponseWriter, request *http.Request) {
status := healthCheckData{
Uptime: currentTime().Sub(startupTime).String(),

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

It got me some time to understand this intermediate variable
startupTime -> time.Now() looks more simple

result := respRecorder.Result()

assert.Equal(t, "application/json", result.Header.Get("Content-type"))
assert.JSONEq(t, `

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member

same here

@@ -24,6 +25,12 @@ func ParseArguments(args []string) (options CommandOptions, err error) {
".",
"Runtime directory for temp files (should be writable)",
)
flags.StringVar(
&options.LocalApiAddress,
"localApi.port",

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member
  • tekilapi.host - string, defaults to "localhost"
  • tekilapi.port - int, defaults to 8000
@@ -24,6 +25,12 @@ func ParseArguments(args []string) (options CommandOptions, err error) {
".",
"Runtime directory for temp files (should be writable)",
)
flags.StringVar(
&options.LocalApiAddress,
"localApi.port",

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 1, 2017

Member
  • tequilapi.host - string, default to "localhost"
  • tequilapi.port - string, default to 4050
@tadovas

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2017

As it was heavily reworked a new review is needed.

@tadovas tadovas self-assigned this Dec 5, 2017

return nil
}

func (cmd *CommandRun) Wait() error {
cmd.httpApiServer.Wait()

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 5, 2017

Member

This wait makes cmd.vpnClient.Wait() statement unreachable.
See in cmd/mysterium_fake/mysterium_fake.go how to handle 2 concurrent waits

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

Will fix

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

Rewritten with error channels and error returning on it.

boundPort int
}

func NewServer(address string, port int, handler http.Handler) (ApiServer, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 5, 2017

Member

What about splitting 2 different responsibilities:

  • NewServer factories ApiServer struct
  • ApiServer.Start() - starts server

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

Will think about it

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

Renamed to StartNewServer to reflect function intent.

server, err := NewServer("", 31337, nil)
assert.Nil(t, err)

assert.Equal(t, 31337, server.Port())

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 5, 2017

Member

You dont need a getter method, because You are testing in same package

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

It's just considence. Port() method is part of interface and it was not intended just for testing.

return nil, err
}

boundPort, err := extractBoundPort(listener)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 5, 2017

Member

Why do we need port extracting? It's here in port variable

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

It covers a case when 0 port can be passed for listening, in that case OS will pickup random free port and bind on it. To avoid if port == 0 then do extract, this flow simply covers both scenarios.

assert.JSONEq(
t,
`{
"IntField" : 1,

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 5, 2017

Member

Looks misformatted

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

Will fix

@Waldz
Copy link
Member

left a comment

  • router looks awesome
  • concurrency in cmd.vpnClient.Wait() is most important
  • other questions are minor
}

func (server *apiServer) Stop() {
server.listener.Close()

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 6, 2017

Contributor

don't we need to do Server.Shutdown here?
https://golang.org/pkg/net/http/#Server.Shutdown

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

We are not using http.Server here, therefore shutdown is not applicable. It's enough to close net.listener

wg.Add(1)

server := apiServer{listener, &wg, boundPort}
go serve(listener, handler, &wg)

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 6, 2017

Contributor

I think if we make Serve a public method, then we could get rid of some of this confusing sync.WaitGroups

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

serve method is internal implementation and is not needed to be public, it is also a blocking function that's why is started in separate go routine, and in order to find out when server finished serving requests, waitgroup synchronization is used.

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 6, 2017

Contributor

but do we really need two syncGroups here?
My thinking is that server.stopped could be enough if we separate server construction and running.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 6, 2017

Author Member

I only see one syncGroup here.

}

func (server *apiServer) Wait() {
server.stopped.Wait()

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 6, 2017

Contributor

I don't quite understand how to use this Wait logic. what are we waiting for and why?

This comment has been minimized.

Copy link
@tadovas
@tadovas

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2017

All issues have been reviewed and fixed/commented

@Waldz

Waldz approved these changes Dec 6, 2017

@Waldz Waldz merged commit 16705e4 into master Dec 6, 2017

@Waldz Waldz deleted the feature/MYST-99-bootstrap-client-api branch Dec 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.