Functionality for Managing Orgs and Users #8
Conversation
admin.go
Outdated
| return user.Id, err | ||
| } | ||
|
|
||
| func (c *Client) CreateUser(email, login, name, password string) (int64, error) { |
There was a problem hiding this comment.
Why does this exist in addition to CreateUserForm ?
There was a problem hiding this comment.
I couldn't find a way for the library and provider to use the same dtos package since the provider was using a vendor'ed one, so this was a workaround. I guess the library could just export one generic method without the use of the dtos package?
There was a problem hiding this comment.
Hmm, I'm not sure if pulling packages from something that is a client of a library is a good direction. Usually it's the other way around.
I do not think passing in the DTO from a client is necessary (or a good approach here).
There was a problem hiding this comment.
Ah, sorry. The dtos come from grafana, not the provider. That's OK, then. :)
There was a problem hiding this comment.
I think that if there are data structs for all the grafana resources, we should import/use them in the client.
There was a problem hiding this comment.
That dtos package created a compile issue due to it being vendored/not vendored between the two. Maybe we can just have this method use the User struct that the other user methods use?
client.go
Outdated
| return req, err | ||
| } | ||
|
|
||
| func (c *Client) newQueryRequest(method, requestPath string, query string) (*http.Request, error) { |
There was a problem hiding this comment.
Do we need this? Can query params be passed to newRequest() as part of the requestPath ?
There was a problem hiding this comment.
I tried that-- any special characters such as ?&= for query parameters get URL-escaped if they're included in the requestPath.
Another option would to modify the existing newRequest() function and to support query parameters if that'd be better?
There was a problem hiding this comment.
I think it would be better to add to the existing method over creating a new one that is pretty much a copy of the other one. Or at least extract a common part.
If we're putting query params into the RawQuery, those params themselves need to be encoded, too. Is this something that the library is going to do?
org_users.go
Outdated
| Role string `json:"role"` | ||
| } | ||
|
|
||
| func (c *Client) OrgUsers(id int64) ([]OrgUser, error) { |
There was a problem hiding this comment.
Could id be also called orgId as in other methods?
There was a problem hiding this comment.
I would change newRequest(method, requestPath, query string, body io.Reader) to newRequest(method, requestPath, query url.Values, body io.Reader).
When there is no query, the user can pass nil instead of an empty string.
When the user needs to add query parameters, they can build a query like this:
v := url.Values{}
v.Add("name", "Ava")
v.Add("friend", "Jess")
In the function: url.RawQuery = query.Encode()
https://golang.org/pkg/net/url/#Values
This is how I think Go expects it to be done: https://golang.org/src/net/url/example_test.go
|
That seems like a much better way to handle the query parameters, I'll add that in. Thanks! |
admin_test.go
Outdated
| Email: "admin@localhost", | ||
| Login: "admin", | ||
| Name: "Administrator", | ||
| Name: "Administrator", |
admin_test.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| createUserJSON = `{"id":1,"message":"User created"}` |
There was a problem hiding this comment.
Weird, something must be configured incorrectly with my editor, I'll run it now.
Previous discussion for this PR is here.
This PR adds functionality to the library for better management of Grafana Users and Organizations, as well as adding tests to cover new (and some existing) functions.