Skip to content
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.

Sign up for GitHub

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

Remove apiVersion from root PluginContext #1015

Closed
wants to merge 3 commits into from
Closed

Remove apiVersion from root PluginContext #1015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67dd667
Remove apiVersion from root PluginContext
andresmgot Jul 2, 2024
d8437bb
lint fix
andresmgot Jul 2, 2024
d6b3a3c
fixes
andresmgot Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions backend/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,6 @@ type PluginContext struct {
// UserAgent is the user agent of the Grafana server that initiated the gRPC request.
// Will only be set if request is made from Grafana v10.2.0 or later.
UserAgent *useragent.UserAgent

// The requested API version
APIVersion string
}

func setCustomOptionsFromHTTPSettings(opts *httpclient.Options, httpSettings *HTTPSettings) {
Expand Down
1 change: 0 additions & 1 deletion backend/convert_from_protobuf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ func (f ConvertFromProtobuf) PluginContext(proto *pluginv2.PluginContext) Plugin
OrgID: proto.OrgId,
PluginID: proto.PluginId,
PluginVersion: proto.PluginVersion,
APIVersion: proto.ApiVersion,
User: f.User(proto.User),
AppInstanceSettings: f.AppInstanceSettings(proto.AppInstanceSettings),
DataSourceInstanceSettings: f.DataSourceInstanceSettings(proto.DataSourceInstanceSettings, proto.PluginId),
Expand Down
6 changes: 1 addition & 5 deletions backend/convert_from_protobuf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,7 @@ var protoPluginContext = &pluginv2.PluginContext{
GrafanaConfig: map[string]string{
"foo": "bar",
},
UserAgent: "Grafana/10.0.0 (linux; amd64)",
ApiVersion: "v0alpha1",
UserAgent: "Grafana/10.0.0 (linux; amd64)",
}

func TestConvertFromProtobufPluginContext(t *testing.T) {
Expand Down Expand Up @@ -262,7 +261,6 @@ func TestConvertFromProtobufPluginContext(t *testing.T) {
requireCounter.Equal(t, protoCtx.DataSourceInstanceSettings.ApiVersion, sdkCtx.DataSourceInstanceSettings.APIVersion)
requireCounter.Equal(t, protoCtx.PluginId, sdkCtx.DataSourceInstanceSettings.Type)
requireCounter.Equal(t, protoCtx.PluginVersion, sdkCtx.PluginVersion)
requireCounter.Equal(t, protoCtx.ApiVersion, sdkCtx.APIVersion)
requireCounter.Equal(t, protoCtx.DataSourceInstanceSettings.Url, sdkCtx.DataSourceInstanceSettings.URL)
requireCounter.Equal(t, protoCtx.DataSourceInstanceSettings.User, sdkCtx.DataSourceInstanceSettings.User)
requireCounter.Equal(t, protoCtx.DataSourceInstanceSettings.Database, sdkCtx.DataSourceInstanceSettings.Database)
Expand Down Expand Up @@ -404,7 +402,6 @@ func TestConvertFromProtobufQueryDataRequest(t *testing.T) {
// PluginContext
requireCounter.Equal(t, protoQDR.PluginContext.OrgId, sdkQDR.PluginContext.OrgID)
requireCounter.Equal(t, protoQDR.PluginContext.PluginId, sdkQDR.PluginContext.PluginID)
requireCounter.Equal(t, protoQDR.PluginContext.ApiVersion, sdkQDR.PluginContext.APIVersion)
// User
requireCounter.Equal(t, protoQDR.PluginContext.User.Login, sdkQDR.PluginContext.User.Login)
requireCounter.Equal(t, protoQDR.PluginContext.User.Name, sdkQDR.PluginContext.User.Name)
Expand Down Expand Up @@ -573,7 +570,6 @@ func TestConvertFromProtobufAdmissionRequest(t *testing.T) {
// PluginContext
requireCounter.Equal(t, protoAR.PluginContext.OrgId, sdkAR.PluginContext.OrgID)
requireCounter.Equal(t, protoAR.PluginContext.PluginId, sdkAR.PluginContext.PluginID)
requireCounter.Equal(t, protoAR.PluginContext.ApiVersion, sdkAR.PluginContext.APIVersion)
// User
requireCounter.Equal(t, protoAR.PluginContext.User.Login, sdkAR.PluginContext.User.Login)
requireCounter.Equal(t, protoAR.PluginContext.User.Name, sdkAR.PluginContext.User.Name)
Expand Down
1 change: 0 additions & 1 deletion backend/convert_to_protobuf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ func (t ConvertToProtobuf) PluginContext(pluginCtx PluginContext) *pluginv2.Plug
OrgId: pluginCtx.OrgID,
PluginId: pluginCtx.PluginID,
PluginVersion: pluginCtx.PluginVersion,
ApiVersion: pluginCtx.APIVersion,
User: t.User(pluginCtx.User),
AppInstanceSettings: t.AppInstanceSettings(pluginCtx.AppInstanceSettings),
DataSourceInstanceSettings: t.DataSourceInstanceSettings(pluginCtx.DataSourceInstanceSettings),
Expand Down
17 changes: 3 additions & 14 deletions genproto/pluginv2/backend.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

54 changes: 37 additions & 17 deletions genproto/pluginv2/backend_grpc.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions proto/backend.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
string role = 4;
}

message PluginContext {

Check failure on line 50 in proto/backend.proto

View workflow job for this annotation

GitHub Actions / main 

Previously present field "9" with name "apiVersion" on message "PluginContext" was deleted.
// The Grafana organization id the request originates from.
int64 orgId = 1;

Expand Down Expand Up @@ -81,9 +81,6 @@

// The user agent of the Grafana server that initiated the gRPC request.
string userAgent = 8;

// The API version that initiated a request
string apiVersion = 9;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure this is fine even though not in use? Have you tested it? Like if a new field 9 is added later with a different type and there are grafana/plugin versions still depending on the old version etc. You never delete anything is what I've learned. I think there's a trick to keep the index unnamed or something

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this should be removed. Any requests starting from apiservers should include it. This is how the backend can consistently know the apiversion for all grpc requests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit fuzzy to me, the apiservers don't start requests, do they? Meaning, the request comes from either a frontend, the alert runner or similar. Why would it append an api version to a request it's receiving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment made offline by @marefr:

Okay. But datasource api server receiving a request will have the api version in the URL and to route that to backend plugin handling that specific api version you might need to forward the api version 🤷 In that case, plugin context seems like a good fit. That's my thinking

Which makes sense. Thanks both for the input!

We are not using this atm but yeah, we may want to use it. I'll close this PR 👍

}

//---------------------------------------------------------
Expand Down
Loading