socialapi/countly: fix various stuff for countly client #10793
Conversation
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| return json.NewDecoder(resp.Body).Decode(&v) | ||
| } | ||
| b, err := ioutil.ReadAll(resp.Body) |
There was a problem hiding this comment.
What about setting some body limit?
const maxBody = 4 << 20
...
b, err := ioutil.ReadAll(io.LimitReader(resp.Body, maxBody))There was a problem hiding this comment.
Is it possible to have nil v? (e.g. no payload)
There was a problem hiding this comment.
What about setting some body limit?
even tho i am not sure at this point how much data we are going to fetch from countly api, 4<< 20 is a big number that we could assume as "safe". will add "LimitReader" ( i have only seen couple hundreds bytes of data received )
Is it possible to have nil v? (e.g. no payload)
didnt check their whole api yet, hence it is hidden under our worker not public/standalone
| return *v, err | ||
| // CreateApp creates an app with admin key. | ||
| func (c *Client) CreateApp(app *App) (*App, error) { | ||
| if app.Name == "" { |
There was a problem hiding this comment.
What about moving all the validation to a separate (*App).Valid() error?
| return nil, err | ||
| // CreateUser creates a user with admin credentials. | ||
| func (c *Client) CreateUser(info *User) (*User, error) { | ||
| if info.FullName == "" { |
There was a problem hiding this comment.
What about moving all the validation to a separate (*User).Valid() error?
| v := new(Apps) | ||
| err := c.do("GET", "/o/apps/mine", values, &v) | ||
| v := new(App) | ||
| err := c.do("GET", "/i/apps/create", values, &v) |
There was a problem hiding this comment.
it's a matter of taste but I prefer http.MethodGet instead of "GET"
There was a problem hiding this comment.
i prefer it like that too, will update.
| ) | ||
|
|
||
| var ( | ||
| defaultDomainURL = "http://localhost:32768" |
There was a problem hiding this comment.
will convert it to const
| } | ||
|
|
||
| for _, option := range opts { | ||
| option(client) | ||
| } | ||
|
|
||
| if client.log == nil { | ||
| logging.NewCustom("countly client", false) |
There was a problem hiding this comment.
client.log = logging.NewCustom(...)
|
i have cherrypicked this commit to other PRs, lets merge this PR after it passed and i will send another PR, works for you guys? |
No description provided.