-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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
Schema: Remove key
from root DataQuery type
#64467
Conversation
Backend code coverage report for PR #64467
|
Frontend code coverage report for PR #64467 |
hide?: bool | ||
|
||
// Unique, guid like, string used in explore mode | ||
key?: 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.
^^^ removed here
(updating comments in this file... everything else is generated)
/** | ||
* Unique, guid like, string (used only in explore mode) | ||
*/ | ||
key?: 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.
^^^ added explicitly to the typescript value here
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.
thank you! the costs of doing this stuff blind to its actual use 😧
hide?: bool | ||
|
||
// Unique, guid like, string used in explore mode | ||
key?: string | ||
|
||
// Specify the query flavor | ||
// TODO make this required and give it a default | ||
queryType?: 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.
@sdboyer Thoughts on how to best approach this one? The property exists on everything, but does not have any valid values until you are in a specific instance.
By gut says we should remove it from the base schema definition (keep it in veneer) and add it to explicit plugins with the valid types. The only global constraint is that the value is string based over json 🤷
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.
this one is really nasty, tbh - it's why #60680 is still open. It's now clear to me that writing and codegen-ing dataquery compo kinds is made less clear by the fact that we don't build in to the schema interface definition the notion that this is a discriminator field...when the field exists.
To actually make that change, IMO we need a design doc specifically for the DataQuery
schema interface to make sure we're covering all the angles
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.
yup -- there are really two modes used here. In some cases, queryType
changes the set of valid values and in others they just all live as siblings.
The github datasource is a good example of the queryType dependant models:
https://github.com/grafana/github-datasource/blob/main/pkg/models/query.go#L42
This removes
key
from DataQuery schema -- it is a property that exists and is used only in explore mode.In general
refId
is used to identify different queries