-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow to set custom headers #148
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
// SetCustomHeader - set additional header that will be sent with each request | ||
func (r *Client) SetCustomHeader(key, value string) { |
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.
I feel like it will be unnecessary confusion for users to have two different APIs for the same purpose. Could we please remove this one as the other one suffices and is clearer i.e. the behaviour is very clear?
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.
so, do you propose to leave SetCustomHeaders
and remove SetCustomHeader
? I think SetCustomHeader
easier to use and will be mostly used, but SetCustomHeaders
still useful if you want to rewrite and clear all custom headers.
WDYT?
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.
I think that the typical way in Go to pass key/value pairs is to use map[KeyType]ValueType
and it has quite a nice syntax. So, I personally don't see a reason to add some extra methods on top of that which simply do someMap[k] = v
. If someone really wants this functionality then it probably won't be a huge inconvenience to store those headers somewhere on the user side 🤔 we could probably add a custom convenience function - GetCustomHeaders
.
I really don't see a point in hiding from the users that it's a map[string]string
underneath as that is what headers are, really 😄
} | ||
|
||
// SetOrgIDHeader - set `X-Grafana-Org-Id` to specified value | ||
func (r *Client) SetOrgIDHeader(oid uint) { |
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.
I'm not sure if we should have this. It was undocumented for a long time and we have https://github.com/grafana-tools/sdk/blob/master/rest-org.go#L145-L146 for this purpose. IMO if someone wants this then they should really set it manually themselves.
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.
https://github.com/grafana-tools/sdk/blob/master/rest-org.go#L145-L146
this one actually differ, cause if you are an admin you need to switch context to use it, but it won't work in parallel at all
I want to be able to set custom headers for grafana clients, for example,
X-Grafana-Org-Id
very useful