-
Notifications
You must be signed in to change notification settings - Fork 5
Support pre-built function runtimes and per-language schema generation #24
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -19,6 +19,9 @@ package v1alpha1 | |
| import ( | ||
| "fmt" | ||
| "path/filepath" | ||
| "slices" | ||
|
|
||
| "k8s.io/apimachinery/pkg/util/validation" | ||
|
|
||
| "github.com/crossplane/crossplane-runtime/v2/pkg/errors" | ||
| ) | ||
|
|
@@ -68,16 +71,59 @@ func (s *ProjectSpec) Validate() error { | |
| errs = append(errs, errors.New("architectures must not be empty")) | ||
| } | ||
|
|
||
| errs = append(errs, s.Schemas.Validate()...) | ||
|
|
||
| // Validate dependencies | ||
| for i, dep := range s.Dependencies { | ||
| if err := dep.Validate(); err != nil { | ||
| errs = append(errs, errors.Wrapf(err, "dependency %d", i)) | ||
| } | ||
| } | ||
|
|
||
| // Validate functions. Names must be unique across the list, regardless of | ||
| // source, since the function name is used to derive both the package | ||
| // metadata name and the OCI repository. | ||
| seen := make(map[string]int, len(s.Functions)) | ||
| for i, fn := range s.Functions { | ||
| if err := fn.Validate(); err != nil { | ||
| errs = append(errs, errors.Wrapf(err, "function %d", i)) | ||
| continue | ||
| } | ||
| name := fn.Name() | ||
| if first, ok := seen[name]; ok { | ||
| errs = append(errs, errors.Errorf("function %d: name %q is already used by function %d", i, name, first)) | ||
| continue | ||
| } | ||
| seen[name] = i | ||
| } | ||
|
|
||
| return errors.Join(errs...) | ||
| } | ||
|
|
||
| // Validate returns errors for an invalid ProjectSchemas. A nil receiver is | ||
| // valid (it means "generate schemas for all languages"); an explicitly empty | ||
| // Languages list is rejected because it would disable all schema generation, | ||
| // which is almost certainly a mistake. | ||
| func (s *ProjectSchemas) Validate() []error { | ||
| if s == nil { | ||
| return nil | ||
| } | ||
| if s.Languages == nil { | ||
| return nil | ||
| } | ||
| if len(s.Languages) == 0 { | ||
| return []error{errors.New("schemas.languages must not be empty when specified")} | ||
| } | ||
| supported := SupportedSchemaLanguages() | ||
| var errs []error | ||
| for i, lang := range s.Languages { | ||
| if !slices.Contains(supported, lang) { | ||
| errs = append(errs, errors.Errorf("schemas.languages[%d]: %q is not a supported schema language, must be one of %v", i, lang, supported)) | ||
| } | ||
|
Comment on lines
+121
to
+122
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. Make new validation errors more user-actionable Great coverage expansion. Could we rephrase these messages to be less technical and include a clear next step (e.g., what to change and retry), especially around unsupported languages and invalid function names? Example wording direction- schemas.languages[%d]: %q is not a supported schema language, must be one of %v
+ schemas.languages[%d]: %q is not supported. Choose one of %v and run the command again.
- name %q is not a valid DNS-1123 subdomain: %v
+ name %q is invalid for a function name. Use lowercase letters, numbers, '-' or '.', then try again.As per coding guidelines: "CRITICAL: Ensure all error messages are meaningful to end users, not just developers - avoid technical jargon, include context about what the user was trying to do, and suggest next steps when possible." Also applies to: 269-270, 287-293 🤖 Prompt for AI Agents |
||
| } | ||
| return errs | ||
| } | ||
|
|
||
| // Validate validates a dependency. | ||
| func (d *Dependency) Validate() error { | ||
| var errs []error | ||
|
|
@@ -172,3 +218,79 @@ func (k *K8sDependency) Validate() error { | |
|
|
||
| return errors.Join(errs...) | ||
| } | ||
|
|
||
| // Validate validates a Function declaration. | ||
| func (f *Function) Validate() error { | ||
| var errs []error | ||
|
|
||
| // Count non-nil sources to enforce that exactly one matches the | ||
| // discriminator. | ||
| sourceCount := 0 | ||
| if f.Directory != nil { | ||
| sourceCount++ | ||
| } | ||
| if f.Tarball != nil { | ||
| sourceCount++ | ||
| } | ||
| if sourceCount != 1 { | ||
| errs = append(errs, errors.New("exactly one source (directory or tarball) must be specified")) | ||
| } | ||
|
|
||
| switch f.Source { | ||
| case FunctionSourceDirectory: | ||
| if err := f.Directory.Validate(); err != nil { | ||
| errs = append(errs, fmt.Errorf("directory: %w", err)) | ||
| } | ||
| case FunctionSourceTarball: | ||
| if err := f.Tarball.Validate(); err != nil { | ||
| errs = append(errs, fmt.Errorf("tarball: %w", err)) | ||
| } | ||
|
Comment on lines
+241
to
+247
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify wrapper style in validation files.
rg -nP --type=go 'fmt\.Errorf\(".*: %w"' apis/dev/v1alpha1Repository: crossplane/cli Length of output: 580 Switch validation error wrapping to
Suggested patch case FunctionSourceDirectory:
if err := f.Directory.Validate(); err != nil {
- errs = append(errs, fmt.Errorf("directory: %w", err))
+ errs = append(errs, errors.Wrap(err, "directory"))
}
case FunctionSourceTarball:
if err := f.Tarball.Validate(); err != nil {
- errs = append(errs, fmt.Errorf("tarball: %w", err))
+ errs = append(errs, errors.Wrap(err, "tarball"))
}🤖 Prompt for AI Agents |
||
| case "": | ||
| errs = append(errs, errors.New("source must not be empty")) | ||
| default: | ||
| errs = append(errs, errors.Errorf("source %q is not supported, must be one of %q or %q", f.Source, FunctionSourceDirectory, FunctionSourceTarball)) | ||
| } | ||
|
|
||
| return errors.Join(errs...) | ||
| } | ||
|
|
||
| // Validate validates a FunctionDirectory. A nil receiver is invalid; this is | ||
| // the failure mode when a function is declared with source Directory but no | ||
| // directory field set. | ||
| func (d *FunctionDirectory) Validate() error { | ||
| if d == nil { | ||
| return errors.Errorf("source %q requires the directory field to be set", FunctionSourceDirectory) | ||
| } | ||
|
|
||
| var errs []error | ||
| if d.Name == "" { | ||
| errs = append(errs, errors.New("name must not be empty")) | ||
| } else if msgs := validation.IsDNS1123Subdomain(d.Name); len(msgs) > 0 { | ||
| errs = append(errs, errors.Errorf("name %q is not a valid DNS-1123 subdomain: %v", d.Name, msgs)) | ||
| } | ||
|
|
||
| return errors.Join(errs...) | ||
| } | ||
|
|
||
| // Validate validates a FunctionTarball. A nil receiver is invalid; this is | ||
| // the failure mode when a function is declared with source Tarball but no | ||
| // tarball field set. | ||
| func (t *FunctionTarball) Validate() error { | ||
| if t == nil { | ||
| return errors.Errorf("source %q requires the tarball field to be set", FunctionSourceTarball) | ||
| } | ||
|
|
||
| var errs []error | ||
| if t.Name == "" { | ||
| errs = append(errs, errors.New("name must not be empty")) | ||
| } else if msgs := validation.IsDNS1123Subdomain(t.Name); len(msgs) > 0 { | ||
| errs = append(errs, errors.Errorf("name %q is not a valid DNS-1123 subdomain: %v", t.Name, msgs)) | ||
| } | ||
| if t.PathPrefix == "" { | ||
| errs = append(errs, errors.New("pathPrefix must not be empty")) | ||
| } else if filepath.IsAbs(t.PathPrefix) { | ||
| errs = append(errs, errors.New("pathPrefix must be relative")) | ||
| } | ||
|
|
||
| return errors.Join(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.
Guard
Name()against a nil receiver for safer API usageNice addition overall—could we make this method nil-safe as well (like
GetLanguages) so external callers don’t panic on(*Function)(nil).Name()?Suggested patch
func (f *Function) Name() string { + if f == nil { + return "" + } switch f.Source { case FunctionSourceDirectory: if f.Directory == nil { return ""📝 Committable suggestion
🤖 Prompt for AI Agents