Do not use JSON for router configuration #72
Conversation
|
Still need to test this against deis/workflow#197 |
|
Ok. Took some doing because of unrelated issues I ran into, but this works fine in conjunction with deis/workflow#197. |
krancour
changed the title
|
Requesting one of the reviews to be from @helgi since he's well familiar with what I'm out to achieve with this PR. |
| Domain string `json:"domain"` | ||
| UseProxyProtocol bool `json:"useProxyProtocol"` | ||
| EnforceWhitelists bool `json:"enforceWhitelists"` | ||
| WorkerProcesses string `router:"workerProcesses"` |
There was a problem hiding this comment.
Can you make this tag deis-router instead of just router? My hope is that it'll reduce the probability for collisions later
There was a problem hiding this comment.
I'm not clear on what collisions you think might occur. Can you elaborate, please? From my point of view, concerns from other components aren't going to magically affect the router's own internal model and have conflicts with how it has chosen to annotate struct fields.
There was a problem hiding this comment.
It should be noted that router in the struct tags is in no way tied to the annotation keys in the manifests. And the latter are namespaced nicely like router.deis.io/whatever to avoid conflicts.
There was a problem hiding this comment.
Just talking about the struct tags, and it's mostly a paranoid thing from me. If other code decides to use a router struct tag, there's the conflict. And since this is an exported type, it could be used elsewhere as well.
However, given this is a router project, you're right that it's probably unlikely that there'll be a collision, so I wouldn't say it's blocking for this PR.
|
@krancour code LGTM, I haven't tested in a cluster though. Let me know if you'd like me to |
|
@arschles fyi... I am still working on exporting error types as you've suggested. Doing so actually is allowing me to make a number of optimizations to the tests as well. Stay tuned. |
|
sounds good @krancour. let me know if you'd like more reviews after your changes! |
|
@arschles error types are exported and docstring'ed now. |
| return &Modeler{prefix: prefix, tag: tag} | ||
| } | ||
|
|
||
| // MapToModel populates the provided model with values from the provided map. |
There was a problem hiding this comment.
would be nice to have some docs here on some of the errors to expect. not blocking for this PR, however
There was a problem hiding this comment.
I will come back to that if/when we extract this "modeler" from the router.
|
@krancour awesome, thanks. made a few more comments on the new code |
Do not use JSON for router configuration
Fixes #63
This is WIP. There are more commits to come as part of this PR.