-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: Cue spec validation #170
base: main
Are you sure you want to change the base?
Conversation
@@ -60,7 +60,7 @@ build: | |||
|
|||
changelog: | |||
- | |||
date: 2023-10-10 | |||
date: 2023-10-10T00:00:00Z |
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.
It is a little annoying to have to specify dates this way. I'm going to look into whether this can be cleaner
@@ -67,7 +67,7 @@ sources: | |||
|
|||
build: | |||
env: | |||
CGO_ENABLED: 1 | |||
CGO_ENABLED: "1" |
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.
We do need to be explicit now about string vs. int values for env variables in the yaml itself
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.
That's a major gotcha in YAML. I think it's worth investigating if there's a way you can get CUE to interpret values in certain structures as strings. I'm not sure that it's possible but it may be.
"time" | ||
) | ||
|
||
// TODO: consider adding defaults to more fields |
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.
It is definitely worthwhile to capture all of the defaults in this PR. We actually have extensive defaults, mostly undocumented.
Some further things to investigate:
|
If we were to adopt this, I think we'd need to generate go types from the cue. |
_, err := pkgyaml.Validate(dt, v) | ||
errs := (cueerrors.Errors(err)) | ||
|
||
var retErrs []error = make([]error, 0, len(errs)) |
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.
nit:
var retErrs []error = make([]error, 0, len(errs)) | |
retErrs := make([]error, 0, len(errs)) |
cueCtx := cuecontext.New() | ||
v := cueCtx.CompileString(cueSpec) |
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.
nit:
cueCtx := cuecontext.New() | |
v := cueCtx.CompileString(cueSpec) | |
ctx := cuecontext.New() | |
v := ctx.CompileString(cueSpec) |
What this PR does / why we need it:
Currently, Dalec specs are validated using a mixture of strict YAML unmarshalling and manual validation of the unmarshaled spec in Go code. This PR allows us to concretely specify a schema for Dalec specs using Cue, and then utilize Cue for almost all spec validation. This has a number of benefits:
I am opening this as a draft mostly for discussion. All the current test cases in the repo are passing. I do have some concerns with user friendliness, as cue's error messages aren't always the nicest and take a bit of getting used to. They do however nearly always point to the specific field (or lack of field) which is causing the issue.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #132
Special notes for your reviewer: