Proxy: use a global switch for Filter Middleware #501
Conversation
Codecov Report
@@ Coverage Diff @@
## master #501 +/- ##
=========================================
- Coverage 41.26% 41.2% -0.07%
=========================================
Files 99 99
Lines 2828 2830 +2
=========================================
- Hits 1167 1166 -1
- Misses 1552 1554 +2
- Partials 109 110 +1
Continue to review full report at Codecov.
|
pkg/module/filter.go
Outdated
| @@ -118,6 +119,7 @@ func (f *Filter) initFromConfig() { | |||
| lines, err := getConfigLines() | |||
|
|
|||
| if err != nil || len(lines) == 0 { | |||
| f.Off = true | |||
There was a problem hiding this comment.
can we also have a ENV override? what do you think? even with a filter file provided, we could turn it off
There was a problem hiding this comment.
@michalpristas I like that. I'll add one. I think the Filter should be Off by default unless provided with an actual configuration (or ENV override). But once we have Olympus working well, then maybe the Filter should be On by default unless the user explicitly turns it off by ENV only.
There was a problem hiding this comment.
Could we also skip the whole middleware if the environment var is set?
There was a problem hiding this comment.
@michalpristas i liked your ENV idea more than my "Off" idea that I basically removed my code 😄 -- it's much easier to just not use the middleware at all if we want it off.
marwan-at-work
changed the title
| // It defaults to "true" until Olympus is the default | ||
| // place to grab modules before the Proxy. | ||
| func FilterOff() bool { | ||
| return envy.Get("PROXY_FILTER_OFF", "true") == "true" |
There was a problem hiding this comment.
Should the envy call default to false as the check is whether the filter is off?
like envy.Get("PROXY_FILTER_OFF", "false") == "true"
if PROXY_FILTER_OFF was true, result would be true
if PROXY_FILTER_OFF was 'false' result would be false
the way it is now, with default of true, if the user does not set PROXY_FILTER_OFF the result will be true.
There was a problem hiding this comment.
I guess not, re-reading the comment clears that up. :D
There was a problem hiding this comment.
Yes by default, it's just the Proxy, no filtering needed. But we will use Olympus by default once Olympus is ready.
| // It defaults to "true" until Olympus is the default | ||
| // place to grab modules before the Proxy. | ||
| func FilterOff() bool { | ||
| return envy.Get("PROXY_FILTER_OFF", "true") == "true" |
There was a problem hiding this comment.
I guess not, re-reading the comment clears that up. :D
|
@michalpristas merging this to unblock myself and @carolynvs -- feel free to make comments if you have any and I'll open another PR :) |
| @@ -113,7 +113,9 @@ func App() (*buffalo.App, error) { | |||
| app.Stop(err) | |||
| } | |||
| app.Use(T.Middleware()) | |||
| app.Use(newFilterMiddleware(mf)) | |||
| if !env.FilterOff() { | |||
* Proxy: do not redirect on filter.Include * Filter: add Off opt * Filter: set Off to true when there's no config * Proxy: use a global switch for filter
Fixes last part of #495