Conversation
ryantxu
requested review from
gabor,
yesoreyeram and
zoltanbedi
and removed request for
a team
|
hi @ryantxu , sorry are you looking for feedback in this PR? (i assume you're not, but want to be sure 😄 ) |
|
@gabor Only in the context of https://docs.google.com/document/d/1la4XW9BZ5XXyU91ZjbzEPE4jvPAyusE0dfJdsrCbbPI/edit#heading=h.5sybau7waq2q. -- trying to get a feeling if this is a reasonable approach or not. Github is interesting because "options" means something different for each query type |
| @@ -0,0 +1,891 @@ | |||
| { | |||
| "kind": "QueryTypeDefinitionList", | |||
There was a problem hiding this comment.
trying to understand this better. How this kind:QueryTypeDefinitionList maps to queryType:Issues in the actual payload? Am I missing any other contract here? Even if we argue queryType is not owned by the plugin but by the grafana(??), at least plugins should pass if they really use queryType and if it use, the schema should have details of all the possible query types.
There was a problem hiding this comment.
note that each "QueryTypeDefinition" can include a discriminator that says queryType is a special field:
"discriminators": [
{
"field": "queryType",
"value": "Issues"
}
],
src/static/schema/query.types.json
Outdated
| "kind": "QueryTypeDefinitionList", | ||
| "apiVersion": "query.grafana.app/v0alpha1", |
There was a problem hiding this comment.
Might not understand everything here, but this feels a bit limited if all plugins share the same apiVersion and colliding kinds?
There was a problem hiding this comment.
In this case the apiVersion defines how to read this JSON file. The apiVersion for each datasource is independent.
This is the equivalent to the apiVersion on a CRD file, not the custom resource (where the apiVersion is defined inside the CRD!)
There was a problem hiding this comment.
the schema for this file is the same across all data sources. If/when we need to make breaking changes to the QueryTypeDefinitionList then the apiVersion would update.
|
|
| } | ||
|
|
||
| require.NoError(t, err) | ||
| err = builder.AddQueries(schemabuilder.QueryTypeInfo{ |
There was a problem hiding this comment.
this will need the new query types added to be accurate
There was a problem hiding this comment.
And gives swagger + agents "example" queries to fill in
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c03e5e0a38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| require.NoError(t, err) | ||
| err = builder.AddQueries(schemabuilder.QueryTypeInfo{ |
There was a problem hiding this comment.
This AddQueries registration is incomplete relative to the datasource's supported query surface, so the generated query.types.json / query.*.schema.json only describe a subset of valid queries. Query kinds still handled elsewhere (for example Code_Scanning, Commit_Files, Pull_Request_Reviews, Pull_Request_Files, Workflow_Runs, Deployments, Organizations, GraphQL, and Projects) will be hidden or rejected by schema-driven editors/admission even though backend handlers can execute them.
Useful? React with 👍 / 👎.
| "required": [ | ||
| "repository", | ||
| "owner", | ||
| "timeField" |
There was a problem hiding this comment.
The generated schema now requires options.repository and options.owner for this query type, but this plugin's persisted query model keeps repo identity at top level and commonly stores only query-specific fields inside options; this mismatch makes existing saved queries fail validation once schema admission is enabled, despite backend handlers already deriving repo/owner from top-level fields.
Useful? React with 👍 / 👎.
5 participants
Using grafana/grafana-plugin-sdk-go#897 to generate a query spec