-
Notifications
You must be signed in to change notification settings - Fork 97
Add methods for interacting with org preferences API (and a small bug fix) #102
Conversation
Signed-off-by: Mike Ball <mikedball@gmail.com>
Signed-off-by: Mike Ball <mikedball@gmail.com>
Signed-off-by: Mike Ball <mikedball@gmail.com>
@@ -0,0 +1,55 @@ | |||
package gapi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite that team preferences methods are homed in team.go
, I chose to home these org preferences methods in their own org_preferences.go
file (rather than stick them in the existing orgs.go
file) because...
- The Grafana API documentation documents the endpoints on a dedicated Preferences page, separate from its organization documentation.
grafana-api-golang-client
itself distinguishes between anorgs.go
andorg_users.go
file, seemingly preferring multiple files with granular collections of orgs-related methods
Signed-off-by: Mike Ball <mikedball@gmail.com>
} | ||
|
||
// Preferences represents Grafana preferences. | ||
type Preferences struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, an incomplete and mistaken type Preferences
implementation lived in team.go
. I've...
- moved it to a shared, common location in
preferences.go
- added the additional fields that may be present
- corrected the
HomeDashboardID
field to utilize ahomeDashboardId
field (rather than ahomeDashboardID
field, which appears to have been incorrect)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I understand from this is that the previous version wasn't properly passing the HomeDashboardID
value to Grafana, then?
If that's the case then this is great. If not, then it means something might break. Can you confirm if it was working before or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inkel I don't believe anything will break. Based on my integration testing (see PR #105), I believe both json:homeDashboardID
and json:homeDashboardId
will work correctly (and so it's perhaps a bit misleading for me to have deemed the use of json:homeDashboardID
a bug).
However, I believe the Grafana API itself uses the field homeDashboardId
(and not homeDashboardID
), as evidenced by the following, where the ff0c251f0c51
Grafana container resulted from the PR #105 integration testing technique:
$ docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
ff0c251f0c51 grafana/grafana:9.0.2 "/run.sh" 42 minutes ago Up 42 minutes 3000/tcp grafana-api-golang-client_grafana_1
$ docker exec -u 0 --interactive --tty ff0c251f0c51 /bin/bash
bash-5.1# apk add curl
fetch https://dl-cdn.alpinelinux.org/alpine/v3.15/main/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/v3.15/community/x86_64/APKINDEX.tar.gz
(1/4) Installing brotli-libs (1.0.9-r5)
(2/4) Installing nghttp2-libs (1.46.0-r0)
(3/4) Installing libcurl (7.80.0-r2)
(4/4) Installing curl (7.80.0-r2)
Executing busybox-1.34.1-r5.trigger
Executing glibc-bin-2.30-r0.trigger
/usr/glibc-compat/sbin/ldconfig: /usr/glibc-compat/lib/ld-linux-x86-64.so.2 is not a symbolic link
OK: 28 MiB in 38 packages
bash-5.1# curl -u admin:admin http://localhost:3000/api/teams/search?query=
{"totalCount":1,"teams":[{"id":8,"orgId":1,"name":"foo","email":"foo@bar.com","avatarUrl":"/avatar/f3ada405ce890b6f8204094deb12d8a8","memberCount":1,"permission":0,"accessControl":null}],"page":1,"perPage":1000}
bash-5.1# curl -u admin:admin http://localhost:3000/api/teams/8/preferences
{"theme":"","homeDashboardId":123,"timezone":"","weekStart":"","navbar":{"savedItems":null},"queryHistory":{"homeTab":""}}
} | ||
|
||
// OrgPreferences fetches org preferences. | ||
func (c *Client) OrgPreferences() (Preferences, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these methods be prefixed with Current
a la CurrentOrgPreferences
?
Hi @inkel and @julienduchesne - Are you the best team members to review this? If so, would you mind taking a look? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution. I gave it a first pass and it looks good, although I've left some comments. I'll give it another pass at another time, I'd like to know @julienduchesne thoughts too.
} | ||
|
||
// Preferences represents Grafana preferences. | ||
type Preferences struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I understand from this is that the previous version wasn't properly passing the HomeDashboardID
value to Grafana, then?
If that's the case then this is great. If not, then it means something might break. Can you confirm if it was working before or not?
Co-authored-by: Leandro López <inkel.ar@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
… fix) (grafana#102) * add additional fields to type Preferences Signed-off-by: Mike Ball <mikedball@gmail.com> * move preferences types to their own file Signed-off-by: Mike Ball <mikedball@gmail.com> * add methods for interacting with org prefs API Signed-off-by: Mike Ball <mikedball@gmail.com> * remove TODO comment from Preferences Signed-off-by: Mike Ball <mikedball@gmail.com> * Update org_preferences.go Co-authored-by: Leandro López <inkel.ar@gmail.com> Signed-off-by: Mike Ball <mikedball@gmail.com> Co-authored-by: Leandro López <inkel.ar@gmail.com>
… fix) (grafana#102) * add additional fields to type Preferences Signed-off-by: Mike Ball <mikedball@gmail.com> * move preferences types to their own file Signed-off-by: Mike Ball <mikedball@gmail.com> * add methods for interacting with org prefs API Signed-off-by: Mike Ball <mikedball@gmail.com> * remove TODO comment from Preferences Signed-off-by: Mike Ball <mikedball@gmail.com> * Update org_preferences.go Co-authored-by: Leandro López <inkel.ar@gmail.com> Signed-off-by: Mike Ball <mikedball@gmail.com> Co-authored-by: Leandro López <inkel.ar@gmail.com>
Hello! This PR seeks to...
Preferences
type to correctly marshal/unmarshal ahomeDashboardId
JSON field. Previously,grafana-api-golang-client
expected ahomeDashboardID
JSON field, but I don't believe that's correct based on the following; I believe the JSON field ishomeDashboardId
:Thanks!