-
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
Plugins: Add apiserver info to settings when it exists #88041
base: main
Are you sure you want to change the base?
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the backend is not a single apiVersion -- it may have a few and a preferred version
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.
why not simplify this just using the preferredVersion
here? In which case the frontend would be interested in knowing all the versions the backend supports?
Also, why duplicating this information? This is currently available in the endpoing /apis
for the datasource backend endpoint.
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.
replied already here: #87639 (comment)
Not sure if the frontend needs to handle this complexity (multiple api versions) but it's syntactically correct so if we want to move forward with this it can be okay
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.
Alternatively we can keep this out of the plugins DTO entirely -- I want to make sure we do not read it to mean more than it does. The issue I have with the simplified version is that it encourages thinking about frontend+backend as one deployment rather than two things: client and server
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.
we need to have something here if we want to set the value in queries / configs. The frontend will use the preferredVersion
anyway and use it if we add the whole api server info. I don't see a use case for the fronted to use the rest of the information so why adding it?
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.
What do we need to move this forward? Agreement on either only including preferred version or plus api group and versions?
Given this is new/experimental can we start with just adding what we actually need right now and iterate figuring out the rest? As long as we don't officially support any new api response field we can add/remove/change how many times we want/need.
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.
As far as I understand from this comment: #87961 (comment) we may want to just remove the apiVersion
from the /api/*
endpoints and leave it for the new ones under /apis/*
🤷
@andresmgot -- See #87639 (comment) -- that thread got lost when I made a new PR here. |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not simplify this just using the preferredVersion
here? In which case the frontend would be interested in knowing all the versions the backend supports?
Also, why duplicating this information? This is currently available in the endpoing /apis
for the datasource backend endpoint.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can probably be in a different PR? It's unrelated to having apiserver info in the plugin DTO.
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.
👍🏻 -- i can split it out soon, already a few too many open PRs for hte same topic 🤣
Hi! Greetings from the Release Guild. Just letting you know I'm updating your PR's milestone to 11.2.x. If you need to see your changes in a previous version, please consider adding a backport label. Thank you! |
This replaces #87639
This updates the settings model so it is more clearly a indication of where the apiserver is running with which versions: