From c8a883be10d62158e3906a23bd8757cb8a5db12d Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 31 Aug 2023 13:36:27 -0700 Subject: [PATCH 1/2] Refactor inline error checking for basic cases --- internal/framework/controller/reconciler.go | 3 +-- internal/framework/controller/register.go | 9 +++------ internal/framework/events/first_eventbatch_preparer.go | 6 ++---- internal/framework/helpers/helpers.go | 3 +-- internal/framework/status/updater.go | 6 ++---- internal/mode/provisioner/deployment.go | 3 +-- internal/mode/provisioner/handler.go | 6 ++---- internal/mode/provisioner/manager.go | 3 +-- internal/mode/static/manager.go | 3 +-- internal/mode/static/nginx/config/execute.go | 3 +-- internal/mode/static/nginx/runtime/manager.go | 3 +-- internal/mode/static/state/graph/gateway_listener.go | 3 +-- .../mode/static/state/resolver/service_resolver_test.go | 3 +-- 13 files changed, 18 insertions(+), 36 deletions(-) diff --git a/internal/framework/controller/reconciler.go b/internal/framework/controller/reconciler.go index 5766721c7..d3bc00058 100644 --- a/internal/framework/controller/reconciler.go +++ b/internal/framework/controller/reconciler.go @@ -74,8 +74,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } obj := newObject(r.cfg.ObjectType) - err := r.cfg.Getter.Get(ctx, req.NamespacedName, obj) - if err != nil { + if err := r.cfg.Getter.Get(ctx, req.NamespacedName, obj); err != nil { if !apierrors.IsNotFound(err) { logger.Error(err, "Failed to get the resource") return reconcile.Result{}, err diff --git a/internal/framework/controller/register.go b/internal/framework/controller/register.go index e52e348e4..0bf7ea575 100644 --- a/internal/framework/controller/register.go +++ b/internal/framework/controller/register.go @@ -82,8 +82,7 @@ func Register( } for field, indexerFunc := range cfg.fieldIndices { - err := addIndex(ctx, mgr.GetFieldIndexer(), objectType, field, indexerFunc) - if err != nil { + if err := addIndex(ctx, mgr.GetFieldIndexer(), objectType, field, indexerFunc); err != nil { return err } } @@ -101,8 +100,7 @@ func Register( NamespacedNameFilter: cfg.namespacedNameFilter, } - err := builder.Complete(cfg.newReconciler(recCfg)) - if err != nil { + if err := builder.Complete(cfg.newReconciler(recCfg)); err != nil { return fmt.Errorf("cannot build a controller for %T: %w", objectType, err) } @@ -119,8 +117,7 @@ func addIndex( c, cancel := context.WithTimeout(ctx, addIndexFieldTimeout) defer cancel() - err := indexer.IndexField(c, objectType, field, indexerFunc) - if err != nil { + if err := indexer.IndexField(c, objectType, field, indexerFunc); err != nil { return fmt.Errorf("failed to add index for %T for field %s: %w", objectType, field, err) } diff --git a/internal/framework/events/first_eventbatch_preparer.go b/internal/framework/events/first_eventbatch_preparer.go index bc1a95767..d905239b8 100644 --- a/internal/framework/events/first_eventbatch_preparer.go +++ b/internal/framework/events/first_eventbatch_preparer.go @@ -70,8 +70,7 @@ func (p *FirstEventBatchPreparerImpl) Prepare(ctx context.Context) (EventBatch, total := 0 for _, list := range p.objectLists { - err := p.reader.List(ctx, list) - if err != nil { + if err := p.reader.List(ctx, list); err != nil { return nil, err } @@ -85,8 +84,7 @@ func (p *FirstEventBatchPreparerImpl) Prepare(ctx context.Context) (EventBatch, for _, obj := range p.objects { key := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} - err := p.reader.Get(ctx, key, obj) - if err != nil { + if err := p.reader.Get(ctx, key, obj); err != nil { if !apierrors.IsNotFound(err) { return nil, err } diff --git a/internal/framework/helpers/helpers.go b/internal/framework/helpers/helpers.go index 5bcc2d268..f312e1f1c 100644 --- a/internal/framework/helpers/helpers.go +++ b/internal/framework/helpers/helpers.go @@ -35,8 +35,7 @@ func PrepareTimeForFakeClient(t metav1.Time) metav1.Time { panic(fmt.Errorf("failed to marshal time: %w", err)) } - err = t.Unmarshal(bytes) - if err != nil { + if err = t.Unmarshal(bytes); err != nil { panic(fmt.Errorf("failed to unmarshal time: %w", err)) } diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 02bbfbe01..49c29651c 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -146,8 +146,7 @@ func (upd *updaterImpl) update( // Otherwise, the Update status API call can fail. // Note: the default client uses a cache for reads, so we're not making an unnecessary API call here. // the default is configurable in the Manager options. - err := upd.cfg.Client.Get(ctx, nsname, obj) - if err != nil { + if err := upd.cfg.Client.Get(ctx, nsname, obj); err != nil { if !apierrors.IsNotFound(err) { upd.cfg.Logger.Error( err, @@ -161,8 +160,7 @@ func (upd *updaterImpl) update( statusSetter(obj) - err = upd.cfg.Client.Status().Update(ctx, obj) - if err != nil { + if err := upd.cfg.Client.Status().Update(ctx, obj); err != nil { upd.cfg.Logger.Error( err, "Failed to update status", diff --git a/internal/mode/provisioner/deployment.go b/internal/mode/provisioner/deployment.go index 376c134df..9068c694b 100644 --- a/internal/mode/provisioner/deployment.go +++ b/internal/mode/provisioner/deployment.go @@ -14,8 +14,7 @@ import ( // It will configure the Deployment to use the Gateway with the given NamespacedName. func prepareDeployment(depYAML []byte, id string, gwNsName types.NamespacedName) (*v1.Deployment, error) { dep := &v1.Deployment{} - err := yaml.Unmarshal(depYAML, dep) - if err != nil { + if err := yaml.Unmarshal(depYAML, dep); err != nil { return nil, fmt.Errorf("failed to unmarshal deployment: %w", err) } diff --git a/internal/mode/provisioner/handler.go b/internal/mode/provisioner/handler.go index e53860d76..e7e69d771 100644 --- a/internal/mode/provisioner/handler.go +++ b/internal/mode/provisioner/handler.go @@ -110,8 +110,7 @@ func (h *eventHandler) ensureDeploymentsMatchGateways(ctx context.Context) { panic(fmt.Errorf("failed to prepare deployment: %w", err)) } - err = h.k8sClient.Create(ctx, deployment) - if err != nil { + if err = h.k8sClient.Create(ctx, deployment); err != nil { panic(fmt.Errorf("failed to create deployment: %w", err)) } @@ -129,8 +128,7 @@ func (h *eventHandler) ensureDeploymentsMatchGateways(ctx context.Context) { for _, nsname := range removedGwsWithDeps { deployment := h.provisions[nsname] - err := h.k8sClient.Delete(ctx, deployment) - if err != nil { + if err := h.k8sClient.Delete(ctx, deployment); err != nil { panic(fmt.Errorf("failed to delete deployment: %w", err)) } diff --git a/internal/mode/provisioner/manager.go b/internal/mode/provisioner/manager.go index 91c074b41..c774068ca 100644 --- a/internal/mode/provisioner/manager.go +++ b/internal/mode/provisioner/manager.go @@ -113,8 +113,7 @@ func StartManager(cfg Config) error { firstBatchPreparer, ) - err = mgr.Add(eventLoop) - if err != nil { + if err := mgr.Add(eventLoop); err != nil { return fmt.Errorf("cannot register event loop: %w", err) } diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 01ca2da11..963f4b61e 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -226,8 +226,7 @@ func StartManager(cfg config.Config) error { eventHandler, firstBatchPreparer) - err = mgr.Add(eventLoop) - if err != nil { + if err = mgr.Add(eventLoop); err != nil { return fmt.Errorf("cannot register event loop: %w", err) } diff --git a/internal/mode/static/nginx/config/execute.go b/internal/mode/static/nginx/config/execute.go index 4a65a1d42..eabc47744 100644 --- a/internal/mode/static/nginx/config/execute.go +++ b/internal/mode/static/nginx/config/execute.go @@ -9,8 +9,7 @@ import ( func execute(template *template.Template, data interface{}) []byte { var buf bytes.Buffer - err := template.Execute(&buf, data) - if err != nil { + if err := template.Execute(&buf, data); err != nil { panic(err) } diff --git a/internal/mode/static/nginx/runtime/manager.go b/internal/mode/static/nginx/runtime/manager.go index 8280d3acd..957d7f491 100644 --- a/internal/mode/static/nginx/runtime/manager.go +++ b/internal/mode/static/nginx/runtime/manager.go @@ -49,8 +49,7 @@ func (m *ManagerImpl) Reload(ctx context.Context) error { // send HUP signal to the NGINX main process reload configuration // See https://nginx.org/en/docs/control.html - err = syscall.Kill(pid, syscall.SIGHUP) - if err != nil { + if err := syscall.Kill(pid, syscall.SIGHUP); err != nil { return fmt.Errorf("failed to send the HUP signal to NGINX main: %w", err) } diff --git a/internal/mode/static/state/graph/gateway_listener.go b/internal/mode/static/state/graph/gateway_listener.go index f8373d299..eb77ab290 100644 --- a/internal/mode/static/state/graph/gateway_listener.go +++ b/internal/mode/static/state/graph/gateway_listener.go @@ -206,8 +206,7 @@ func validateListenerHostname(listener v1beta1.Listener) []conditions.Condition return nil } - err := validateHostname(h) - if err != nil { + if err := validateHostname(h); err != nil { path := field.NewPath("hostname") valErr := field.Invalid(path, listener.Hostname, err.Error()) return staticConds.NewListenerUnsupportedValue(valErr.Error()) diff --git a/internal/mode/static/state/resolver/service_resolver_test.go b/internal/mode/static/state/resolver/service_resolver_test.go index fedcf1174..741e49896 100644 --- a/internal/mode/static/state/resolver/service_resolver_test.go +++ b/internal/mode/static/state/resolver/service_resolver_test.go @@ -72,8 +72,7 @@ func createSlice( func createFakeK8sClient(initObjs ...client.Object) (client.Client, error) { scheme := runtime.NewScheme() - err := discoveryV1.AddToScheme(scheme) - if err != nil { + if err := discoveryV1.AddToScheme(scheme); err != nil { return nil, err } From 8749d12d6223a649fd998eb8d53e5c77dd9cd7ac Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 31 Aug 2023 14:49:01 -0700 Subject: [PATCH 2/2] Refactor longer length inline error checking statements --- internal/framework/controller/register.go | 8 +++++++- internal/mode/provisioner/manager.go | 9 +++++++-- internal/mode/static/handler.go | 3 +-- internal/mode/static/manager.go | 9 +++++++-- internal/mode/static/state/graph/httproute.go | 12 +++++++++--- 5 files changed, 31 insertions(+), 10 deletions(-) diff --git a/internal/framework/controller/register.go b/internal/framework/controller/register.go index 0bf7ea575..a101e66c6 100644 --- a/internal/framework/controller/register.go +++ b/internal/framework/controller/register.go @@ -82,7 +82,13 @@ func Register( } for field, indexerFunc := range cfg.fieldIndices { - if err := addIndex(ctx, mgr.GetFieldIndexer(), objectType, field, indexerFunc); err != nil { + if err := addIndex( + ctx, + mgr.GetFieldIndexer(), + objectType, + field, + indexerFunc, + ); err != nil { return err } } diff --git a/internal/mode/provisioner/manager.go b/internal/mode/provisioner/manager.go index c774068ca..16f481a67 100644 --- a/internal/mode/provisioner/manager.go +++ b/internal/mode/provisioner/manager.go @@ -72,8 +72,13 @@ func StartManager(cfg Config) error { eventCh := make(chan interface{}) for _, regCfg := range controllerRegCfgs { - err := controller.Register(ctx, regCfg.objectType, mgr, eventCh, regCfg.options...) - if err != nil { + if err := controller.Register( + ctx, + regCfg.objectType, + mgr, + eventCh, + regCfg.options..., + ); err != nil { return fmt.Errorf("cannot register controller for %T: %w", regCfg.objectType, err) } } diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index 3d9e78304..17427964d 100644 --- a/internal/mode/static/handler.go +++ b/internal/mode/static/handler.go @@ -90,8 +90,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, batch events.Ev } var nginxReloadRes nginxReloadResult - err := h.updateNginx(ctx, dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver)) - if err != nil { + if err := h.updateNginx(ctx, dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver)); err != nil { h.cfg.logger.Error(err, "Failed to update NGINX configuration") nginxReloadRes.error = err } else { diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 963f4b61e..ac5a23977 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -156,8 +156,13 @@ func StartManager(cfg config.Config) error { ctx := ctlr.SetupSignalHandler() for _, regCfg := range controllerRegCfgs { - err := controller.Register(ctx, regCfg.objectType, mgr, eventCh, regCfg.options...) - if err != nil { + if err := controller.Register( + ctx, + regCfg.objectType, + mgr, + eventCh, + regCfg.options..., + ); err != nil { return fmt.Errorf("cannot register controller for %T: %w", regCfg.objectType, err) } } diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index f0b6d3802..39edf32ad 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -180,8 +180,10 @@ func buildRoute( ParentRefs: sectionNameRefs, } - err := validateHostnames(ghr.Spec.Hostnames, field.NewPath("spec").Child("hostnames")) - if err != nil { + if err := validateHostnames( + ghr.Spec.Hostnames, + field.NewPath("spec").Child("hostnames"), + ); err != nil { r.Valid = false r.Conditions = append(r.Conditions, staticConds.NewRouteUnsupportedValue(err.Error())) @@ -550,7 +552,11 @@ func validateMatch( allErrs = append(allErrs, validateQueryParamMatch(validator, q, queryParamPath)...) } - if err := validateMethodMatch(validator, match.Method, matchPath.Child("method")); err != nil { + if err := validateMethodMatch( + validator, + match.Method, + matchPath.Child("method"), + ); err != nil { allErrs = append(allErrs, err) }