Convert kindsys to a basic interface #33
Conversation
ryantxu
changed the title
| "versions": [ | ||
| { | ||
| "version": "v0-0", | ||
| "software": "v6.0" | ||
| }, | ||
| { | ||
| "version": "v0-1", | ||
| "software": "v9.1", | ||
| "changelog": [ | ||
| "adding the dashboard_by_uid type", | ||
| "deprecating the dashboard_by_id type", | ||
| "deprecating the PlaylistItem.title property (now optional and unused)", | ||
| "TODO: verify that k8s name and spec.uid match" | ||
| ] | ||
| }, |
| @@ -0,0 +1,68 @@ | |||
| { | |||
There was a problem hiding this comment.
Out of curiosity; Is this a manually-written file? Or a code-generated one? 🤔
There was a problem hiding this comment.
I tried a few flavors, these were generated from https://github.com/invopop/jsonschema and then I manually created the three versions.
| 3. Schemas (and validation) based on jsonschema | ||
|
|
||
|
|
||
| ### Why playlist? |
There was a problem hiding this comment.
I'm confused; haven't we agreed that dashboards are THE EXAMPLE? I'm happy with any, but I'm just wondering why sometimes we kinda agree on kind as the use case to focus on as a great example, and why sometimes we agree on another one 🤷🏻
Is it because it has migrations that rely on database lookups? Or why?
There was a problem hiding this comment.
dashboards -- with nested schemas at multiple versions -- are the reason/motivation/target for a complex schema system; specifically the "composable kinds"
This example is closer to a hello world -- we need to be grounded in something concrete to work against. I have not yet seen any example try more than one version, so starting with something simple seems reasonable.
| "plural": "playlists", | ||
| "singular": "playlist" | ||
| }, | ||
| "versions": [ |
There was a problem hiding this comment.
How strong / provable / reproducible is this definition? 🤔
Does this pull request adds some kind of "framework", "similar to Thema" to ensure the correctness there? Is it delegated to "santosh" lib? Could you point to that or clarify it, for the sake of reviewing the pull request, please? Thanks!
There was a problem hiding this comment.
this PR is exploring how we can prove something works (or not) by having a set of tests and examples that we expect to be valid or invalid.
The actual ability to migrate up+down is hugely dependant on context, and I don't believe any system that claims to do this without human validation.
delegated to "santosh" lib?
just the json validation part, not the migration part. However, I don't think any particular library is important here. The point is that we have valid and invalid inputs that we should be able to test if they are valid or invalid
There was a problem hiding this comment.
I agree with what regards to the implementation details, but I suspect will need to have a single source of truth for those schemas (registry?) and that's the reason behind my initial question.
I see this as plugin version, and its manifest details (Grafana version compat), or like Grafana semver, I strongly suspect we'll need a mechanism to keep a coherence and with some guarantees across all kind schemas. Otherwise, it'd be almost impossible to build something strong on top of it, no matter what underlying technology we use.
|
|
||
| checkValidVersion(t, sys) | ||
|
|
||
| // Thema is not yet using the resource version to validate a payload |
There was a problem hiding this comment.
What should be SSoT? Kinds or ResourceKinds? 🤔 Thanks!
There was a problem hiding this comment.
I don't quite understand the question. The issue pointed to in this test is that a resource says its version:
{
"apiVersion": "playlists.ext.grafana.com/v0-0",
"kind": "Playlist",
...
and we need to use that to validate the payload.
ResourceKinds will represent kinds that can be represented as k8s resources, while "Kinds" is the more generic term that also represents ComposableKinds. The actual kind implementation is responsible as the source of truth for that kind.
|
|
||
| // Read data into a Resource, when strict is true, all validation rules will be checked | ||
| // otherwise a resource will be created if possible, but all validation may not have been run | ||
| Read(reader io.Reader, strict bool) (Resource, error) |
There was a problem hiding this comment.
Why is Read and Migrate on ResourceKind rather than on Kind? Which non-k8s kind can't be migrated /read?
There was a problem hiding this comment.
A ComposableKind does not exist on its own -- by its nature, it exists inside another kind, so we can not read it independently. For example, panel options are are nested property within a dashboard panel, and the version/schema is known from a sibling.
There was a problem hiding this comment.
What about when working with those kinds within the plugin? Then you won't work with the entire dashboard but only each specific composable kind on its own.
| Kind | ||
|
|
||
| // eg: panel(options+fieldconfig) | transformation | dataquery | matcher | ||
| GetComposableType() string |
There was a problem hiding this comment.
What is ComposableType in this context? couldn't we just use kind.name?
There was a problem hiding this comment.
This still needs exploration -- and I think should be an explicit enumeration. This is essentially what the kind implements.
eg:
ComposableType = PanelOptions
Kind.name = timeseries | heatmap | stats | ...
Then we can ask for all ComposableKinds of type "PanelOptions" when constructing/validating panels
There was a problem hiding this comment.
Couldn't we use the slot for the exact same purpose?
There was a problem hiding this comment.
currently (before this PR) each "ComposableKind" has an schemaInterface -- that is the same as type here, and "slot" is used to say a sub-element.
The only place this is used is a "panel" schemaInterface that has two slots: options and fieldConfig. I think it is worth exploring if these can just be two kinds (PanelOptions and PanelFieldConfig?) rather than having both type/schemaInterface and slots concept. but again... this will take some effort to see how using it for real things feels before we should really decide
There was a problem hiding this comment.
I think the kind interface(s) look good, though there may still be a use-case for providing a Resource object to read into, rather than having Read return a Resource.
Along those same lines, I think the Resource interface will need some revisiting as we go on, it's grown a bit complex due to things that the app-sdk has to be able to handle in a "generic" sense, without utilizing the kind. As a first step, I think it seems valid that we should try to remove the Unmarshal in Resource once we can accomplish the same logic using the kind itself (though this means, if we wish to preserve current app-sdk logic, that we need a way for the kind to do a ReadInto of some sort).
| } | ||
|
|
||
| func (u *GenericResource[Spec, CustomMeta, Status]) Subresources() map[string]any { | ||
| return map[string]any{ |
There was a problem hiding this comment.
Do we expect/want GenericResource to handle other non-status subresources as well in a any kind of capacity? Or is the idea here that this only represents known, typed data?
There was a problem hiding this comment.
TBD for sure, but in general I think we should be very careful to add additional properties (sub-resources) that are expected to be saved on the resource and are not compatible with standard CRD storage
| default: | ||
| fmt.Printf("grafana anno> %s = %v\n", g, v) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
For non-grafana.com/* annotations, we'll want to add them to the annotations in ExtraFields. I think in the future we'll also just want Annotations exposed in CommonMetadata anyway.
Yes absolutely! my goal here was changing it as little as possible, because that is a project in itself. |
| @@ -0,0 +1,109 @@ | |||
| package santhoshsys | |||
There was a problem hiding this comment.
What do you think about calling this package jsonschema and view the santhosh just being the underlying framework used for implementation of jsonschema support.
There was a problem hiding this comment.
I called this santhoshsys mainly so it did not look like the solution to rule them all 😜
but yes, we should have something with a reasonable name that will provide reasonable support when you have a pile of jsonshchema files on disk
| func (k *rawPlaylistKind) Migrate(ctx context.Context, obj kindsys.Resource, targetVersion string) (kindsys.Resource, error) { | ||
| return k.migrator(ctx, obj, targetVersion) | ||
| } |
There was a problem hiding this comment.
Hmm, could we define an interface or convention for how migrations should be written and structured? Or is that out of scope for this PR?
There was a problem hiding this comment.
Afaik, the aim is to leave that to kind/service owners, not forced from Kindsys. Right @ryantxu?
Isn't that the purpose of the kindsys.ResourceMigrator definition? 🤔
There was a problem hiding this comment.
yes -- interface in ResourceMigrator
how migrations should be written and structured? Or is that out of scope for this PR?
yes out of scope here, but I think totally possible for us to explore declarative ways to write migrations. perhaps based on JSONPath, JSONata, or cel-go... or even js+wasm 😝 In general, this feels like something that a single approach would be hard to meet all needs
There was a problem hiding this comment.
yes out of scope here [...]
that will be implementation specific, at least for now, right? 🤔
| "kind": "Playlist", | ||
| "description": "A set of dashboards that will be displayed in a loop (dummy for testing)", | ||
| "maturity": "merged", | ||
| "machineName": { |
There was a problem hiding this comment.
I do understand that this is heavily inspired from K8 but for me machineName feels weird to use in this context.
There was a problem hiding this comment.
ya -- it is a weird one, but needed for k8s 😬 Note that "composable" kinds do not need any of the machine stuff
| //******************************************************************************************* | ||
| // NOTE!! | ||
| // This file is exploring generating the JSONSchema from golang... but that is paused for now | ||
| // The tests are just about the schema for now | ||
| //******************************************************************************************* |
There was a problem hiding this comment.
What are we aiming for long term? Defining the kinds in golang and generating schemas from those or the other way around?
There was a problem hiding this comment.
long term... TBD and I think will depend on the actual resource.
The playlist example is quite simple -- if feels most natural to me that we would define it in golang and generate JSONSchema + typescript from it.
Other resources... dashboards! feel much more natural to define in typescript and generate the JSONSchema + golang from that definition
|
|
||
| // Read data into a Resource, when strict is true, all validation rules will be checked | ||
| // otherwise a resource will be created if possible, but all validation may not have been run | ||
| Read(reader io.Reader, strict bool) (Resource, error) |
There was a problem hiding this comment.
What about when working with those kinds within the plugin? Then you won't work with the entire dashboard but only each specific composable kind on its own.
| Kind | ||
|
|
||
| // eg: panel(options+fieldconfig) | transformation | dataquery | matcher | ||
| GetComposableType() string |
There was a problem hiding this comment.
Couldn't we use the slot for the exact same purpose?
| } | ||
|
|
||
| // Not yet implemented, but will be required for kinds that require composition | ||
| type KindRegistry interface { |
There was a problem hiding this comment.
Could we also add a GetAllKinds? I guess it depends on how much of the code generation logic in Grafana we are planning on keeping but in those scenarios we would like to fetch all kinds regardless of type(?).
There was a problem hiding this comment.
I'm a bit confused by seeing code generation being mentioned here.
Isn't this a runtime service designed to be used by services (plugins or whatever) to query available kinds? I guess for code generation we'll need something like the kind-registry (repo), not as a runtime service but like a "central repository".
Could you clarify that, please? @ryantxu
There was a problem hiding this comment.
For composition to work, we will need a runtime service saying what is known right now -- The central repository project... I am not really sure what value it has TBH, I assumed it is a codegen requirement but not really sure.
There was a problem hiding this comment.
This might be an incorrect assumption from my side but currently we are using the kindsys during code/schema- generation while producing types/schemas.
You are correct regarding the kind-registry but at the same time it doesn't make sense (I think) for e.g. plugins/core grafana to rely on kinds that they provide/own from the registry when they just can use the "local" ones which potentially is a version not yet being published to the registry.
There was a problem hiding this comment.
I guess the alternative would be to provide a separate set of interfaces/abstractions that should be used to generate code/schemas.
There was a problem hiding this comment.
For composition to work, we will need a runtime service saying what is known right now
Gotcha, thanks! We'll most likely need to clearly identify/separate both to avoid confusion from now on.
This might be an incorrect assumption from my side but currently we are using the kindsys during code/schema- generation while producing types/schemas.
This is correct, although we'll need to update code generation pipelines sooner than later, at very least to reduce coupling between those and Thema/CUE. Looks like if we need to have a "standard schema language" for that, it's mostly likely going to be OAPI/JSON Schema.
You are correct regarding the kind-registry but at the same time it doesn't make sense (I think) for e.g. plugins/core grafana to rely on kinds that they provide/own from the registry when they just can use the "local" ones which potentially is a version not yet being published to the registry.
Yeah, I agree @mckn that for local uses, it makes no sense. I guess that's similar for different schemas/kinds defined within the same app.
I guess the alternative would be to provide a separate set of interfaces/abstractions that should be used to generate code/schemas.
I'm not even sure if those need to be in Kindsys, and/or if entirely there.
| case MaturityMerged: | ||
| return "merged" |
There was a problem hiding this comment.
Honestly I don't understand the need for maturity framework at all... this just mimics the properties defined in thema+kindsys today.
I think it makes more sense to:
- not include kinds that are not used anywhere (the "merged" maturity)
- stick "-alpha" at the end of a specific version string until it should be considered stable
but my goal here was not to revisit that structure yet!
There was a problem hiding this comment.
Ok ok, I like the suggestion you made here. The easier we can make it the better and it will increase the likelyhood of getting developers to actually use it.
There was a problem hiding this comment.
Agreed @ryantxu
PS: But better to leave it for a bit later, yeah!
| @@ -6,20 +6,20 @@ import ( | |||
| "path/filepath" | |||
|
|
|||
| "github.com/grafana/codejen" | |||
| "github.com/grafana/kindsys" | |||
| "github.com/grafana/kindsys/pkg/themasys" | |||
There was a problem hiding this comment.
Since this has a dependency on themasys could it live as a sub package within the themasys package? Just to keep a clean dependency graph.
There was a problem hiding this comment.
ya -- though that may be a LOT to change in one go, I think our codegen will require a few iterations to be based on new interfaces etc.
There was a problem hiding this comment.
Yeah, I have a PR where I have changed most of it to the Provider in kindsys. I'm not sure if we still want to continue on that route? grafana/grafana#71184
There was a problem hiding this comment.
I'm not sure how generic codegen should be at this point - I clearly see two "use cases" for it:
- types / definitions used "internally" (Grafana and/or apps codebase)
- as-code clients, used "externally"
and I'm wondering if we do actually need to solve them all at once / with a single generic definition/interface; cause I guess that while for the first, the workflow might depend on the specific service (as you said before, playlist and dashboards would probably be slightly different), for the latter we might use JSON Schema use a generalized workflow.
There was a problem hiding this comment.
We also have plugin developers that would like to consume some kind of codegen functionality. Both when developing plugin owned kinds and when consuming other kinds from the registry (depending on what we publish in some kind of registry).
| @@ -0,0 +1,57 @@ | |||
| # Playlist examples | |||
There was a problem hiding this comment.
I know the general agreement is to go away from Thema and use santhosys or anything else based on JSON Schema. But, considering the purpose here seems to be demonstrating the benefits of the generic interface, and that it looks like we may need to keep compatibility for both for a while, wouldn't make more sense, and make it more readable, to have each example isolated?
I'd be happy to help with that if that makes sense and does worth it!
There was a problem hiding this comment.
excellent -- yes this exists because we need something to exercise interfaces so we can learn if they are reasonable. This is the part I expect will evolve the most
| } | ||
|
|
||
| // A kind that manages a k8s style resource | ||
| type ResourceKind interface { |
There was a problem hiding this comment.
I feel like the wording is starting to become really messy.
In the same package kindsys, you can read:
// The relationship between Resource and [Kind] is similar to the
// relationship between objects and classes in conventional object oriented
// design:
//
// - Objects are instantiated from classes. The name of the class is the type of object.
// - Resources are instantiated from kinds. The name of the kind is the type of resource.
which imho is super confusing in combination with this ResourceKind definition. So, either one or the other will require some changes.
| @@ -1,9 +1,11 @@ | |||
| package kindsys | |||
There was a problem hiding this comment.
[Self-reminder]
This was consumed externally, for instance from here, so we'll need to update these references once we get this merged, otherwise everything will crash.
cc/ @AgnesToulet
|
Hey @ryantxu, what do you thing is still strictly needed to get this PR ready to merge? I see there's a (large) TODO list in PR's description, is that accurate? I'll be happy to help with any specific action to get it merged! |
There was a problem hiding this comment.
Don't have any strong opinions really and I could see this work for keeping backward compatibility with Thema. But still a bit hard to judge when where not really sure where we're headed in regards to schematization. In general I get the impression that we're trying to perhaps do/solve too many things here by mixing the concepts of schematization, codegen, multiple versions and migrations.
If we play with the idea that JSON schema is the decided schema format then I would argue that JSON schema is the abstraction and all this might or might not be needed. I do understand that something similar might be needed for particular use cases such as storage of resource to be able to define resources using Go, but feels perhaps easier to have a library targeting only that use case/concern.
| // Return a JSON schema definition for the selected version | ||
| // When composition is required, the slots will have an any node | ||
| // TODO? include an option to have `AnyOf(latest known options)` | ||
| GetJSONSchema(version string) (string, error) |
There was a problem hiding this comment.
Nit. Maybe this should be it's own interface to not mixing up raw type definitions and encoding/decoding. There's other places (Read) which seems like a decoder.
| CurrentVersion() string | ||
|
|
||
| // Get all versions | ||
| GetVersions() []VersionInfo |
There was a problem hiding this comment.
I understand this is a result of thema, but does it make sense from app platform perspective and comparing with k8s where you define resources using Go structs and clearly separate versions in separate packages, e.g. v1, v2 etc.
|
|
||
| // Migrate from one object to another version | ||
| // NOTE, this may require database calls | ||
| Migrate(ctx context.Context, obj Resource, targetVersion string) (Resource, error) |
There was a problem hiding this comment.
This feels a bit misplaced perhaps. Would rather keep migration concerns in a separate package and a specific interface to keep schemas/types with migrations separated. Similarly to my other comment, when defining resources as Go struct you might have one for each version and that might imply that a migrator takes a v1 object and return a v2 object. Understand this is due to thema supports multiple versions and migrations in the same definition. Something to keep in mind
There was a problem hiding this comment.
Do we have any apiserver (or whatever) example to see how the generic definitions here would be used (from the consumer point of view)? Perhaps that helps reasoning about what should be the minimum contract here, and what could be left, up to every specific implementation.
|
|
||
| // Read data into a Resource, when strict is true, all validation rules will be checked | ||
| // otherwise a resource will be created if possible, but all validation may not have been run | ||
| Read(reader io.Reader, strict bool) (Resource, error) |
There was a problem hiding this comment.
Should we include a wire format for the Read method, or we always assuming JSON (or is it up to the implemented to determine the format of the bytes themselves)?
Status: WIP/POC... this is a pretty big change that is only possible to evaluate when looking at the source tree (diff is too big!)
This PR changes kindsys from a strictly cue+thema managed system to one that is defined by a golang interface, and can be implemented in with alternative approaches.
This takes a few big steps:
Before:
After:
TODO:
FAQ:
Is this necessary?
Regardless how we choose to validate objects, we should build our tools against an interface that is forced to advertise its capabilities explicitly and that can be replaced/evolved as needed. The current surface area is too large/undefined/leaky to solidify into the foundation of all future grafana services.
What about existing kindsys usage and codegen?
Current kinds and codegen will continue to work after minor changes to the import package structures (TODO)
Future codegen packages can choose if they need to depend directly on thema+cue, or if access to a more generic kind interface is sufficient.
What about the shared static cuecontext?
This can still exist and work for thema based kinds. This context currently acts as a global registry -- in the future it could only be used to get access to cue based kinds.