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

feat: Cue spec validation #170

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

adamperlin
Copy link
Contributor

@adamperlin adamperlin commented Mar 8, 2024

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:

  1. It removes the need for some of the currently existing validation code within Dalec, within which it is hard to account for every error case.
  2. It allows us to add new spec validations easily and declaratively, without changing Go validation code
  • As a proof of concept for this, the Cue spec included in this PR places additional restrictions on source names
  1. It gives us a model to use for generating templated YAML which is correct-by-construction. We can use the Cue spec included here to create templates which can be used for generating starter spec stubs for new Dalec users, as well as creating templates over subcomponents of the Dalec spec (i.e., reducing boilerplate for particular commonly used sources, and any number of other tasks).

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:

@@ -60,7 +60,7 @@ build:

changelog:
-
date: 2023-10-10
date: 2023-10-10T00:00:00Z
Copy link
Contributor Author

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"
Copy link
Contributor Author

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

Copy link
Contributor

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.

@adamperlin adamperlin changed the title Cue spec validation feat: Cue spec validation Mar 9, 2024
"time"
)

// TODO: consider adding defaults to more fields
Copy link
Contributor

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.

@adamperlin
Copy link
Contributor Author

adamperlin commented Mar 11, 2024

Some further things to investigate:

  • Could we use returned error values from cue to provide more user-friendly and Dalec specific validation messages? For instance, something like validation error in field x.y.z, expected values are: ... I.e., can we avoid exposing users to extra cue concepts that may not be relevant, i.e. cue value unification
  • Further regexes to constrain URLs, semvers, etc.
  • Try to encapsulate all Dalec defaults in Cue spec

@cpuguy83
Copy link
Member

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))

Choose a reason for hiding this comment

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

nit:

Suggested change
var retErrs []error = make([]error, 0, len(errs))
retErrs := make([]error, 0, len(errs))

Comment on lines +281 to +282
cueCtx := cuecontext.New()
v := cueCtx.CompileString(cueSpec)

Choose a reason for hiding this comment

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

nit:

Suggested change
cueCtx := cuecontext.New()
v := cueCtx.CompileString(cueSpec)
ctx := cuecontext.New()
v := ctx.CompileString(cueSpec)

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.

Experiment with Spec Validation Using Cue
4 participants