search

albertrdixon / romulus

A kubernetes ingress controller
MIT License
103 stars 12 forks source link

big refactor #16

Closed albertrdixon closed 8 years ago

albertrdixon commented 8 years ago

Romulus got a little unwieldy mostly since it began life as a just a tiny utility and became a little larger than that.

at any rate this is a bit cleaner I think and deals with some of the ordering issues romulus was having.

soellman commented 8 years ago

I'm gonna comment here since the diff is huge and often involves complete files..

soellman commented 8 years ago

etcd.go - I would rename realEtcdClient to etcdClient, and move fakeEtcdClient into a test file.

soellman commented 8 years ago

logger.go - you might consider renaming L to F - it's clear they're all "level" by their function names, and it's no longer clear that they take format strings.

soellman commented 8 years ago

For docs - consider adding good comments on each of the funcs in registrar.go since that's where the meat is regarding vulcan integration. Also, maybe just describe what this does at a high level in doc.go.

soellman commented 8 years ago

Lastly, this looks way simpler and well designed, nice work!

Extra points challenge - make a simple interface for the LB we're managing and stick the vulcand implementation behind it. Not currently needed but I'd imagine that we could pretty easily manage other LBs like traefik or something else. Awesome!