deis / router

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

feat(ingress) Experimental Native Ingress #316

Closed krisnova closed 7 years ago

krisnova commented 7 years ago

Preliminary work for disabling the router chart when using EXPERIMENTAL_NATIVE_INGRESS

Note: this will probably be refactored once we figure out #771

Requires deis/builder#495 Requires deis/workflow#732 Requires deis/controller#1243

codecov-io commented 7 years ago

Codecov Report

Merging #316 into master will increase coverage by 3.78%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   55.69%   59.48%   +3.78%     
==========================================
  Files           6        6              
  Lines         386      543     +157     
==========================================
+ Hits          215      323     +108     
- Misses        151      189      +38     
- Partials       20       31      +11
Impacted Files Coverage Δ
model/model.go 54.16% <0%> (+7.82%) :white_check_mark:
nginx/config.go 64.93% <0%> (+8.07%) :white_check_mark:

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 2d6f130...018e469. Read the comment docs.

krancour commented 7 years ago

@kris-nova, I think I see a problem here...

.Values.global.experimental_native_ingress looks like a value from the "parent" Workflow chart, which merely depends upon this chart. I don't think this chart will ever see that value (i.e., it can't look "up"), which can certainly complicate the notion of conditionally installing or not installing the resources defined in this chart based on some config option in the parent chart.

Thinking through this more, I suspect what we really need to do is not touch the router chart at all, but conditionally include it or not include it in the Workflow chart. I'm wondering if that can actually be done... going to consult with the Helm team a bit.

krisnova commented 7 years ago

@mboersma @bacongobbler does anyone else see anything that needs attention on this one?

bacongobbler commented 7 years ago

Nope! Looks like all of the templated objects from this chart is covered.

mboersma commented 7 years ago

With the caveat that we intend to refactor this eventually (see deis/workflow#771), I'm ok with this change.