feature(app): Add Limits field to App
#203
Conversation
brandon-welsch
force-pushed
the
fix/GRR-42/add-limits-field-to-app
branch
2 times, most recently
from
8448657 to
1015411
Compare
apps.go
Outdated
| ForceHTTPS bool `json:"force_https"` | ||
| RouterLogs bool `json:"router_logs"` | ||
| Flags map[string]bool `json:"flags"` | ||
| Limits map[string]int64 `json:"limits"` |
There was a problem hiding this comment.
My question is: are we sure it's an int, can't it be a float64 sometimes ?
There was a problem hiding this comment.
Currently everything is in integer, but indeed if we add a field of another type it could cause some problems. I can put an interface instead if you want
There was a problem hiding this comment.
I'm afraid it's not safe over time actually
There was a problem hiding this comment.
(So maybe interface{} is safer, like that at each usage, the developer has to wonder, which type it is)
There was a problem hiding this comment.
Ok i change it to map[string]interface{}
There was a problem hiding this comment.
I don't see how we would break applications, knowing that the field is not already present
There was a problem hiding this comment.
Then the question is at a different level: I don't think this lib should adapt to the organizational problem of how to think about updating our libs when we update the output of the API. That's why we have this PR template on the API: https://github.com/Scalingo/api/blame/master/.github/pull_request_template.md#L4
There was a problem hiding this comment.
To me there is also the thing that we should create libs with nice API that we don't break for any business reason, that's why I was in favor of interface{} since there are unknowns here and it would prevent us from breaking the API in the future
There was a problem hiding this comment.
I'm in favor of the interface{}
brandon-welsch
force-pushed
the
fix/GRR-42/add-limits-field-to-app
branch
from
1015411 to
eb3afff
Compare
Related to GRR-42