-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add apimachinery dependency #909
Conversation
@@ -47,6 +47,15 @@ type Frame struct { | |||
Meta *FrameMeta | |||
} | |||
|
|||
func (f *Frame) DeepCopy() *Frame { |
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.
For now, frame does not need to be a root object, but implementing DeepCopy allows an upstream struct to include Frame and use standard k8s codegen tools
experimental/api/v0alpha1/openapi.go
Outdated
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.
any suggestions for a better root path/name? "sdk?" once we are comfortable with this approach, this should live at the root
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.
Please do not merge this until this is finished and approved by plugins platform - https://docs.google.com/document/d/1la4XW9BZ5XXyU91ZjbzEPE4jvPAyusE0dfJdsrCbbPI/edit
I understand why it makes sense to keep the openapi definition close to the struct ( Extra 3MB per binary (6x3=18MB per plugin) can be noticeable for deployments with multiple plugins (e.g. ~180MB with 10 plugins), and there is no direct benefit for SDK consumers to have this today. I believe the next, less painful, approach would be to have this wrapper in grafana/grafana, where the actual consumer of this is at the moment. We can re-evaluate this when there are multiple consumers of the openapi definition. |
// QueryDataResponse contains the results from a QueryDataRequest. | ||
// It is the return type of a QueryData call. | ||
type QueryDataResponse struct { | ||
metav1.TypeMeta `json:",inline"` |
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 might confuse plugin authors - what is it, do they need to populate/do anything with it etc? Creating a wrapper as @andresmgot syggests seems straightforward and allows us to keep this non-exposed for plugin devs for now. Interestingly, these objects including data frame might require multiple apiVersions eventually as well and how de we handle that? The wrapper can still live in the sdk though, maybe under experimental/api or something.
Interestingly we've discussed that Grafana should maybe have its own implementation of QueryDataResponse to allow deciding whether or not based on need to decode the responses into Go struct/data frames or not and return the encoded bytes (if json) directly to the client/browser. Earlier discussions have touched upon the idea that QueryDataResponse should be an interface to allow what just mentioned within Grafana.
This pull request has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 2 weeks. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Everything exposed in the grafana apiserver will need an openapi representation compatible with https://github.com/kubernetes/kube-openapi, and top level resources will need to implement https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime#Object
We could try to keep the SDK k8s dependency free; but would then need to add wrappers/helpers somewhere and these would not have an easy integration point.
This PR adds the key dependencies:
This enables an apiserver to use results from the SDK directly without any special wrappers. The downside is bigger (but not tooooo much bigger) plugin builds
~3mb for each executable 🤷🏻 , but greatly simplifies our dependency cycles
With only kube-openapi, it adds ~1mb: