Skip to content

Commit f4bd348

Browse files
thaJeztahthaJeztah
committed
validate: refactor multi-errors
Use a slightly more idiomatic approach, and collect errors before joining them. This prevents deeply nested error trees. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
1 parent eea4efe commit f4bd348

File tree

1 file changed

+74
-77
lines changed

1 file changed

+74
-77
lines changed

‎validate/validate.go‎

Lines changed: 74 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -104,22 +104,27 @@ func NewValidatorFromPath(bundlePath string, hostSpecific bool, platform string)
104104

105105
// CheckAll checks all parts of runtime bundle
106106
func (v *Validator) CheckAll() error {
107-
errs := errors.Join(
108-
v.CheckJSONSchema(),
109-
v.CheckPlatform(),
110-
v.CheckRoot(),
111-
v.CheckMandatoryFields(),
112-
v.CheckSemVer(),
113-
v.CheckMounts(),
114-
v.CheckProcess(),
115-
v.CheckLinux(),
116-
v.CheckAnnotations(),
117-
)
107+
checks := []func() error{
108+
v.CheckJSONSchema,
109+
v.CheckPlatform,
110+
v.CheckRoot,
111+
v.CheckMandatoryFields,
112+
v.CheckSemVer,
113+
v.CheckMounts,
114+
v.CheckProcess,
115+
v.CheckLinux,
116+
v.CheckAnnotations,
117+
}
118118
if v.platform == "linux" || v.platform == "solaris" {
119-
errs = errors.Join(errs, v.CheckHooks())
119+
checks = append(checks, v.CheckHooks)
120120
}
121-
122-
return errs
121+
var errs []error
122+
for _, check := range checks {
123+
if err := check(); err != nil {
124+
errs = append(errs, err)
125+
}
126+
}
127+
return errors.Join(errs...)
123128
}
124129

125130
// JSONSchemaURL returns the URL for the JSON Schema specifying the
@@ -140,84 +145,75 @@ func JSONSchemaURL(version string) (url string, err error) {
140145
// CheckJSONSchema validates the configuration against the
141146
// runtime-spec JSON Schema, using the version of the schema that
142147
// matches the configuration's declared version.
143-
func (v *Validator) CheckJSONSchema() (errs error) {
148+
func (v *Validator) CheckJSONSchema() error {
144149
logrus.Debugf("check JSON schema")
145150

146151
url, err := JSONSchemaURL(strings.TrimSuffix(v.spec.Version, "-dev"))
147152
if err != nil {
148-
errs = errors.Join(errs, err)
149-
return errs
153+
return err
150154
}
151155

152156
schemaLoader := gojsonschema.NewReferenceLoader(url)
153157
documentLoader := gojsonschema.NewGoLoader(v.spec)
154158
result, err := gojsonschema.Validate(schemaLoader, documentLoader)
155159
if err != nil {
156-
errs = errors.Join(errs, err)
157-
return errs
160+
return err
158161
}
159162

160163
if !result.Valid() {
164+
var errs []error
161165
for _, resultError := range result.Errors() {
162-
errs = errors.Join(errs, errors.New(resultError.String()))
166+
errs = append(errs, errors.New(resultError.String()))
163167
}
168+
return errors.Join(errs...)
164169
}
165170

166-
return errs
171+
return nil
167172
}
168173

169174
// CheckRoot checks status of v.spec.Root
170-
func (v *Validator) CheckRoot() (errs error) {
175+
func (v *Validator) CheckRoot() error {
171176
logrus.Debugf("check root")
172177

173178
if v.platform == "windows" {
174179
if v.spec.Windows != nil && v.spec.Windows.HyperV != nil {
175180
if v.spec.Root != nil {
176-
errs = errors.Join(errs,
177-
specerror.NewError(specerror.RootOnHyperVNotSet, fmt.Errorf("for Hyper-V containers, Root must not be set"), rspec.Version))
181+
return specerror.NewError(specerror.RootOnHyperVNotSet, fmt.Errorf("for Hyper-V containers, Root must not be set"), rspec.Version)
178182
}
179-
return
183+
return nil
180184
} else if v.spec.Root == nil {
181-
errs = errors.Join(errs,
182-
specerror.NewError(specerror.RootOnWindowsRequired, fmt.Errorf("on Windows, for Windows Server Containers, Root is REQUIRED"), rspec.Version))
183-
return
185+
return specerror.NewError(specerror.RootOnWindowsRequired, fmt.Errorf("on Windows, for Windows Server Containers, Root is REQUIRED"), rspec.Version)
184186
}
185187
} else if v.spec.Root == nil {
186-
errs = errors.Join(errs,
187-
specerror.NewError(specerror.RootOnNonWindowsRequired, fmt.Errorf("on all other platforms, Root is REQUIRED"), rspec.Version))
188-
return
188+
return specerror.NewError(specerror.RootOnNonWindowsRequired, fmt.Errorf("on all other platforms, Root is REQUIRED"), rspec.Version)
189189
}
190190

191+
var errs []error
191192
if v.platform == "windows" {
192193
matched, err := regexp.MatchString(`\\\\[?]\\Volume[{][a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}[}]\\`, v.spec.Root.Path)
193194
if err != nil {
194-
errs = errors.Join(errs, err)
195+
errs = append(errs, err)
195196
} else if !matched {
196-
errs = errors.Join(errs,
197-
specerror.NewError(specerror.RootPathOnWindowsGUID, fmt.Errorf("root.path is %q, but it MUST be a volume GUID path when target platform is windows", v.spec.Root.Path), rspec.Version))
197+
errs = append(errs, specerror.NewError(specerror.RootPathOnWindowsGUID, fmt.Errorf("root.path is %q, but it MUST be a volume GUID path when target platform is windows", v.spec.Root.Path), rspec.Version))
198198
}
199199

200200
if v.spec.Root.Readonly {
201-
errs = errors.Join(errs,
202-
specerror.NewError(specerror.RootReadonlyOnWindowsFalse, fmt.Errorf("root.readonly field MUST be omitted or false when target platform is windows"), rspec.Version))
201+
errs = append(errs, specerror.NewError(specerror.RootReadonlyOnWindowsFalse, fmt.Errorf("root.readonly field MUST be omitted or false when target platform is windows"), rspec.Version))
203202
}
204203

205-
return
204+
return errors.Join(errs...)
206205
}
207206

208207
absBundlePath, err := filepath.Abs(v.bundlePath)
209208
if err != nil {
210-
errs = errors.Join(errs, fmt.Errorf("unable to convert %q to an absolute path", v.bundlePath))
211-
return
209+
return fmt.Errorf("unable to convert %q to an absolute path", v.bundlePath)
212210
}
213211

214212
if filepath.Base(v.spec.Root.Path) != "rootfs" {
215-
errs = errors.Join(errs,
216-
specerror.NewError(specerror.RootPathOnPosixConvention, fmt.Errorf("path name should be the conventional 'rootfs'"), rspec.Version))
213+
errs = append(errs, specerror.NewError(specerror.RootPathOnPosixConvention, fmt.Errorf("path name should be the conventional 'rootfs'"), rspec.Version))
217214
}
218215

219-
var rootfsPath string
220-
var absRootPath string
216+
var rootfsPath, absRootPath string
221217
if filepath.IsAbs(v.spec.Root.Path) {
222218
rootfsPath = v.spec.Root.Path
223219
absRootPath = filepath.Clean(rootfsPath)
@@ -226,92 +222,93 @@ func (v *Validator) CheckRoot() (errs error) {
226222
rootfsPath = filepath.Join(v.bundlePath, v.spec.Root.Path)
227223
absRootPath, err = filepath.Abs(rootfsPath)
228224
if err != nil {
229-
errs = errors.Join(errs, fmt.Errorf("unable to convert %q to an absolute path", rootfsPath))
230-
return
225+
errs = append(errs, fmt.Errorf("unable to convert %q to an absolute path", rootfsPath))
226+
return errors.Join(errs...)
231227
}
232228
}
233229

234230
if fi, err := os.Stat(rootfsPath); err != nil {
235-
errs = errors.Join(errs,
236-
specerror.NewError(specerror.RootPathExist, fmt.Errorf("cannot find the root path %q", rootfsPath), rspec.Version))
231+
errs = append(errs, specerror.NewError(specerror.RootPathExist, fmt.Errorf("cannot find the root path %q", rootfsPath), rspec.Version))
237232
} else if !fi.IsDir() {
238-
errs = errors.Join(errs,
239-
specerror.NewError(specerror.RootPathExist, fmt.Errorf("root.path %q is not a directory", rootfsPath), rspec.Version))
233+
errs = append(errs, specerror.NewError(specerror.RootPathExist, fmt.Errorf("root.path %q is not a directory", rootfsPath), rspec.Version))
240234
}
241235

242236
rootParent := filepath.Dir(absRootPath)
243237
if absRootPath == string(filepath.Separator) || rootParent != absBundlePath {
244-
errs = errors.Join(errs,
245-
specerror.NewError(specerror.ArtifactsInSingleDir, fmt.Errorf("root.path is %q, but it MUST be a child of %q", v.spec.Root.Path, absBundlePath), rspec.Version))
238+
errs = append(errs, specerror.NewError(specerror.ArtifactsInSingleDir, fmt.Errorf("root.path is %q, but it MUST be a child of %q", v.spec.Root.Path, absBundlePath), rspec.Version))
246239
}
247240

248-
return
241+
return errors.Join(errs...)
249242
}
250243

251244
// CheckSemVer checks v.spec.Version
252-
func (v *Validator) CheckSemVer() (errs error) {
245+
func (v *Validator) CheckSemVer() error {
253246
logrus.Debugf("check semver")
254247

255248
version := v.spec.Version
256249
_, err := semver.Parse(version)
257250
if err != nil {
258-
errs = errors.Join(errs,
259-
specerror.NewError(specerror.SpecVersionInSemVer, fmt.Errorf("%q is not valid SemVer: %s", version, err.Error()), rspec.Version))
251+
return specerror.NewError(specerror.SpecVersionInSemVer, fmt.Errorf("%q is not valid SemVer: %s", version, err.Error()), rspec.Version)
260252
}
261253
if version != rspec.Version {
262-
errs = errors.Join(errs, fmt.Errorf("validate currently only handles version %s, but the supplied configuration targets %s", rspec.Version, version))
254+
return fmt.Errorf("validate currently only handles version %s, but the supplied configuration targets %s", rspec.Version, version)
263255
}
264256

265-
return
257+
return nil
266258
}
267259

268260
// CheckHooks check v.spec.Hooks
269-
func (v *Validator) CheckHooks() (errs error) {
261+
func (v *Validator) CheckHooks() error {
270262
logrus.Debugf("check hooks")
271263

272264
if v.platform != "linux" && v.platform != "solaris" {
273-
errs = errors.Join(errs, fmt.Errorf("For %q platform, the configuration structure does not support hooks", v.platform))
274-
return
265+
return fmt.Errorf("for %q platform, the configuration structure does not support hooks", v.platform)
275266
}
276267

268+
var errs []error
277269
if v.spec.Hooks != nil {
278-
errs = errors.Join(errs, v.checkEventHooks("prestart", v.spec.Hooks.Prestart, v.HostSpecific)) //nolint:staticcheck // Ignore SA1019: v.Spec.Hooks.Prestart is deprecated
279-
errs = errors.Join(errs, v.checkEventHooks("poststart", v.spec.Hooks.Poststart, v.HostSpecific))
280-
errs = errors.Join(errs, v.checkEventHooks("poststop", v.spec.Hooks.Poststop, v.HostSpecific))
270+
if err := v.checkEventHooks("prestart", v.spec.Hooks.Prestart, v.HostSpecific); err != nil { //nolint:staticcheck // Ignore SA1019: v.Spec.Hooks.Prestart is deprecated
271+
errs = append(errs, err)
272+
}
273+
if err := v.checkEventHooks("poststart", v.spec.Hooks.Poststart, v.HostSpecific); err != nil {
274+
errs = append(errs, err)
275+
}
276+
if err := v.checkEventHooks("poststop", v.spec.Hooks.Poststop, v.HostSpecific); err != nil {
277+
errs = append(errs, err)
278+
}
281279
}
282280

283-
return
281+
return errors.Join(errs...)
284282
}
285283

286-
func (v *Validator) checkEventHooks(hookType string, hooks []rspec.Hook, hostSpecific bool) (errs error) {
284+
func (v *Validator) checkEventHooks(hookType string, hooks []rspec.Hook, hostSpecific bool) error {
285+
var retErrs []error
287286
for i, hook := range hooks {
287+
var errs []error
288288
if !osFilepath.IsAbs(v.platform, hook.Path) {
289-
errs = errors.Join(errs,
290-
specerror.NewError(
291-
specerror.PosixHooksPathAbs,
292-
fmt.Errorf("hooks.%s[%d].path %v: is not absolute path",
293-
hookType, i, hook.Path),
294-
rspec.Version))
289+
errs = append(errs, specerror.NewError(specerror.PosixHooksPathAbs, fmt.Errorf("hooks.%s[%d].path %v: is not absolute path", hookType, i, hook.Path), rspec.Version))
295290
}
296291

297292
if hostSpecific {
298293
fi, err := os.Stat(hook.Path)
299294
if err != nil {
300-
errs = errors.Join(errs, fmt.Errorf("cannot find %s hook: %v", hookType, hook.Path))
301-
}
302-
if fi.Mode()&0o111 == 0 {
303-
errs = errors.Join(errs, fmt.Errorf("the %s hook %v: is not executable", hookType, hook.Path))
295+
errs = append(errs, fmt.Errorf("cannot find %s hook: %v", hookType, hook.Path))
296+
} else if fi.Mode()&0o111 == 0 {
297+
errs = append(errs, fmt.Errorf("the %s hook %v: is not executable", hookType, hook.Path))
304298
}
305299
}
306300

307301
for _, env := range hook.Env {
308302
if !envValid(env) {
309-
errs = errors.Join(errs, fmt.Errorf("env %q for hook %v is in the invalid form", env, hook.Path))
303+
errs = append(errs, fmt.Errorf("env %q for hook %v is in the invalid form", env, hook.Path))
310304
}
311305
}
306+
if err := errors.Join(errs...); err != nil {
307+
retErrs = append(retErrs, err)
308+
}
312309
}
313310

314-
return
311+
return errors.Join(retErrs...)
315312
}
316313

317314
// CheckProcess checks v.spec.Process

0 commit comments

Comments
 (0)