deis / router

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

Add ability to specify nginx access log format #290

Closed rvadim closed 7 years ago

rvadim commented 7 years ago

Hi, This PR add ability for customizing nginx access log format through router deployment annotations.

We need this ability to migrate from deis 1.x.

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.

deis-bot commented 7 years ago

@mboersma, @helgi and @krancour are potential reviewers of this pull request based on my analysis of git blame information. Thanks @rvadim!

codecov-io commented 7 years ago

Current coverage is 55.23% (diff: 100%)

Merging #290 into master will increase coverage by 0.11%

@@             master       #290   diff @@
==========================================
  Files             6          6          
  Lines           381        382     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            210        211     +1   
  Misses          151        151          
  Partials         20         20          

Powered by Codecov. Last update 563d731...2f287d0

mboersma commented 7 years ago

Jenkins, add to whitelist.

krancour commented 7 years ago

I'd want @jchauncey to have final say on this. This might affect the metrics that are collected from the router.

jchauncey commented 7 years ago

Yes if you change the router log format in any way you will definitely jack up the router metrics we collect. So while I'm not opposed to this we should have a warning if you change it.

rvadim commented 7 years ago

So, any tasks for me here? Need to add warning to log?

krancour commented 7 years ago

@rvadim I think you'd need to add a warning to the documentation for this feature.

rvadim commented 7 years ago

Done, but get CI error in infrastructure.

bacongobbler commented 7 years ago

thanks @rvadim!