Mock api #185
Mock api #185
Conversation
# Conflicts: # pkg/api/server/rest/v1/systems.go # pkg/definition/resolver/system_resolver.go
pkg/api/server/mock/backend.go
Outdated
| } | ||
| } | ||
|
|
||
| type SystemRecord struct { |
There was a problem hiding this comment.
this struct can probably be not exported type systemRecord struct
There was a problem hiding this comment.
additionally, while probably not too big of a deal, maps and slices aren't thread safe in go, I'd probably include a RWMutex with the systemRecord and just lock it appropriately on access
pkg/api/server/mock/backend.go
Outdated
| } | ||
| // validate version | ||
| if v != "1.0.0" { | ||
| return nil, fmt.Errorf("bad version: %v", v) |
There was a problem hiding this comment.
i think this should be a v1.InvalidSystemVersionError
pkg/api/server/mock/backend.go
Outdated
| func (backend *MockBackend) RunJob(systemID v1.SystemID, path tree.NodePath, command []string, | ||
| environment definitionv1.ContainerEnvironment, | ||
| ) (*v1.Job, error) { | ||
| return nil, nil |
There was a problem hiding this comment.
probably want to support jobs at some point. maybe put a TODO or implement?
pkg/api/server/mock/backend.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (backend *MockBackend) getDeployForBuild(buildID v1.BuildID) *v1.Deploy { |
There was a problem hiding this comment.
in practice build -> deploy is a one -> many relationship. do we need to support that in this mock?
pkg/api/server/mock/backend.go
Outdated
| return build | ||
| } | ||
|
|
||
| func (backend *MockBackend) runBuild(build *v1.Build) { |
There was a problem hiding this comment.
i think the semantics are a bit off here.
firstly, a build can be run without an accompanying deploy
build state machine looks like this:
pending -> running -----------------
|
[ n container builds]
/ \
/ \
/ \
[all succeeded] [one or more failed]
| |
succeeded failed
deploy state machine looks like this:
pending -> accepted -----------------
|
[build]
/ \
/ \
/ \
[succeeded] [failed]
| |
in progress --> failed
|
succeeded
sorry i know this isn't well (or at all) documented. happy to talk it over
| @@ -0,0 +1,427 @@ | |||
| package mock | |||
There was a problem hiding this comment.
tests look good overall.
i think at some point we might want to extract this out to be the end to end testing framework (i implemented a very shitty version of an e2e testing framework in e2e)
could be nice to have a pluggable testing framework that you just point at an api server and it'll run a similar set of tests, but with configurable timeouts, system definition urls, checks to ensure things worked, etc.
anyways, a task for another day :)
There was a problem hiding this comment.
ah one other thing though, I think we should use this test to ensure that proper error codes are being returned for expected errors.
i think that right now the api-server will return a 500 for pretty much anything that's not happy path. this may be a good opportunity to start righting that
There was a problem hiding this comment.
Not sure i fully understand comment about error code. Lets talk.
# Conflicts: # cmd/kubernetes/api-server/rest/app/BUILD.bazel # cmd/kubernetes/api-server/rest/app/root.go # pkg/api/server/rest/server.go # pkg/api/server/rest/v1/handlers.go # pkg/api/server/rest/v1/systems.go # pkg/api/v1/errors.go # pkg/definition/resolver/BUILD.bazel # pkg/definition/resolver/system_resolver.go
# Conflicts: # cmd/kubernetes/api-server/rest/app/BUILD.bazel # cmd/kubernetes/api-server/rest/app/root.go # pkg/api/server/rest/server.go # pkg/api/server/rest/v1/handlers.go # pkg/api/server/rest/v1/systems.go # pkg/api/v1/errors.go # pkg/definition/resolver/BUILD.bazel # pkg/definition/resolver/system_resolver.go
# Conflicts: # pkg/definition/resolver/BUILD.bazel
|
Added mock/test for logs and jobs |
pkg/api/server/mock/backend.go
Outdated
| } | ||
|
|
||
| func getDeployStateByBuild(buildState v1.BuildState) v1.DeployState { | ||
| switch buildState { |
There was a problem hiding this comment.
still a bit of a mismatch here:
currently the semantics are:
- there can only ever be one deploy per system at a time that is in accepted or in-progress
- a deploy is first set into pending
- a lock is grabbed, and if there aren't any other deploys in accepted or in-progress, the deploy can move to accepted. if there are, the deploy moves to failed
- the deploy then stays in accepted until its build has succeeded or failed. if the build fails, the deploy moves to failed. if the build succeeds, the deploy moves from accepted to in progress
- the deploy can then succeed or fail
sorry my prior explanations were much more confusing. does this make sense?
this is also subject to change in the future, but i'll be happy to update this to match those semantics as they are introduced
pkg/definition/resolver/interface.go
Outdated
| @@ -0,0 +1,12 @@ | |||
| package resolver | |||
There was a problem hiding this comment.
this should be removed now i think
pkg/api/server/mock/backend.go
Outdated
| v v1.SystemVersion) (*v1.Build, error) { | ||
|
|
||
| record, err := backend.getSystemRecord(systemID) | ||
| record.recordLock.Lock() |
There was a problem hiding this comment.
this will panic if err != nil, should check err first
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // ensure that build exists |
There was a problem hiding this comment.
do you need to acquire the system lock here?
Makefile
Outdated
| @@ -106,6 +106,7 @@ git.install-hooks: | |||
|
|
|||
| # docker | |||
| DOCKER_IMAGES := kubernetes-api-server-rest \ | |||
| mock-api-server \ | |||
There was a problem hiding this comment.
let's keep this alphabetical, so move this to the end
docker/BUILD
Outdated
| @@ -71,6 +71,12 @@ lattice_go_container_image( | |||
| path = "cmd/kubernetes/api-server/rest", | |||
| ) | |||
|
|
|||
| lattice_go_container_image( | |||
There was a problem hiding this comment.
move this to the end too
#157