Skip to content
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 Kubernetes CRD generator #17

Merged
merged 7 commits into from
Feb 22, 2024
Merged

Add Kubernetes CRD generator #17

merged 7 commits into from
Feb 22, 2024

Conversation

jackkleeman
Copy link
Contributor

Heavily based on the jsonschema generator, except with some CRD specific logic including the ability to set converters which replace types with types that you already have a definition for (eg EnvVar)

Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Copy link
Contributor

@holzensp holzensp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is almost good to go. Dotting some Is and crossing Ts.

packages/k8s.contrib.crd/generate.pkl Outdated Show resolved Hide resolved
packages/k8s.contrib.crd/generate.pkl Outdated Show resolved Hide resolved
packages/k8s.contrib.crd/generate.pkl Outdated Show resolved Hide resolved
packages/k8s.contrib.crd/generate.pkl Outdated Show resolved Hide resolved
Comment on lines 81 to 82
/// Converts is a Mapping from the CRD name (eg 'restateclusters.restate.dev') to a mapping from the path to a type
/// in that CRD (eg 'List("spec", "compute", "env", "env")') to a module, class, or type alias to use as the generated type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On doccomment style; please keep the summary line (the first line of a doccomment) a single, preferably short sentence.

packages/k8s.contrib.crd/internal/ModuleGenerator.pkl Outdated Show resolved Hide resolved
Comment on lines 309 to 315
.filter((path, _) -> !pathPrefixes(path).any((prefix) -> converters.containsKey(prefix))) // path or prefix are not explicitly in converters
.entries
.fold(Map(), (accumulator: Type.TypeNames, pair) ->
let (path = pair.first)
let (schema = pair.second)
let (typeName = determineTypeName(path, path.lastOrNull?.capitalize() ?? "Item", accumulator.values.toSet(), 0))
accumulator.put(schema, typeName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's quite a bit of packing/unpacking going on here; are you intentionally being quiet in the case of repeated map keys?

Comment on lines 347 to 360
.filter((path, _) -> converters.containsKey(if (logPaths) trace(path) else path))
.entries
.fold(Map(), (accumulator: Map<JsonSchema, ImportAndType>, pair) ->
let (path = pair.first)
let (schema = pair.second)
let (converted = converters[path])
let (reflected = if (converted is Module) reflect.Module(converted) else if (converted is TypeAlias) reflect.TypeAlias(converted) else reflect.Class(converted))
accumulator.put(schema, new ImportAndType {
type {
moduleName = if (reflected is reflect.Module) module.moduleName else reflected.enclosingDeclaration.name
name = reflected.name.split(".").last
}
_import = if (reflected is reflect.Module) reflected.uri else reflected.enclosingDeclaration.uri
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ packing/unpacking/quiet-overwrite

packages/k8s.contrib.crd/internal/ModuleGenerator.pkl Outdated Show resolved Hide resolved
packages/k8s.contrib.crd/internal/ModuleGenerator.pkl Outdated Show resolved Hide resolved
Copy link
Contributor

@holzensp holzensp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, was meant to be more explicit that there some things to be addressed.

bioball and others added 2 commits February 21, 2024 17:10
Co-authored-by: Philip K.F. Hölzenspies <holzensp@gmail.com>
* Kubernetes import path is configurable
* Module name is prefixed with reverse-fqdn
* Tests use output files
* Stylistic changes
* Reduce unnecessary warnings
/// // Use dependency notation, assuming the dependency is called `@k8s`.
/// k8sImportPath = "@k8s"
/// ```
k8sImportPath: String = "package://pkg.pkl-lang.org/pkl-k8s/k8s@\(k8sVersion)#"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added these two properties so that the Kubernetes version can be changed easily.

local sourceUri =
if (source.startsWith(Regex(#"\w+:"#))) source // absolute URI
else if (source.startsWith("/")) "file://\(source)" // absolute file path
else "file://\(read("env:PWD"))/\(source)" // relative file path

/// The CRD's source contents, as computed from [source].
sourceContents: String|Resource = read(URI.encode(sourceUri))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this so that we can swap it out for testing.


/// Converts is a Mapping from the CRD name (eg 'restateclusters.restate.dev') to a mapping from the path to a type
/// in that CRD (eg 'List("spec", "compute", "env", "env")') to a module, class, or type alias to use as the generated type
converters: Mapping<String, Mapping<List<String>, Module|Class|TypeAlias>>?

modules = new Listing<ModuleGenerator> {
modules: Listing<ModuleGenerator> = new {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: It's always best practice to add a type annotation. If no type annotation, the property's type is unknown.

@@ -102,7 +110,7 @@ converters: Mapping<List<String>, Module|Class|TypeAlias>
baseUri: URI

/// The name of this module
moduleName: String = crd.spec.names.kind
moduleName: String = crd.spec.group.split(".").reverse().add(crd.spec.names.kind).join(".")
Copy link
Contributor

@bioball bioball Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our convention for k8s is to use the reverse-FQDN of the group, then API version, then the kind.


local function getWarnings(schema: JsonSchema, type: "class"|"module"): String? =
if (type == "class") null
else if (schema.type == "object")
"WARN: The root schema describes open-ended properties, but this is not possible to describe at the module level."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kubernetes apparently differs from OpenAPI and JSON Schema because properties means a closed struct, and additionalProperties means arbitrary keys, and there's no in-between. So, this warning isn't very meaningful.

)
k8sImportPath + uri.dropWhile((it) -> it != "#").drop(1)
else uri
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this so that you can use any version of our k8s library, and we'll still replace it with k8sImportPath.

@@ -20,7 +20,8 @@ import "package://pkg.pkl-lang.org/pkl-k8s/k8s@1.0.1#/api/core/v1/EnvVar.pkl"
import "package://pkg.pkl-lang.org/pkl-k8s/k8s@1.0.1#/api/networking/v1/NetworkPolicy.pkl"

local generator = (generate) {
source = "file:crds.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read("file:crds.yaml") isn't recommended, and actually doesn't have well defined behavior right now. It's best to do read("crds.yaml") to read relative file paths.

@bioball bioball merged commit 8685ad6 into apple:main Feb 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants