Skip to content
This repository has been archived by the owner on Apr 26, 2021. It is now read-only.

Rename DatasourceId path parameter #34

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Rename DatasourceId path parameter #34

merged 2 commits into from
Mar 23, 2021

Conversation

papagian
Copy link
Collaborator

@papagian papagian commented Mar 22, 2021

We hope that this would be less confusing for grafana users.

Its value should be grafana for requests to be handled by grafana and the numeric datasource id for requests to be forwarded to a lotex datasource.

The chosen name Recipient is discussable.

Update
I have added a 0ed8a62 which is also required for grafana/grafana#32208.

@papagian papagian requested review from domasx2 and owen-d March 22, 2021 11:29
Copy link
Collaborator

@domasx2 domasx2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recipient makes me think of emails :D

Still gonna be int, === datasource.id, right? or will it take datasource.name as well?

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recipient sounds good, but we'll need to ensure a datasource name won't conflict with this reserved "grafana"key.

@papagian
Copy link
Collaborator Author

papagian commented Mar 22, 2021

Recipient makes me think of emails :D

Still gonna be int, === datasource.id, right? or will it take datasource.name as well?

In the suggested implementation if it equals the reserved "grafana" key is handled by the grafana otherwise is expected to be convertible to integer and that this number corresponds to a lotex datasource id.
Hence, identifying the datasource by the name is not supported. It's feasible though to support it but it would complicate things a bit
as @owen-d mentioned if the datasource name conflicts with the reserved key. Therefore, I would suggest keeping it as is unless it's absolutely necessary.

@domasx2
Copy link
Collaborator

domasx2 commented Mar 22, 2021

Recipient makes me think of emails :D
Still gonna be int, === datasource.id, right? or will it take datasource.name as well?

In the suggested implementation if it equals the reserved "grafana" key is handled by the grafana otherwise is expected to be convertible to integer and that this number corresponds to a lotex datasource id.
Hence, identifying the datasource by the name is not supported. It's feasible though to support it but it would complicate things a bit
as @owen-d mentioned if the datasource name conflicts with the reserved key. Therefore, I would suggest keeping it as is unless it's absolutely necessary.

That's cool, just wanted to be sure. Thanks!

@@ -31,7 +31,21 @@ type Ack struct{}
type Backend int

const (
GrafanaBackend Backend = iota
GrafanaBackend Backend = iota + 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this to be iota + 1?
edit: to not conflict with the zero value -- that's fine.

@owen-d owen-d merged commit 8873de5 into master Mar 23, 2021
@owen-d owen-d deleted the rename-path-parameter branch March 23, 2021 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants