-
Notifications
You must be signed in to change notification settings - Fork 91
validator: introduce mode-aware constructors and validation API #1177
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 8 commits
c2b6ed7
1166296
892c661
a5c8cfc
30cfd76
d482f31
2769edc
324c4a0
d999602
fe83a0a
491658c
15750f5
fcaf8c2
f4e266e
e9d0013
cbd3b7c
0f275c0
d1efd03
4233331
da2a325
8539209
b0049ff
d4508b1
8d441f1
15cc91e
cc5e273
2a3fef8
d8a2a6c
ebac0c7
d895e49
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 |
|---|---|---|
|
|
@@ -6,3 +6,5 @@ fuzz | |
| /build/ | ||
| .vscode/ | ||
| .DS_Store | ||
|
|
||
| .scratch/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| // or more contributor license agreements. Licensed under the Elastic License; | ||
| // you may not use this file except in compliance with the Elastic License. | ||
|
|
||
| package modes | ||
|
|
||
| // Mode represents the validation mode used when validating a package. | ||
| type Mode string | ||
|
|
||
| // Validation modes. The public API re-exports these as validator.Mode* constants. | ||
| const ( | ||
| Legacy Mode = "legacy" | ||
| Source Mode = "source" | ||
| Build Mode = "build" | ||
| ) | ||
|
|
||
| // Valid reports whether m is a recognised validation mode. | ||
| func (m Mode) Valid() bool { | ||
| switch m { | ||
| case Legacy, Source, Build: | ||
| return true | ||
| } | ||
| return false | ||
| } |
|
Member
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. Nit. "api" is redundant here, any public method is going to be part of the package API 🙂 This could belong to |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,183 @@ | ||||||||||||||
| // Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||||||||||||||
| // or more contributor license agreements. Licensed under the Elastic License; | ||||||||||||||
| // you may not use this file except in compliance with the Elastic License. | ||||||||||||||
|
|
||||||||||||||
| package validator | ||||||||||||||
|
|
||||||||||||||
| import ( | ||||||||||||||
| "archive/zip" | ||||||||||||||
| "errors" | ||||||||||||||
| "fmt" | ||||||||||||||
| "io" | ||||||||||||||
| "io/fs" | ||||||||||||||
| "os" | ||||||||||||||
|
|
||||||||||||||
| "github.com/elastic/package-spec/v3/code/go/internal/linkedfiles" | ||||||||||||||
| "github.com/elastic/package-spec/v3/code/go/internal/packages" | ||||||||||||||
| internalvalidator "github.com/elastic/package-spec/v3/code/go/internal/validator" | ||||||||||||||
| "github.com/elastic/package-spec/v3/code/go/internal/validator/common" | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| // Validator holds the configuration for a package validation run. | ||||||||||||||
| // Create one with NewFromPath, NewFromZip, or NewFromFS, then call Validate. | ||||||||||||||
| type Validator struct { | ||||||||||||||
| mode Mode | ||||||||||||||
| location string | ||||||||||||||
| fsys fs.FS | ||||||||||||||
| warningsAsErrors bool | ||||||||||||||
| // closer is non-nil when the Validator owns a resource (e.g. a zip reader) | ||||||||||||||
| // that must be released after Validate returns. | ||||||||||||||
| closer io.Closer | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Option configures a Validator. | ||||||||||||||
| type Option func(*Validator) | ||||||||||||||
|
|
||||||||||||||
| // WithWarningsAsErrors makes validation warnings count as errors when v is true. | ||||||||||||||
| func WithWarningsAsErrors(v bool) Option { | ||||||||||||||
| return func(va *Validator) { va.warningsAsErrors = v } | ||||||||||||||
|
teresaromero marked this conversation as resolved.
Outdated
|
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // NewFromPath returns a Validator for the package rooted at packageRootPath. | ||||||||||||||
| // | ||||||||||||||
| // For ModeLegacy and ModeSource the filesystem honours linked (.link) files; | ||||||||||||||
| // for ModeBuild linked files are blocked (matching a built package artifact). | ||||||||||||||
| func NewFromPath(mode Mode, packageRootPath string, opts ...Option) (*Validator, error) { | ||||||||||||||
|
Member
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. Nit. Configuration structs are usually easier to discover than functional options, and use to be simpler.
Suggested change
Contributor
Author
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. This config is used for the warnings, which is read from the env, so i removed the options/config and read it directly from the env. Changed the test case too to use t.Env instead of a local variable.
Member
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. Part of the original design in #549 was actually to introduce the "Option C" API shape, a new "Validator" object that can be somehow customized with different options. With current code we are back to something like "Option A": methods with the mode as parameter. Only that we have one method per mode instead of having the mode as a parameter. I still think it would be valuable to introduce this "Validator" object. Having the object we can add configuration options later without needing to add more functions to the package, or parameters to these functions. For this initial version it could have |
||||||||||||||
| info, err := os.Stat(packageRootPath) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("invalid package path %q: %w", packageRootPath, err) | ||||||||||||||
| } | ||||||||||||||
| if !info.IsDir() { | ||||||||||||||
| return nil, fmt.Errorf("invalid package path %q: not a directory", packageRootPath) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| var fsys fs.FS | ||||||||||||||
| if mode == ModeBuild { | ||||||||||||||
| fsys = linkedfiles.NewBlockFS(os.DirFS(packageRootPath)) | ||||||||||||||
| } else { | ||||||||||||||
| // ModeLegacy and ModeSource: resolve linked files transparently. | ||||||||||||||
| fsys = linkedfiles.NewFS(packageRootPath, os.DirFS(packageRootPath)) | ||||||||||||||
| } | ||||||||||||||
| return buildValidator(mode, packageRootPath, fsys, nil, opts), nil | ||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // NewFromZip returns a Validator for the package stored in the zip file at zipPath. | ||||||||||||||
| // | ||||||||||||||
| // Zip files always contain built packages — they are the output format produced | ||||||||||||||
| // by elastic-package build and consumed by the package registry and Fleet. | ||||||||||||||
| // Validation always runs in ModeBuild; source-only artifacts (_dev/, .link files, | ||||||||||||||
| // external: ecs references) are therefore rejected. | ||||||||||||||
| // | ||||||||||||||
| // The returned Validator owns the underlying zip reader; calling Validate closes it. | ||||||||||||||
| // Do not call Validate more than once on a Validator created by NewFromZip. | ||||||||||||||
| func NewFromZip(zipPath string, opts ...Option) (_ *Validator, err error) { | ||||||||||||||
| return newFromZip(ModeBuild, zipPath, opts...) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // newFromZip is the internal constructor used by both NewFromZip (ModeBuild) and | ||||||||||||||
| // the deprecated ValidateFromZip wrapper (ModeLegacy). | ||||||||||||||
| func newFromZip(mode Mode, zipPath string, opts ...Option) (_ *Validator, err error) { | ||||||||||||||
| r, openErr := zip.OpenReader(zipPath) | ||||||||||||||
| if openErr != nil { | ||||||||||||||
| return nil, fmt.Errorf("failed to open zip file (%s): %w", zipPath, openErr) | ||||||||||||||
| } | ||||||||||||||
| // Close the reader on any error path; on success the Validator takes ownership. | ||||||||||||||
| defer func() { | ||||||||||||||
| if err != nil { | ||||||||||||||
| if cerr := r.Close(); cerr != nil { | ||||||||||||||
| err = errors.Join(err, cerr) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| }() | ||||||||||||||
|
|
||||||||||||||
| dirs, err := fs.ReadDir(r, ".") | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("failed to read root directory in zip file (%s): %w", zipPath, err) | ||||||||||||||
| } | ||||||||||||||
| if len(dirs) != 1 { | ||||||||||||||
| return nil, fmt.Errorf("a single directory is expected in zip file, %d found", len(dirs)) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| subDir, err := fs.Sub(r, dirs[0].Name()) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, err | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Zip archives contain built packages; linked files are always blocked. | ||||||||||||||
| fsys := linkedfiles.NewBlockFS(subDir) | ||||||||||||||
| return buildValidator(mode, zipPath, fsys, r, opts), nil | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // NewFromFS returns a Validator for the package accessible through fsys at location. | ||||||||||||||
| // | ||||||||||||||
| // For ModeSource, if fsys is not already a *linkedfiles.FS, it is wrapped with | ||||||||||||||
| // linkedfiles.NewFS so that .link files are resolved transparently (consistent | ||||||||||||||
| // with ModeSource's contract). For ModeLegacy and ModeBuild, a BlockFS is applied | ||||||||||||||
| // instead — matching the behaviour of ValidateFromFS. | ||||||||||||||
| func NewFromFS(mode Mode, location string, fsys fs.FS, opts ...Option) (*Validator, error) { | ||||||||||||||
|
jsoriano marked this conversation as resolved.
Outdated
|
||||||||||||||
| info, err := fs.Stat(fsys, ".") | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("invalid package filesystem at %q: %w", location, err) | ||||||||||||||
| } | ||||||||||||||
| if !info.IsDir() { | ||||||||||||||
| return nil, fmt.Errorf("invalid package filesystem at %q: root is not a directory", location) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if _, ok := fsys.(*linkedfiles.FS); !ok { | ||||||||||||||
| if mode == ModeSource { | ||||||||||||||
| fsys = linkedfiles.NewFS(location, fsys) | ||||||||||||||
|
Member
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. On what cases we receive a linked FS here? Nit. Maybe |
||||||||||||||
| } else { | ||||||||||||||
| fsys = linkedfiles.NewBlockFS(fsys) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||
| return buildValidator(mode, location, fsys, nil, opts), nil | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // buildValidator is the shared internal constructor. | ||||||||||||||
| func buildValidator(mode Mode, location string, fsys fs.FS, closer io.Closer, opts []Option) *Validator { | ||||||||||||||
| v := &Validator{ | ||||||||||||||
| mode: mode, | ||||||||||||||
| location: location, | ||||||||||||||
| fsys: fsys, | ||||||||||||||
| warningsAsErrors: common.IsDefinedWarningsAsErrors(), | ||||||||||||||
| closer: closer, | ||||||||||||||
| } | ||||||||||||||
| for _, opt := range opts { | ||||||||||||||
| opt(v) | ||||||||||||||
| } | ||||||||||||||
| return v | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Validate runs package validation and returns any errors encountered. | ||||||||||||||
| // | ||||||||||||||
| // If the Validator was created by NewFromZip it owns an open zip reader; | ||||||||||||||
| // Validate closes it on return, so Validate must not be called more than once | ||||||||||||||
| // on such a Validator. | ||||||||||||||
| func (v *Validator) Validate() (err error) { | ||||||||||||||
| if v.closer != nil { | ||||||||||||||
| defer func() { | ||||||||||||||
| if cerr := v.closer.Close(); cerr != nil { | ||||||||||||||
| err = errors.Join(err, cerr) | ||||||||||||||
| } | ||||||||||||||
| }() | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| pkg, err := packages.NewPackageFromFS(v.location, v.fsys) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| } | ||||||||||||||
| if pkg.SpecVersion == nil { | ||||||||||||||
| return errors.New("could not determine specification version for package") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| spec, err := internalvalidator.NewSpec(*pkg.SpecVersion, v.mode) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| } | ||||||||||||||
| spec.WarningsAsErrors = v.warningsAsErrors | ||||||||||||||
|
|
||||||||||||||
| if errs := spec.ValidatePackage(*pkg); len(errs) > 0 { | ||||||||||||||
| return errs | ||||||||||||||
| } | ||||||||||||||
| return nil | ||||||||||||||
| } | ||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.