TranslateIngress changes #275
Conversation
k8s-ci-robot
added
cncf-cla: yes
nicksardo
force-pushed
the
controller
branch
4 times, most recently
from
3577266 to
19fed83
Compare
| @@ -150,6 +152,19 @@ func trimFieldsEvenly(max int, fields ...string) []string { | |||
| return ret | |||
| } | |||
|
|
|||
| // PrettyJson marshals an object in a human-friendly format. | |||
| func PrettyJson(data interface{}) (string, error) { | |||
There was a problem hiding this comment.
Maybe I'm missing something, but this doesn't seem to be used anywhere.
There was a problem hiding this comment.
Yea, but is a useful func to exist when we need to add more tests.
pkg/utils/gceurlmap.go
Outdated
| return &GCEURLMap{HostRules: make(map[string][]PathRule)} | ||
| } | ||
|
|
||
| func EqualStructure(a, b *GCEURLMap) bool { |
There was a problem hiding this comment.
Can we call this Compare() instead?
There was a problem hiding this comment.
It's only checking ServiceProjectID and Compare makes it sound like it's a full ServicePort check.
pkg/test/utils.go
Outdated
| } | ||
| } | ||
|
|
||
| // DecodeIngress returns deserializes an Ingress from yaml. |
There was a problem hiding this comment.
remove "returns" from comment.
| // hostRules is a mapping from hostnames to path rules for that host. | ||
| hostRules map[string][]PathRule | ||
| HostRules map[string][]PathRule |
There was a problem hiding this comment.
Any reason for exposing the host rules? I left them package private so that we never access it without using the public getter
There was a problem hiding this comment.
Didn't have an immediate need. Was a little confused why AllRules() is used over HostRules. I have no strong opinion on this... will make it private
There was a problem hiding this comment.
Ah, actually, we need exported for serializing/deserializing into JSON.
There was a problem hiding this comment.
Damn, okay. Let's get rid of AllRules() then too and just replace that with direct access.
pkg/utils/gceurlmap.go
Outdated
| for _, pathRule := range pathRules { | ||
| if _, ok := uniquePaths[pathRule.Path]; ok { | ||
| glog.V(4).Infof("Equal paths (%v) for host %v. Using backend %+v", pathRule.Path, hostname, pathRule.Backend) | ||
| seen := make(map[string]struct{}) |
There was a problem hiding this comment.
can the value for this just be a bool? Might read a little better.
| @@ -68,6 +63,127 @@ func fakeTranslator(negEnabled bool) *Translator { | |||
| return gce | |||
| } | |||
|
|
|||
| func ingressFromFile(filename string) *extensions.Ingress { | |||
There was a problem hiding this comment.
Nit: Can we move these helpers to the bottom of the file? I don't like them being the first thing I see when I open the file :)
| return v | ||
| } | ||
|
|
||
| func TestTranslateIngress(t *testing.T) { |
There was a problem hiding this comment.
Nice job on this test, looks awesome. One thing I would like to see is maybe a case that results in 2 or more errors.
pkg/controller/controller_test.go
Outdated
| func TestLbCreateDelete(t *testing.T) { | ||
| testFirewallName := "quux" | ||
| cm := NewFakeClusterManager(flags.DefaultClusterUID, testFirewallName) | ||
| func TestIngressSyncError(t *testing.T) { |
There was a problem hiding this comment.
Can we add a small comment to this test that is a little more descriptive than the function name itself? Same for other tests as well.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Changes
ToSvcPortsiterates all ingresses (for now) and will continue to collect all known service ports