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.

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

APIVersion: Add API Version to protobuf #911

Merged
merged 17 commits into from May 8, 2024
Merged

APIVersion: Add API Version to protobuf #911

merged 17 commits into from May 8, 2024

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Feb 28, 2024

In an effort to limit the number of supported backend versions, we will want to specify what apiVersion we want, and what we are running. This PR adds apiVersion to the protobuf and startup arguments so it can throw an error when there is a mismatch.

When versions are not set, there is no added verification

@ryantxu ryantxu requested a review from wbrowne February 28, 2024 00:17
Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

I think this makes sense 👍 But have you thought about how this relates to the plugin.json version, and how/where we might map them?

proto/backend.proto Outdated Show resolved Hide resolved

// The API version when the settings were saved.
// NOTE: this may be an older version than the current apiVersion
string apiVersion = 12;
Copy link
Member Author

Choose a reason for hiding this comment

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

@wbrowne -- I updated this so we can now send the apiVersion when the plugin was saved. This can be something older than what people are currently running

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Make sense to support passing the API version on each request, but would like some more context around ManagedOpts.APIVersion (see comment)

// ApiVersion is the required ApiVersion -- setting this will
// fail any request asking for a different version. For multi-version servers,
// this value should not be set, and be handled directly in each server
APIVersion string
Copy link
Member

Choose a reason for hiding this comment

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

Why shouldn't the SDK support multi-version and/or why would you want to populate this? Hard to understand without seeing the full picture.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question -- I'll remove this config and make the api-management entirely up to the handler.

@wbrowne
Copy link
Member

wbrowne commented Mar 6, 2024

Also wondering - currently the runtime config param tells us what version of the API we want to run. But if we're loading a plugin as we traditionally do, we would want to know what it reports itself in order to validate, right?

@andresmgot
Copy link
Contributor

FTR, I am taking a look at this and the discussion in grafana/grafana#76419. I'll try to gather the proposal and feedback and write a small design doc so we can agree. Hope that makes sense!

@ryantxu
Copy link
Member Author

ryantxu commented May 7, 2024

This change will propagate thought many things -- I'm not sure the best approach here, but these are a few exploration branches:
https://github.com/grafana/grafana/tree/datasource-apiversion
https://github.com/grafana/grafana/tree/plugin-apiserver

Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

Nice! Just a few nits

proto/backend.proto Outdated Show resolved Hide resolved
backend/common.go Outdated Show resolved Hide resolved
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

I think a first step (before adding the API version to protobuf), would be to decide what would be the origin (source of truth) for this API version. Would that be the plugin.json or another schema somewhere?

Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

LGTM

@marefr
Copy link
Member

marefr commented May 8, 2024

I think a first step (before adding the API version to protobuf), would be to decide what would be the origin (source of truth) for this API version. Would that be the plugin.json or another schema somewhere?

Don't think it's a blocker for merging this given it's not in use and currently we only have v0alpha1 for the foreseeable future so we can default to this to begin with if we want to start populating/forward this?

Feels like your question is related to how plugins can return openapi schemas per api version as well, i.e. resources. Something @ryantxu has been talking about earlier. So maybe this needs some additional discussions/design doc?

@ryantxu
Copy link
Member Author

ryantxu commented May 8, 2024

I think a first step (before adding the API version to protobuf), would be to decide what would be the origin (source of truth) for this API version. Would that be the plugin.json or another schema somewhere?

We can set the apiVersion when the request goes through an apisever with a version -- for now they are all hardcoded to "v0alpha1", so yes the next thing is to have a plugin somehow advertise which apiVerions it supports (and what is the preferred version). We need something to control:

image

plugin.json is the easiest, but may not be the best since we should also standardize on how plugins can define schemas, so maybe a new static file in a known place is a good path forward.

ryantxu and others added 6 commits May 8, 2024 13:35
@ryantxu ryantxu marked this pull request as ready for review May 8, 2024 10:39
@ryantxu ryantxu requested a review from a team as a code owner May 8, 2024 10:39
@ryantxu ryantxu requested review from wbrowne, andresmgot and oshirohugo and removed request for a team May 8, 2024 10:39
@andresmgot
Copy link
Contributor

Don't think it's a blocker for merging this given it's not in use

Yes, that's my point. It will be dead code at the moment but 🤷

plugin.json is the easiest, but may not be the best since we should also standardize on how plugins can define schemas, so maybe a new static file in a known place is a good path forward

Yes, ideally a schema file with the query / settings definition would be the best but we kind of lost the progress there. We could temporarily use the plugin.json to specify that.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Code LGTM, we can merge even if it's not in use

Copy link
Member

@wbrowne wbrowne left a comment

Choose a reason for hiding this comment

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

🚀

@ryantxu ryantxu merged commit 878555a into main May 8, 2024
4 checks passed
@ryantxu ryantxu deleted the add-api-version branch May 8, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants