From deea806d9c048629e7adbc956d4bd2b213903259 Mon Sep 17 00:00:00 2001 From: Paul Meyer <49727155+katexochen@users.noreply.github.com> Date: Tue, 7 Feb 2023 15:19:59 +0100 Subject: [PATCH] Improve code sequences with multiple errs Signed-off-by: Paul Meyer <49727155+katexochen@users.noreply.github.com> --- cli/internal/cloudcmd/rollback.go | 20 +++++++--------- cli/internal/cmd/terminate.go | 8 +++---- debugd/internal/debugd/server/server.go | 4 +--- hack/image-measurement/main.go | 8 +++---- internal/config/config.go | 6 ++--- internal/config/config_test.go | 31 ++++++++++++++++++------- internal/config/validation.go | 4 ++++ internal/installer/installer_test.go | 12 ++++------ internal/versionsapi/cli/rm.go | 4 ++-- 9 files changed, 53 insertions(+), 44 deletions(-) diff --git a/cli/internal/cloudcmd/rollback.go b/cli/internal/cloudcmd/rollback.go index 0890ada9e..50ff1a6e9 100644 --- a/cli/internal/cloudcmd/rollback.go +++ b/cli/internal/cloudcmd/rollback.go @@ -38,12 +38,10 @@ type rollbackerTerraform struct { } func (r *rollbackerTerraform) rollback(ctx context.Context) error { - var err error - err = errors.Join(err, r.client.Destroy(ctx)) - if err == nil { - err = errors.Join(err, r.client.CleanUpWorkspace()) + if err := r.client.Destroy(ctx); err != nil { + return err } - return err + return r.client.CleanUpWorkspace() } type rollbackerQEMU struct { @@ -52,14 +50,12 @@ type rollbackerQEMU struct { createdWorkspace bool } -func (r *rollbackerQEMU) rollback(ctx context.Context) error { - var err error +func (r *rollbackerQEMU) rollback(ctx context.Context) (retErr error) { if r.createdWorkspace { - err = errors.Join(err, r.client.Destroy(ctx)) + retErr = r.client.Destroy(ctx) } - err = errors.Join(err, r.libvirt.Stop(ctx)) - if err == nil { - err = r.client.CleanUpWorkspace() + if retErr := errors.Join(retErr, r.libvirt.Stop(ctx)); retErr != nil { + return retErr } - return err + return r.client.CleanUpWorkspace() } diff --git a/cli/internal/cmd/terminate.go b/cli/internal/cmd/terminate.go index a49d331e3..e0c8955c6 100644 --- a/cli/internal/cmd/terminate.go +++ b/cli/internal/cmd/terminate.go @@ -76,14 +76,14 @@ func terminate(cmd *cobra.Command, terminator cloudTerminator, fileHandler file. cmd.Println("Your Constellation cluster was terminated successfully.") - var retErr error + var removeErr error if err := fileHandler.Remove(constants.AdminConfFilename); err != nil && !errors.Is(err, fs.ErrNotExist) { - retErr = errors.Join(err, fmt.Errorf("failed to remove file: '%s', please remove it manually", constants.AdminConfFilename)) + removeErr = errors.Join(err, fmt.Errorf("failed to remove file: '%s', please remove it manually", constants.AdminConfFilename)) } if err := fileHandler.Remove(constants.ClusterIDsFileName); err != nil && !errors.Is(err, fs.ErrNotExist) { - retErr = errors.Join(err, fmt.Errorf("failed to remove file: '%s', please remove it manually", constants.ClusterIDsFileName)) + removeErr = errors.Join(err, fmt.Errorf("failed to remove file: '%s', please remove it manually", constants.ClusterIDsFileName)) } - return retErr + return removeErr } diff --git a/debugd/internal/debugd/server/server.go b/debugd/internal/debugd/server/server.go index 8d9968498..dd1750275 100644 --- a/debugd/internal/debugd/server/server.go +++ b/debugd/internal/debugd/server/server.go @@ -116,9 +116,7 @@ func (s *debugdServer) UploadFiles(stream pb.Debugd_UploadFilesServer) error { } // continue on error to allow other units to be overridden err = s.serviceManager.OverrideServiceUnitExecStart(stream.Context(), file.OverrideServiceUnit, file.TargetPath) - if err != nil { - overrideUnitErr = errors.Join(overrideUnitErr, err) - } + overrideUnitErr = errors.Join(overrideUnitErr, err) } if overrideUnitErr != nil { diff --git a/hack/image-measurement/main.go b/hack/image-measurement/main.go index a7426ae12..9fac91a4d 100644 --- a/hack/image-measurement/main.go +++ b/hack/image-measurement/main.go @@ -35,7 +35,7 @@ type libvirtInstance struct { imagePath string } -func (l *libvirtInstance) uploadBaseImage(baseVolume *libvirt.StorageVol) (err error) { +func (l *libvirtInstance) uploadBaseImage(baseVolume *libvirt.StorageVol) (retErr error) { stream, err := l.conn.NewStream(libvirt.STREAM_NONBLOCK) if err != nil { return err @@ -46,7 +46,7 @@ func (l *libvirtInstance) uploadBaseImage(baseVolume *libvirt.StorageVol) (err e return fmt.Errorf("error while opening %s: %s", l.imagePath, err) } defer func() { - err = errors.Join(err, file.Close()) + retErr = errors.Join(err, file.Close()) }() fi, err := file.Stat() @@ -282,7 +282,7 @@ func (l *libvirtInstance) deleteLibvirtInstance() error { return err } -func (l *libvirtInstance) obtainMeasurements() (measurements measurements.M, err error) { +func (l *libvirtInstance) obtainMeasurements() (measurements measurements.M, retErr error) { // sanity check if err := l.deleteLibvirtInstance(); err != nil { return nil, err @@ -295,7 +295,7 @@ func (l *libvirtInstance) obtainMeasurements() (measurements measurements.M, err } }() defer func() { - err = errors.Join(err, l.deleteLibvirtInstance()) + retErr = errors.Join(retErr, l.deleteLibvirtInstance()) }() if err := l.createLibvirtInstance(); err != nil { return nil, err diff --git a/internal/config/config.go b/internal/config/config.go index 2617366ed..988debfe4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -580,13 +580,13 @@ func (c *Config) Validate(force bool) error { return nil } - var errs validator.ValidationErrors - if !errors.As(err, &errs) { + var validationErrs validator.ValidationErrors + if !errors.As(err, &validationErrs) { return err } var validationErrMsgs []string - for _, e := range errs { + for _, e := range validationErrs { validationErrMsgs = append(validationErrMsgs, e.Translate(trans)) } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ccd63ba14..a8ae81fc4 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -184,13 +184,19 @@ func TestNewWithDefaultOptions(t *testing.T) { } func TestValidate(t *testing.T) { + const defaultErrCount = 21 // expect this number of error messages by default because user-specific values are not set and multiple providers are defined by default + const azErrCount = 9 + const gcpErrCount = 6 + testCases := map[string]struct { - cnf *Config - wantErr bool + cnf *Config + wantErr bool + wantErrCount int }{ "default config is not valid": { - cnf: Default(), - wantErr: true, + cnf: Default(), + wantErr: true, + wantErrCount: defaultErrCount, }, "v0 is one error": { cnf: func() *Config { @@ -198,7 +204,8 @@ func TestValidate(t *testing.T) { cnf.Version = "v0" return cnf }(), - wantErr: true, + wantErr: true, + wantErrCount: defaultErrCount + 1, }, "v0 and negative state disk are two errors": { cnf: func() *Config { @@ -207,7 +214,8 @@ func TestValidate(t *testing.T) { cnf.StateDiskSizeGB = -1 return cnf }(), - wantErr: true, + wantErr: true, + wantErrCount: defaultErrCount + 2, }, "default Azure config is not valid": { cnf: func() *Config { @@ -217,7 +225,8 @@ func TestValidate(t *testing.T) { cnf.Provider.Azure = az return cnf }(), - wantErr: true, + wantErr: true, + wantErrCount: azErrCount, }, "Azure config with all required fields is valid": { cnf: func() *Config { @@ -245,7 +254,8 @@ func TestValidate(t *testing.T) { cnf.Provider.GCP = gcp return cnf }(), - wantErr: true, + wantErr: true, + wantErrCount: gcpErrCount, }, "GCP config with all required fields is valid": { cnf: func() *Config { @@ -287,10 +297,15 @@ func TestValidate(t *testing.T) { for name, tc := range testCases { t.Run(name, func(t *testing.T) { assert := assert.New(t) + require := require.New(t) err := tc.cnf.Validate(false) + if tc.wantErr { assert.Error(err) + var valErr *ValidationError + require.ErrorAs(err, &valErr) + assert.Equal(tc.wantErrCount, valErr.messagesCount()) return } assert.NoError(err) diff --git a/internal/config/validation.go b/internal/config/validation.go index 0be597544..780d133ea 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -47,6 +47,10 @@ func (e *ValidationError) LongMessage() string { return msg } +func (e *ValidationError) messagesCount() int { + return len(e.validationErrMsgs) +} + func registerInvalidK8sVersionError(ut ut.Translator) error { return ut.Add("supported_k8s_version", "{0} specifies an unsupported Kubernetes version. {1}", true) } diff --git a/internal/installer/installer_test.go b/internal/installer/installer_test.go index 7490f1dce..b42dbb849 100644 --- a/internal/installer/installer_test.go +++ b/internal/installer/installer_test.go @@ -735,14 +735,10 @@ func (w *tarGzWriter) Bytes() []byte { return w.buf.Bytes() } -func (w *tarGzWriter) Close() (result error) { - if err := w.tarWriter.Close(); err != nil { - result = errors.Join(result, err) - } - if err := w.gzWriter.Close(); err != nil { - result = errors.Join(result, err) - } - return result +func (w *tarGzWriter) Close() (retErr error) { + retErr = errors.Join(retErr, w.tarWriter.Close()) + retErr = errors.Join(retErr, w.gzWriter.Close()) + return retErr } func stringPtr(s string) *string { diff --git a/internal/versionsapi/cli/rm.go b/internal/versionsapi/cli/rm.go index ffb4527be..ee123cda9 100644 --- a/internal/versionsapi/cli/rm.go +++ b/internal/versionsapi/cli/rm.go @@ -152,8 +152,6 @@ func deleteSingleVersion(ctx context.Context, clients rmImageClients, ver versio } func deleteRef(ctx context.Context, clients rmImageClients, ref string, dryrun bool, log *logger.Logger) error { - var retErr error - var vers []versionsapi.Version for _, stream := range []string{"nightly", "console", "debug"} { log.Infof("Listing versions of stream %s", stream) @@ -179,6 +177,8 @@ func deleteRef(ctx context.Context, clients rmImageClients, ref string, dryrun b } log.Infof("Found %d versions to delete", len(vers)) + var retErr error + for _, ver := range vers { if err := deleteImage(ctx, clients, ver, dryrun, log); err != nil { retErr = errors.Join(retErr, fmt.Errorf("deleting images for version %s: %w", ver.Version, err))