-
Notifications
You must be signed in to change notification settings - Fork 0
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
Convert kindsys to a basic interface #33
base: main
Are you sure you want to change the base?
Changes from all commits
37574fc
28a376f
8cfb92c
22c5f6e
f46a23e
855516c
3009d89
e62706c
ce5bec6
ae027bd
9df4afc
1facab1
9c2348b
266df51
595213a
ab9fe10
f13e3ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"cSpell.words": [ | ||
"Finalizers", | ||
"themasys", | ||
"jsoniter", | ||
"hasher", | ||
"Santhosh", | ||
"santhoshsys", | ||
"jsonschema" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,8 @@ | ||
# kindsys | ||
a kind system for schematizing objects | ||
a kind system for working with well typed objects at multiple versions | ||
|
||
|
||
This repository currently includes two concrete frameworks to help implement kind support: | ||
* themasys -- kinds based on thema | ||
* santhoshsys -- (dummy) a set of json schema files on disk | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
# Playlist examples | ||
This package explores a few options for implementing a playlist kind with kindsys. | ||
|
||
Specifically, this implements three options: | ||
1. Schemas defined in cue using thema | ||
2. Direct golang validation | ||
3. Schemas (and validation) based on jsonschema | ||
|
||
|
||
### Why playlist? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
The playlist object is simple and can be used to illustrate key requirements. | ||
Specifically this requires a database lookup for migrations. | ||
|
||
|
||
### History | ||
|
||
Playlists have been in core grafana for a long time. The APIs were originally written against | ||
internal dashboard ids (numeric integers). | ||
* In v8.? (TODO) -- we allowed saving references to UIDs in addition to internal IDs | ||
* In v9.? (TODO) -- we updated the UI so that the "title" attribute is not used, it now shows the loaded dashboard title instead. This should also ensure that the spec.uid field is the k8s wrapper name | ||
* In some future version -- we want to force everythign do reference dashboard UIDs and remove the unused title version. This version will also remove the uid field on spec because that is a duplicate for what is in the wrapper metadata | ||
|
||
|
||
## Schemas | ||
|
||
In this example, we will define three versions | ||
|
||
* v0.0 -- the original schema that only takes internal ids | ||
* v0.1 -- adds a uid option to each playlist item and deprecates the id version and unused title | ||
* v1.0 -- removes the internal ID option, and unused title properties | ||
|
||
### Compatibility | ||
|
||
#### v0.0 -> v0.1 | ||
✅ This just adds additional features that may or may not be used | ||
|
||
#### v0.1 -> v1.0 | ||
⚠️ Requires database access to convert ID to UID | ||
|
||
#### v1.0 -> v0.1 | ||
✅ This can migrate OK | ||
|
||
#### v0.1 -> v0.0 | ||
⚠️ Requires database access to convert UID to ID | ||
⚠️ Requires database access to lookup title from UID | ||
|
||
|
||
|
||
## TODO | ||
|
||
thema: | ||
* use the apiVersion to specify the resource version | ||
* thema version should list all versions | ||
|
||
nice to have: | ||
* can we define 1.0 as the "next" version, not the current one? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,225 @@ | ||
package playlist | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"fmt" | ||
"strconv" | ||
|
||
"github.com/grafana/kindsys" | ||
"github.com/grafana/kindsys/examples/playlist/v0x" | ||
"github.com/grafana/kindsys/examples/playlist/v1x" | ||
) | ||
|
||
// The real version will need access to a database for this to work | ||
type MigrationLookupHooks interface { | ||
GetUIDFromID(ctx context.Context, id int64) (string, error) | ||
GetTitleAndIDFromUID(ctx context.Context, uid string) (int64, string, error) | ||
} | ||
|
||
var _ MigrationLookupHooks = &dummyLookupHooks{} | ||
|
||
type dummyLookupHooks struct{} | ||
|
||
func (k *dummyLookupHooks) GetUIDFromID(ctx context.Context, id int64) (string, error) { | ||
switch id { | ||
case 111: | ||
return "AAA", nil | ||
case 222: | ||
return "BBB", nil | ||
} | ||
return "", fmt.Errorf("unknown internal id") | ||
} | ||
|
||
func (k *dummyLookupHooks) GetTitleAndIDFromUID(ctx context.Context, uid string) (int64, string, error) { | ||
switch uid { | ||
case "AAA": | ||
return 111, "Title for ID(111)", nil | ||
case "BBB": | ||
return 222, "Title for ID(222)", nil | ||
} | ||
return 0, "", fmt.Errorf("unknown uid") | ||
} | ||
|
||
// EEEEP... this is awful, but at least it works | ||
func newMigrator(hooks MigrationLookupHooks) kindsys.ResourceMigrator { | ||
return func(ctx context.Context, obj kindsys.Resource, targetVersion string) (kindsys.Resource, error) { | ||
srcVersion := obj.StaticMetadata().Version | ||
if srcVersion == targetVersion { | ||
return obj, nil | ||
} | ||
|
||
srcMajor := -1 | ||
srcMinor := -1 | ||
targetMajor := -1 | ||
targetMinor := -1 | ||
n, err := fmt.Sscanf(srcVersion, "v%d-%d", &srcMajor, &srcMinor) | ||
if err != nil || n != 2 { | ||
return nil, fmt.Errorf("error reading source version") | ||
} | ||
n, err = fmt.Sscanf(targetVersion, "v%d-%d", &targetMajor, &targetMinor) | ||
if err != nil || n != 2 { | ||
return nil, fmt.Errorf("error reading target version") | ||
} | ||
|
||
data, err := json.Marshal(obj.SpecObject()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
static := obj.StaticMetadata() | ||
static.Version = targetVersion | ||
|
||
switch srcMajor { | ||
case 0: | ||
spec := &v0x.Spec{} | ||
err = json.Unmarshal(data, spec) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if static.Name == "" { | ||
static.Name = spec.Uid | ||
} | ||
switch targetMajor { | ||
case 0: | ||
switch targetMinor { | ||
case 0: | ||
// Migrate down (uid to id if necessary) | ||
for i, item := range spec.Items { | ||
if item.Type == v0x.ItemTypeDashboardByUid { | ||
id, title, err := hooks.GetTitleAndIDFromUID(ctx, item.Value) | ||
if err != nil { | ||
return nil, err | ||
} | ||
spec.Items[i].Type = v0x.ItemTypeDashboardById | ||
spec.Items[i].Title = title | ||
spec.Items[i].Value = fmt.Sprintf("%d", id) | ||
} | ||
if spec.Items[i].Title == "" { | ||
spec.Items[i].Title = spec.Items[i].Value | ||
} | ||
} | ||
case 1: | ||
// Migrate minor up (id to uid if possible) | ||
for i, item := range spec.Items { | ||
if item.Type == v0x.ItemTypeDashboardById { | ||
id, err := strconv.ParseInt(item.Value, 10, 64) | ||
if err != nil { | ||
return nil, err | ||
} | ||
uid, err := hooks.GetUIDFromID(ctx, id) | ||
if err == nil { | ||
spec.Items[i] = v0x.Item{ | ||
Type: v0x.ItemTypeDashboardByUid, | ||
Value: uid, | ||
} | ||
} | ||
} | ||
spec.Items[i].Title = "" // clear the title | ||
} | ||
} | ||
return &ResourceV0{ | ||
StaticMeta: static, | ||
CommonMeta: obj.CommonMetadata(), | ||
Spec: *spec, | ||
}, nil | ||
|
||
case 1: | ||
// from 0 to 1 | ||
targetSpec := v1x.Spec{ | ||
Interval: spec.Interval, | ||
Name: spec.Name, | ||
Items: make([]v1x.Item, len(spec.Items)), | ||
Xxx: "just here for the change detection version bypass", | ||
} | ||
for i, item := range spec.Items { | ||
dst, err := migrateItemV0ToV1(ctx, item, hooks) | ||
if err != nil { | ||
return nil, err | ||
} | ||
targetSpec.Items[i] = dst | ||
} | ||
return &ResourceV1{ | ||
StaticMeta: static, | ||
CommonMeta: obj.CommonMetadata(), | ||
Spec: targetSpec, | ||
}, nil | ||
|
||
default: | ||
return nil, fmt.Errorf("invalid target") | ||
} | ||
|
||
case 1: | ||
spec := &v1x.Spec{} | ||
err = json.Unmarshal(data, spec) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if targetMajor == 0 { | ||
targetSpec := v0x.Spec{ | ||
Interval: spec.Interval, | ||
Name: spec.Name, | ||
Items: make([]v0x.Item, len(spec.Items)), | ||
Uid: static.Name, | ||
} | ||
for i, item := range spec.Items { | ||
itemV0 := v0x.Item{Value: item.Value} | ||
switch item.Type { | ||
case v1x.ItemTypeDashboardByTag: | ||
itemV0.Type = v0x.ItemTypeDashboardByTag | ||
case v1x.ItemTypeDashboardByUid: | ||
itemV0.Type = v0x.ItemTypeDashboardByUid | ||
if targetMinor == 0 { | ||
id, title, err := hooks.GetTitleAndIDFromUID(ctx, item.Value) | ||
if err != nil { | ||
return nil, err | ||
} | ||
itemV0.Title = title | ||
itemV0.Value = fmt.Sprintf("%d", id) | ||
} | ||
} | ||
if targetMinor == 0 && itemV0.Title == "" { | ||
itemV0.Title = item.Value | ||
} | ||
targetSpec.Items[i] = itemV0 | ||
} | ||
return &ResourceV0{ | ||
StaticMeta: static, | ||
CommonMeta: obj.CommonMetadata(), | ||
Spec: targetSpec, | ||
}, nil | ||
} else { | ||
return nil, fmt.Errorf("invalid migration") | ||
} | ||
} | ||
|
||
return nil, fmt.Errorf("invalid version") | ||
} | ||
} | ||
|
||
func migrateItemV0ToV1(ctx context.Context, src v0x.Item, hooks MigrationLookupHooks) (v1x.Item, error) { | ||
dst := v1x.Item{ | ||
Type: v1x.ItemTypeDashboardByUid, | ||
Value: src.Value, | ||
} | ||
switch src.Type { | ||
case v0x.ItemTypeDashboardById: | ||
id, err := strconv.ParseInt(src.Value, 10, 64) | ||
if err != nil { | ||
return dst, err | ||
} | ||
uid, err := hooks.GetUIDFromID(ctx, id) | ||
if err != nil { | ||
return dst, err | ||
} | ||
dst.Value = uid | ||
case v0x.ItemTypeDashboardByTag: | ||
dst.Type = v1x.ItemTypeDashboardByTag | ||
case v0x.ItemTypeDashboardByUid: | ||
dst.Type = v1x.ItemTypeDashboardByUid | ||
default: | ||
return dst, fmt.Errorf("invalid src type") | ||
} | ||
return dst, nil | ||
} |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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