Updates to Custom kind for new schema format #9

IfSentient · 2023-04-13T18:14:59Z

Initial work on the Custom kind to enforce the new schema format, specifically when the kind as a crd trait.

If a Custom kind has the crd trait, then the lineage's schema enforces a specific format using joinSchema:

{
    metadata: CommonMetadata & {...string}
    spec: {...}
    status: CommonStatus & {...}
}

As part of adding the optional crd trait, the appID field was added to allow for generating the group (see TODO note about this).

Also as a part of this PR, a codegen trait was added, allowing app/plugin development tooling to know what kind of code to generate from this kind.

…e a schema format when being used to generate a CRD.

marefr · 2023-04-14T16:26:35Z

kindcat_custom.cue

+	// TODO: should this just be pluginID? Is there a world where an app and plugin may not share the same ID?
+	// The practical reason to have it different is to allow the appID to be shorter than the pluginID as the group
+	// is generated from the appID, and <kind name>+<group> cannot exceed 63 characters in kubernetes.
+	appID: =~"^([a-z][a-z0-9-]{0,61}[a-z0-9])$"


Discussed with @IfSentient a nice compromise might be to default to use the plugin id, but in case that results in a longer group than 63 characters we could allow an override (appId or group). This will allow plugin authors to easily determine the rules, if needed.

I've updated the PR so that now there is a plugin trait with an id field (there is a TODO on whether this should be a trait, or if pluginID should just be a top-level field in the kind). The crd trait has an optional groupOverride field which can be used to override using plugin.id as the group name.

Nice. Yeah let's discuss plugin trait vs part of kind. Since common and composable kinds refer to plugins maybe make most sense to include plugin id as a field?

I've done a weird temporary compromise where pluginID is a top-level field now, but the plugin trait still exists, and plugin.id is set to the pluginID field. Not sure if we want the trait to continue to exist, or if we want to revert that and have the ID live solely in the trait. I'm somewhat unopinionated on this, because I'm not sure if we're going to ever need more plugin info in the kind--if we do, we'll want to keep the trait. If there's both the trait and pluginID fields, it seems a bit confusing to use.

…ded plugin trait for specifing plugin ID which nominally determines CRD group unless crd.groupOverride is provided.

…ata in Custom.lineage.joinSchema, as thema codegen fails for externally-defined types. Update CustomProperties in props.go to reflect new traits added to the Custom kind. Add the method ToDef in load.go, which works similarly to ToKindProps, except it returns a def that includes the props _and_ the unified CUE value, allowing changes to the lineage to be part of the CUE value when passed to Bind<X>.

load.go

sdboyer · 2023-04-17T13:50:27Z

kindcat_custom.cue

+			joinSchema: {
+				// metadata contains embedded CommonMetadata and can be extended with custom string fields
+				// TODO: use CommonMetadata instead of redfining here; currently needs to be defined here 
+				// without extenal reference as using the CommonMetadata reference breaks thema codegen.


What breaks, exactly?

We should be able to specifically force dereferencing of such a type with a NameFunc.

I got a nasty nil pointer panic from inside of cue's openapi package, I didn't look too far into it, but it appears that it's a problem when there's an external ref. For now, it was simpler to not dig into that and unify the CommonMetadata model into the joinSchema once that's been worked out.

Tried adding a NameFunc, and it panics before I see any calls to the NameFunc. I'll open an issue for this in thema and look into it after dealing with this PR (as I have a tighter timeline with getting this into kindsys).

SGTM on timing - and huh! well, i'm poking at that code right now as part of lineage flattening, i'll keep an eye out for panics

sdboyer · 2023-04-17T13:55:10Z

kindcat_custom.cue

+		// If the crd trait is defined, the schemas in the lineage must follow the format:
+		// {
+		//     "metadata": CommonMetadata & {...string}
+		//     "spec": {...}
+		//     "status": {...}
+		// }
+		if S.crd != _|_ {


There's a couple ways we might structure the CUE, depending on whether we want to make using the CRD trait a decision that's made in kindsys for an entire kind category, or left up to individual kind authors.

Regardless, i'd suggest that for introspectability purposes, it's worth turning this comparison to bottom into a simple boolean in a top-level field within the kind, such as isCRD: crd != _|_, then referencing that field here. The fewer comparisons to bottom the better, and having an explicit field will make a kind trivially introspectable either on Go structs or when exported to JSON.

I think I've now addressed all this:

created isCRD, which is equal to S.crd == _|_

moved the definition of metadata/spec/status out of the lineage and into a kindsys top-level _crdSchema type (tested--codegen still works fine with this)

moved the isCRD check out of the lineage to the root of the kind, if true, it applies lineage: joinSchema: _crdSchema

Let me know if I'm missing something here, or if I should do anything in an alternate way.

… field in joinSchema.

…which is used in if is true.

…def. This cleans up the generated lineage.

IfSentient · 2023-04-17T15:23:28Z

kindcat_custom.cue

@@ -10,6 +98,79 @@ package kindsys
 Custom: S={
 	_sharedKind

-	lineage: { name: S.machineName }
+	// pluginID is the unique identifier of the plugin which owns this Custom kind
+	pluginID: =~"^([a-z][a-z0-9-]*[a-z0-9])$"


Thoughts on whether this should be a top-level field, or live only in the plugin trait (or even if the plugin trait should disappear, and only the pluginID reference should remain)?

i think we need to firm up the notion of a trait to really decide.

For the CUE part of it, i've been imagining traits as discrete sets of fields that can be composed into kind category definitions. Those sets - which in this PR i would imagine include both the top-level props and the joinschema - would then be imported into the github.com/grafana/kindsys CUE package and composed into the kind category definitions in as simple a manner possible. i'm thinking of that as valuable because as a reader/maintainer of the kind cat definitions, i want to be able to easily see and reason about how the trait is composed across kind categories, basically independent of what's in the trait itself.

Given the above, and because we actually need plugins to be a container/wrapper around kind definitions, they fall at least partly outside my current operating concept of "trait". Happy to change my mind here if we find something that feels right, though!

Makes sense to me, I think we can ditch the plugin trait in favor of just having the pluginID field as the top-level prop for Custom.

@IfSentient this is nit-picking but I would prefer if the kind system weren't aware of specific entities owning kinds. IMHO this is in agreement to sam's point about plugins (or apps) being containers of kind definitions.

To that end I suggest renaming pluginID to something more general, like a kindGroup or simply group (similar to CRD groups basically).

Based on discussions/input yesterday and the need for grouping all kinds (core, custom, composable) to fit in a schema registry I think group or similar makes more sense as well. Currently working on a doc that hopefully should propose how all of this are supposed to fit together in regards to a schema registry and plugins.

Don't mind leaving it as this either for now. We can change it later if needed given it happens soon (before too many depends on this)

I've updated it to be group in the top-level, just so we don't need to change as much later, and it doesn't explicitly reference plugins within the kind itself.

sdboyer · 2023-04-17T16:28:09Z

cc @AgnesToulet

kindcat_custom.cue

radiohead

Looks good from my side. I left a nitpicky comment about the naming of the pluginID field but I don't mind leaving it as it is.

IfSentient · 2023-04-18T15:13:22Z

kindcat_custom.cue

+			if S.crd.groupOverride != _|_ {
+				strings.ToLower(S.crd.groupOverride) + ".apps.grafana.com",
+			}
+			strings.ToLower(strings.Replace(S.pluginID, "-","_",-1)) + ".apps.grafana.com"


As an open comment here as well: <kind>.<pluginID/custom override>.apps.grafana.com--do we want this to be apps.grafana.com, ext.grafana.com, plugins.grafana.com? I think it makes sense to group all custom kinds under one <x>.grafana.com, as that way they can't overlap with core kinds (which are <kind>.grafana.com), but what should be?

CC: @bcotton @radiohead @mildwonkey

ext.grafana.com works for me.

Side note – IMHO the core kinds should also be grouped under a subdomain, e.g. core.grafana.com.

bcotton · 2023-04-18T15:14:05Z

kindcat_custom.cue

+		// This field could be inlined into `group`, but is separate for clarity.
+		_computedGroups: [
+			if S.crd.groupOverride != _|_ {
+				strings.ToLower(S.crd.groupOverride) + ".apps.grafana.com",


Is "apps" too specific?

Will there be cases where Kinds are defined in the scope of other "extensions". Datasources, Apps, Plugins generally?

Perhaps "ext.grafana.com"?

I'm in favor of ext.grafana.com. Doesn't take up too much of the allotted 63 characters for the full GroupKind, and is clear enough about what it is (an extension vs something in core).

ext.grafana.com works for me.

Now uses <group>.ext.grafana.com (where group is typically the plugin ID). groupOverride can be set, which overrides the entire crd.group property, so there is a way to make the group something else if the need arises.

…ana.com', remove openness (...) wherever possible.

…efore 'ext.grafana.com', as 'group' is no longer explicitly tied to plugin ID.

sdboyer

LGTM!

Since this is scoped to only custom kinds, it doesn't force our hand on integrating with core kinds yet. That'll be a key follow-up once this is merged

radiohead · 2023-04-20T11:23:04Z

kindcat_custom.cue

@@ -130,19 +140,19 @@ Custom: S={
 		// This field could be inlined into `group`, but is separate for clarity.
 		_computedGroups: [
 			if S.crd.groupOverride != _|_ {
-				strings.ToLower(S.crd.groupOverride) + ".apps.grafana.com",
+				strings.ToLower(S.crd.groupOverride) + ".ext.grafana.com",


@IfSentient as per our discussion earlier this week, let's make sure there's an escape hatch of overriding also the group suffix, in case there's no viable migration path for SLO / CLE apps.

Yup, most recent commit should have groupOverride overriding the whole group string.

Initial work on updating the Custom kind to use traits, and to enforc…

79880de

…e a schema format when being used to generate a CRD.

marefr reviewed Apr 14, 2023

View reviewed changes

IfSentient added 2 commits April 16, 2023 16:41

Added 'groupOverride' optional field for overriding the CRD group. Ad…

0abad6b

…ded plugin trait for specifing plugin ID which nominally determines CRD group unless crd.groupOverride is provided.

IfSentient marked this pull request as ready for review April 17, 2023 00:28

Missing join on _computedGroupKind constraint.

538482e

IfSentient changed the title ~~[WIP] Updates to Custom kind for new schema format~~ Updates to Custom kind for new schema format Apr 17, 2023

IfSentient commented Apr 17, 2023

View reviewed changes

load.go Show resolved Hide resolved

sdboyer reviewed Apr 17, 2023

View reviewed changes

IfSentient added 3 commits April 17, 2023 11:04

Add top-level fields isCRD and pluginID. Add state information to the…

5494a2b

… field in joinSchema.

Move schema format type out of the lineage and into _crdSchema type, …

7bfa3be

…which is used in if is true.

Move the isCRD conditional out of the lineage def into the root kind …

be9b229

…def. This cleans up the generated lineage.

IfSentient commented Apr 17, 2023

View reviewed changes

sdboyer reviewed Apr 17, 2023

View reviewed changes

kindcat_custom.cue Outdated Show resolved Hide resolved

IfSentient added 2 commits April 17, 2023 13:30

Remove plugin trait in favor of pluginID field.

5dabd38

Update CustomProperties with changes to Custom kind.

07ce4be

radiohead approved these changes Apr 18, 2023

View reviewed changes

IfSentient commented Apr 18, 2023

View reviewed changes

bcotton reviewed Apr 18, 2023

View reviewed changes

IfSentient added 2 commits April 19, 2023 08:16

Change pluginID to group, use 'ext.grafana.com' instead of 'apps.graf…

da6f4b2

…ana.com', remove openness (...) wherever possible.

Allow groupOverride to override the entire group, not just the part b…

e1ad221

…efore 'ext.grafana.com', as 'group' is no longer explicitly tied to plugin ID.

sdboyer approved these changes Apr 19, 2023

View reviewed changes

radiohead reviewed Apr 20, 2023

View reviewed changes

IfSentient merged commit 1944cac into main Apr 20, 2023

IfSentient deleted the kind-schema-format branch April 20, 2023 11:57

sdboyer mentioned this pull request Jul 4, 2023

Consider adding apiVersion and kind back to the shared _crdSchema #24

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to Custom kind for new schema format #9

Updates to Custom kind for new schema format #9

IfSentient commented Apr 13, 2023 •

edited

marefr Apr 14, 2023

IfSentient Apr 16, 2023

marefr Apr 17, 2023 •

edited

IfSentient Apr 17, 2023

sdboyer Apr 17, 2023 •

edited

IfSentient Apr 17, 2023

IfSentient Apr 17, 2023

sdboyer Apr 17, 2023

sdboyer Apr 17, 2023 •

edited

IfSentient Apr 17, 2023

IfSentient Apr 17, 2023

sdboyer Apr 17, 2023 •

edited

IfSentient Apr 17, 2023

radiohead Apr 18, 2023

marefr Apr 18, 2023

marefr Apr 18, 2023

IfSentient Apr 19, 2023

sdboyer commented Apr 17, 2023

radiohead left a comment

IfSentient Apr 18, 2023

radiohead Apr 19, 2023

bcotton Apr 18, 2023 •

edited

IfSentient Apr 18, 2023

radiohead Apr 19, 2023

IfSentient Apr 19, 2023

sdboyer left a comment

radiohead Apr 20, 2023

IfSentient Apr 20, 2023

Updates to Custom kind for new schema format #9

Updates to Custom kind for new schema format #9

Conversation

IfSentient commented Apr 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marefr Apr 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer Apr 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer Apr 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer Apr 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Apr 17, 2023

radiohead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcotton Apr 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IfSentient commented Apr 13, 2023 •

edited

marefr Apr 17, 2023 •

edited

sdboyer Apr 17, 2023 •

edited

sdboyer Apr 17, 2023 •

edited

sdboyer Apr 17, 2023 •

edited

bcotton Apr 18, 2023 •

edited