Split implementations from generic code #12
Conversation
aledbf
force-pushed
the
merge-aledbf-ingress
branch
from
9b0e57d
to
ad05e85
Compare
aledbf
force-pushed
the
merge-aledbf-ingress
branch
from
24272a9
to
eb0a78c
Compare
aledbf
force-pushed
the
merge-aledbf-ingress
branch
from
eb0a78c
to
48d7836
Compare
aledbf
changed the title
aledbf
force-pushed
the
merge-aledbf-ingress
branch
from
48d7836
to
bea2b6f
Compare
aledbf
force-pushed
the
merge-aledbf-ingress
branch
from
bea2b6f
to
8931374
Compare
|
@bprashanth ping |
There was a problem hiding this comment.
This is really hard to review, the change is too big. Can you split the important bits (generic controller/interface) into another pr and keep this one for mostly the things that are remaining same. Nginx controller refactor to accomodate generic controller is fine, I'd just like to do a more thorough review of the interface between the generic controller and the backends.
| @@ -0,0 +1,22 @@ | |||
| package = "lua-resty-http" | |||
There was a problem hiding this comment.
what is this? dependency management for lua?
|
|
||
| data := map[string]string{} | ||
| ing.SetAnnotations(data) | ||
| /* |
There was a problem hiding this comment.
why is it commentd out?
|
|
||
| // Interface holds the methods to handle an Ingress backend | ||
| type Interface interface { | ||
| Start() |
| /* | ||
| Copyright 2015 The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); |
There was a problem hiding this comment.
can you move this into ingress/controller/generic/
with
README
interfaces.go
controller.go
tests...
etc?
| Start() | ||
| Stop() error | ||
|
|
||
| Info() string |
There was a problem hiding this comment.
I find it weird that all the interface methods don't take args, how are we communicating endpoint/service info to the backend?
| return ir < jr | ||
| } | ||
|
|
||
| // getUpstreamServers returns a list of Upstream and Server to be used by the backend |
There was a problem hiding this comment.
Please define what an upstream is somewhere, since only nginx really uses it
|
@bprashanth ok, #14 contains only changes to the gce controller (package names) and a godep update. After that I can continue with #15. |
Edit: this is the first step
fixes #4