deis / router

Edge router for Deis Workflow
https://deis.com
MIT License
80 stars 57 forks source link

Allow returning 200 for root requests #347

Closed tmc closed 7 years ago

tmc commented 7 years ago

This enables placing deis-router behind a GCE ingress which sends health-checks to /.

deis-admin commented 7 years ago

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

mboersma commented 7 years ago

This LGTM--thanks @tmc for doing this. @krancour could you take a look at this too?

krancour commented 7 years ago

I'm not opposed to this, but I'm not entirely clear on why you'd want to do this. The router is, essentially, an ingress controller (although it uses annotated services instead of actual ingress resources because it was created at a time before ingress was stable). Workflow also has support now for "bring your own ingress controller," and you wouldn't include a router in your Workflow installation if you used that option. What I'm not understanding is why one would opt to put an ingress controller in front of the router, since I see those two components as mutually exclusive.

mboersma commented 7 years ago

Jenkins, add to whitelist.

codecov-io commented 7 years ago

Codecov Report

Merging #347 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #347   +/-   ##
=======================================
  Coverage   56.47%   56.47%           
=======================================
  Files           6        6           
  Lines         425      425           
=======================================
  Hits          240      240           
  Misses        159      159           
  Partials       26       26
Impacted Files Coverage Δ
nginx/config.go 56.86% <ø> (ø) :arrow_up:
model/model.go 49.18% <ø> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0272d4b...5c3859c. Read the comment docs.

mboersma commented 7 years ago

Workflow also has support now for "bring your own ingress controller,"

True, but until some issues are fixed that support is incomplete.

why one would opt to put an ingress controller in front of the router...?

@tmc could you elaborate a bit on the use case for this?

mboersma commented 7 years ago

@tmc we still have some questions about the general usefulness of this change. I'm going to close it since it hasn't gotten attention in a while, but please re-open with some comments if I was too hasty.

Thanks again!