Improve apiVersion in plugin models #87639
Conversation
grafana-pr-automation
bot
added
area/frontend
area/backend
datasource/Prometheus
labels
| @@ -105,5 +105,6 @@ | |||
| "url": "https://prometheus.io/" | |||
| } | |||
| ] | |||
| } | |||
| }, | |||
| "apiVersion": "v0alpha3" | |||
There was a problem hiding this comment.
^^^ should be removed before commit!
But note, that this will make the server run on the configured apiVersion 🎉
| APIVersion string `json:"apiVersion"` | ||
|
|
||
| // The apiVersion (and schema) when the datasource was saved | ||
| // This value links the JsonData and SecureJsonFields with an explicit version |
There was a problem hiding this comment.
we can remove the comment if it helps 🤷🏻
But I keep getting confused which one indicates where it is running vs where it was saved!
pkg/api/dtos/plugins.go
Outdated
| @@ -28,7 +28,7 @@ type PluginSetting struct { | |||
| SignatureType plugins.SignatureType `json:"signatureType"` | |||
| SignatureOrg string `json:"signatureOrg"` | |||
| AngularDetected bool `json:"angularDetected"` | |||
| APIVersion string `json:"apiVersion"` | |||
There was a problem hiding this comment.
alternatively we could use the group+version flavor, but I find it super confusing that k8s mixes the use of these eg:
apiVersion = "v0alpha1"
or
apiVersion = "prometheus.datasource.grafana.app/v0alpha"
sadly k8s uses both all over the place 🤒
There was a problem hiding this comment.
I don't think we need this yet? I guess that the idea here is that the frontend would get this api server config and start querying the datasource in the new URL, right? (/apis/<api_group>/<api_version>/namespaces/stack/connections/<ds_UID>/query vs /api/ds/query).
Since we already have the /apis endpoint, which returns similar information coming from the k8s tooling, do we need to modify this for the "legacy" endpoint?
There was a problem hiding this comment.
Alternatively we could remove/hide it entirely entirely for now. "apiVersion" without the group is a bit awkward in this api call.
By adding APIServerInfo we add enough info to point to everything required for:
`/apis/<api_group>/<api_version>/namespaces//connections/<ds_UID>/query
There was a problem hiding this comment.
Alternatively we could remove/hide it entirely entirely for now. "apiVersion" without the group is a bit awkward in this api call.
I think it's still useful to have just the apiVersion, for example to store it in datasource settings and dashboards.
By adding APIServerInfo we add enough info to point to everything required for:
`/apis/<api_group>/<api_version>/namespaces//connections/<ds_UID>/query
Yes, but I don't know if this is the best place to have it (also because it's a bit hardcoded). I think a better workflow would be for the frontend to have a map between pluginID -> DS server (e.g. prometheus: prometheus-url:6443) and then obtain the apiVersions and groups with a request to the DS server (prometheus-url:6443/apis). Isn't that closer to a native workflow?
There was a problem hiding this comment.
I'm not sure the best strategy to get from where we are to where we need to be -- long term, the plugins settings will not be there. It will be a resource managed in an apiserver saying what is available.
My concern with setting a single apiversion -- is mainly that people will start to think that is true and depend on it. The frontend needs to know what apiVersion it is able to talk to if it wants to use the preferredVersion that is fine, we just want to make sure the pattern is clear that a datasource/app can have many versions and the client must know who they are talking to
| @@ -224,7 +227,9 @@ func (b *DataSourceAPIBuilder) getPluginContext(ctx context.Context, uid string) | |||
| if err != nil { | |||
| return backend.PluginContext{}, err | |||
| } | |||
| return b.contextProvider.PluginContextForDataSource(ctx, instance) | |||
| pctx, err := b.contextProvider.PluginContextForDataSource(ctx, instance) | |||
| pctx.APIVersion = b.connectionResourceInfo.APIVersion() | |||
There was a problem hiding this comment.
^^^ this is the interesting part
This tells the backend what apiversion was requested.
NOTE: pctx.DataSourceInstanceSettings.APIVersion may be different! it should be the version when the datasource settings were saved
what is the frontend part? IIUC, nothign in the the existing /api should change -- only if we are talking to /apis/... |
|
Closing and replacing with #88041 |
This is a proposed update to #87510
This updates the settings model so it is more clearly a indication of where the apiserver is running with which versions:
It also makes the value optional on individual apis so we don't advertise the value until it is actually saved.