Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Commit

Permalink
Use fmt.Errorf instead of errors.Wrap where error handling make sense
Browse files Browse the repository at this point in the history
This commit partially addresses issue #59, which encourages to use new
error wrapping semantics available since Go 1.13, using %w formatting
verb instead of 3rd party 'pkg/errors' package.

This commit changes errors handling code from using errors.Wrap with
fmt.Errorf where error handling should remain as is. For other uses of
errors.Wrap, it adds a comment with TODO how code should be changed to
either avoid or delegate error handling to someone else.

This commit also changes error phrasing to the format we agreed in #59.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
  • Loading branch information
invidian committed Aug 24, 2020
1 parent a175b37 commit f3b9137
Show file tree
Hide file tree
Showing 23 changed files with 86 additions and 68 deletions.
3 changes: 2 additions & 1 deletion cli/cmd/cluster-apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,12 @@ func runClusterApply(cmd *cobra.Command, args []string) {
func verifyCluster(kubeconfig []byte, expectedNodes int) error {
cs, err := k8sutil.NewClientset(kubeconfig)
if err != nil {
return errors.Wrapf(err, "failed to set up clientset")
return fmt.Errorf("creating Kubernetes clientset: %w", err)
}

cluster, err := lokomotive.NewCluster(cs, expectedNodes)
if err != nil {
// TODO: NewCluster should be changed to not return error.
return errors.Wrapf(err, "failed to set up cluster client")
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/components/cert-manager/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/pkg/errors"

"github.com/kinvolk/lokomotive/internal/template"
"github.com/kinvolk/lokomotive/pkg/components"
Expand Down Expand Up @@ -83,12 +82,12 @@ func (c *component) RenderManifests() (map[string]string, error) {

values, err := template.Render(chartValuesTmpl, c)
if err != nil {
return nil, errors.Wrap(err, "render chart values template")
return nil, fmt.Errorf("rendering chart values template: %w", err)
}

renderedFiles, err := util.RenderChart(helmChart, name, c.Namespace, values)
if err != nil {
return nil, errors.Wrap(err, "render chart")
return nil, fmt.Errorf("rendering chart: %w", err)
}

return renderedFiles, nil
Expand Down
9 changes: 4 additions & 5 deletions pkg/components/cluster-autoscaler/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/packethost/packngo"
"github.com/pkg/errors"

"github.com/kinvolk/lokomotive/internal/template"
"github.com/kinvolk/lokomotive/pkg/components"
Expand Down Expand Up @@ -356,17 +355,17 @@ func (c *component) RenderManifests() (map[string]string, error) {
if c.Provider == "packet" {
cl, err := packngo.NewClient()
if err != nil {
return nil, errors.Wrap(err, "create packet API client")
return nil, fmt.Errorf("creating Packet API client: %w", err)
}

devices, _, err := cl.Devices.List(c.Packet.ProjectID, nil)
if err != nil {
return nil, errors.Wrapf(err, "listing devices in project %q", c.Packet.ProjectID)
return nil, fmt.Errorf("listing devices in project %q: %w", c.Packet.ProjectID, err)
}

userData, err := getWorkerUserdata(c.ClusterName, c.Packet.Facility, devices)
if err != nil {
return nil, errors.Wrapf(err, "getting worker data for cluster %q", c.ClusterName)
return nil, fmt.Errorf("getting worker data for cluster %q: %w", c.ClusterName, err)
}

c.Packet.UserData = userData
Expand All @@ -375,7 +374,7 @@ func (c *component) RenderManifests() (map[string]string, error) {

values, err := template.Render(chartValuesTmpl, c)
if err != nil {
return nil, errors.Wrap(err, "render chart values template")
return nil, fmt.Errorf("rendering chart values template: %w", err)
}

return util.RenderChart(helmChart, name, c.Namespace, values)
Expand Down
1 change: 1 addition & 0 deletions pkg/components/dex/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ func marshalToStr(obj interface{}) (string, error) {
return string(b), nil
}

// TODO: convert to helm chart.
func (c *component) RenderManifests() (map[string]string, error) {
// Add the default path to google's connector, this is the default path
// where the user given google suite json file will be available via a
Expand Down
9 changes: 4 additions & 5 deletions pkg/components/external-dns/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/pkg/errors"

"github.com/kinvolk/lokomotive/internal/template"
"github.com/kinvolk/lokomotive/pkg/components"
Expand Down Expand Up @@ -118,27 +117,27 @@ func (c *component) RenderManifests() (map[string]string, error) {
if c.AwsConfig.AccessKeyID == "" {
accessKeyID, ok := os.LookupEnv("AWS_ACCESS_KEY_ID")
if !ok || accessKeyID == "" {
return nil, errors.New("AWS Credentials not found.")
return nil, fmt.Errorf("AWS Access Key ID not found")
}
c.AwsConfig.AccessKeyID = accessKeyID
}

if c.AwsConfig.SecretAccessKey == "" {
secretAccessKey, ok := os.LookupEnv("AWS_SECRET_ACCESS_KEY")
if !ok || secretAccessKey == "" {
return nil, errors.New("AWS Credentials not found.")
return nil, fmt.Errorf("AWS Secret Access key not found")
}
c.AwsConfig.SecretAccessKey = secretAccessKey
}

values, err := template.Render(chartValuesTmpl, c)
if err != nil {
return nil, errors.Wrap(err, "render chart values template")
return nil, fmt.Errorf("rendering chart values template: %w", err)
}

renderedFiles, err := util.RenderChart(helmChart, name, c.Namespace, values)
if err != nil {
return nil, errors.Wrap(err, "render chart")
return nil, fmt.Errorf("rendering chart: %w", err)
}

return renderedFiles, nil
Expand Down
4 changes: 2 additions & 2 deletions pkg/components/flatcar-linux-update-operator/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
package flatcarlinuxupdateoperator

import (
"fmt"
"path/filepath"

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/pkg/errors"

"github.com/kinvolk/lokomotive/pkg/assets"
"github.com/kinvolk/lokomotive/pkg/components"
Expand Down Expand Up @@ -47,7 +47,7 @@ func (c *component) RenderManifests() (map[string]string, error) {
walk := assets.DumpingWalker(ret, ".yaml")
p := filepath.Join("/components", name)
if err := assets.Assets.WalkFiles(p, walk); err != nil {
return nil, errors.Wrap(err, "failed to walk assets")
return nil, fmt.Errorf("traversing assets: %w", err)
}

return ret, nil
Expand Down
1 change: 1 addition & 0 deletions pkg/components/gangway/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ func (c *component) LoadConfig(configBody *hcl.Body, evalContext *hcl.EvalContex
return gohcl.DecodeBody(*configBody, evalContext, c)
}

// TODO: Convert to helm chart.
func (c *component) RenderManifests() (map[string]string, error) {
tmpl, err := template.New("config-map").Parse(configMapTmpl)
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions pkg/components/httpbin/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ func (c *component) LoadConfig(configBody *hcl.Body, evalContext *hcl.EvalContex
return gohcl.DecodeBody(*configBody, evalContext, c)
}

// TODO: Convert to helm chart.
func (c *component) RenderManifests() (map[string]string, error) {
tmpl, err := template.New("ingress").Parse(ingressTmpl)
if err != nil {
Expand Down
5 changes: 4 additions & 1 deletion pkg/components/metallb/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
package metallb

import (
"fmt"

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/pkg/errors"
Expand Down Expand Up @@ -54,6 +56,7 @@ func (c *component) LoadConfig(configBody *hcl.Body, evalContext *hcl.EvalContex
return gohcl.DecodeBody(*configBody, evalContext, c)
}

// TODO: Convert to helm chart.
func (c *component) RenderManifests() (map[string]string, error) {
// Here are `nodeSelectors` and `tolerations` that are set by upstream. To make sure that we
// don't miss them out we set them manually here. We cannot make these changes in the template
Expand All @@ -78,7 +81,7 @@ func (c *component) RenderManifests() (map[string]string, error) {

t, err := util.RenderTolerations(c.SpeakerTolerations)
if err != nil {
return nil, errors.Wrap(err, "failed to marshal speaker tolerations")
return nil, fmt.Errorf("marshalling speaker tolerations: %w", err)
}
c.SpeakerTolerationsJSON = t

Expand Down
5 changes: 2 additions & 3 deletions pkg/components/metrics-server/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/pkg/errors"

"github.com/kinvolk/lokomotive/internal/template"
"github.com/kinvolk/lokomotive/pkg/components"
Expand Down Expand Up @@ -79,12 +78,12 @@ func (c *component) RenderManifests() (map[string]string, error) {

values, err := template.Render(chartValuesTmpl, c)
if err != nil {
return nil, errors.Wrap(err, "render chart values template")
return nil, fmt.Errorf("rendering chart values template: %w", err)
}

renderedFiles, err := util.RenderChart(helmChart, name, c.Namespace, values)
if err != nil {
return nil, errors.Wrap(err, "render chart")
return nil, fmt.Errorf("rendering chart: %w", err)
}

return renderedFiles, nil
Expand Down
3 changes: 2 additions & 1 deletion pkg/components/openebs-storage-class/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,14 @@ func (c *component) validateConfig() error {
maxDefaultStorageClass++
}
if maxDefaultStorageClass > 1 {
return errors.New("cannot have more than one default storage class")
return fmt.Errorf("cannot have more than one default storage class")
}
}

return nil
}

// TODO: Convert to helm chart.
func (c *component) RenderManifests() (map[string]string, error) {

scTmpl, err := template.New(name).Parse(storageClassTmpl)
Expand Down
5 changes: 2 additions & 3 deletions pkg/components/prometheus-operator/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/pkg/errors"

"github.com/kinvolk/lokomotive/internal/template"
"github.com/kinvolk/lokomotive/pkg/components"
Expand Down Expand Up @@ -174,12 +173,12 @@ func (c *component) RenderManifests() (map[string]string, error) {

values, err := template.Render(chartValuesTmpl, c)
if err != nil {
return nil, errors.Wrap(err, "render chart values template")
return nil, fmt.Errorf("rendering chart values template: %w", err)
}

renderedFiles, err := util.RenderChart(helmChart, name, c.Namespace, values)
if err != nil {
return nil, errors.Wrap(err, "render chart")
return nil, fmt.Errorf("rendering chart: %w", err)
}

return renderedFiles, nil
Expand Down
1 change: 1 addition & 0 deletions pkg/components/rook-ceph/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ func (c *component) LoadConfig(configBody *hcl.Body, evalContext *hcl.EvalContex
return gohcl.DecodeBody(*configBody, evalContext, c)
}

// TODO: Convert to helm chart.
func (c *component) RenderManifests() (map[string]string, error) {
// Generate YAML for Ceph cluster.
var err error
Expand Down
5 changes: 2 additions & 3 deletions pkg/components/velero/velero.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/pkg/errors"

"github.com/kinvolk/lokomotive/internal/template"
"github.com/kinvolk/lokomotive/pkg/components"
Expand Down Expand Up @@ -156,12 +155,12 @@ func (c *component) RenderManifests() (map[string]string, error) {

values, err := template.Render(chartValuesTmpl, c)
if err != nil {
return nil, errors.Wrap(err, "render chart values template")
return nil, fmt.Errorf("rendering chart values template: %w", err)
}

renderedFiles, err := util.RenderChart(helmChart, name, c.Namespace, values)
if err != nil {
return nil, errors.Wrap(err, "render chart")
return nil, fmt.Errorf("rendering chart: %w", err)
}

return renderedFiles, nil
Expand Down
3 changes: 1 addition & 2 deletions pkg/dns/dns.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"strings"

"github.com/kinvolk/lokomotive/pkg/terraform"
"github.com/pkg/errors"
)

const (
Expand Down Expand Up @@ -109,7 +108,7 @@ func readDNSEntries(ex *terraform.Executor) ([]dnsEntry, error) {
var entries []dnsEntry

if err := ex.Output("dns_entries", &entries); err != nil {
return nil, errors.Wrap(err, "failed to get DNS entries")
return nil, fmt.Errorf("getting DNS entries: %w", err)
}

return entries, nil
Expand Down
6 changes: 3 additions & 3 deletions pkg/helm/charts.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
package helm

import (
"fmt"
"io/ioutil"
"os"

"github.com/kinvolk/lokomotive/pkg/assets"
"github.com/pkg/errors"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chart/loader"
)
Expand All @@ -39,7 +39,7 @@ type LokomotiveChart struct {
func ChartFromAssets(location string) (*chart.Chart, error) {
tmpDir, err := ioutil.TempDir("", "lokoctl-chart-")
if err != nil {
return nil, errors.Wrap(err, "creating temporary dir")
return nil, fmt.Errorf("creating temporary directory: %w", err)
}

// TODO: os.RemoveAll() returns an error which we currently don't handle. Handling the error
Expand All @@ -49,7 +49,7 @@ func ChartFromAssets(location string) (*chart.Chart, error) {
// Rendered files could contain secrets - allow r/w access to owner only.
walk := assets.CopyingWalker(tmpDir, 0700)
if err := assets.Assets.WalkFiles(location, walk); err != nil {
return nil, errors.Wrap(err, "walking assets")
return nil, fmt.Errorf("traversing assets: %w", err)
}

return loader.Load(tmpDir)
Expand Down
10 changes: 5 additions & 5 deletions pkg/install/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,9 @@
package install

import (
"fmt"
"os"

"github.com/pkg/errors"

"github.com/kinvolk/lokomotive/pkg/assets"
"github.com/kinvolk/lokomotive/pkg/util"
)
Expand All @@ -29,13 +28,13 @@ import (
func PrepareTerraformRootDir(path string) error {
pathExists, err := util.PathExists(path)
if err != nil {
return errors.Wrapf(err, "failed to stat path %q: %v", path, err)
return fmt.Errorf("checking if path %q exists: %w", path, err)
}
if pathExists {
return nil
}
if err := os.MkdirAll(path, 0755); err != nil {
return errors.Wrapf(err, "failed to create terraform assets directory at: %s", path)
return fmt.Errorf("creating Terraform assets directory at %q: %w", path, err)
}
return nil
}
Expand All @@ -51,7 +50,8 @@ func PrepareTerraformRootDir(path string) error {
func PrepareLokomotiveTerraformModuleAt(path string) error {
walk := assets.CopyingWalker(path, 0755)
if err := assets.Assets.WalkFiles(assets.TerraformModulesSource, walk); err != nil {
return errors.Wrap(err, "failed to walk assets")
return fmt.Errorf("traversing assets: %w", err)
}

return nil
}
6 changes: 2 additions & 4 deletions pkg/install/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import (
"fmt"
"time"

"github.com/pkg/errors"

"github.com/kinvolk/lokomotive/pkg/lokomotive"
"github.com/kinvolk/lokomotive/pkg/util/retryutil"
)
Expand All @@ -42,7 +40,7 @@ func Verify(cl *lokomotive.Cluster) error {
// Wait for cluster to become available
err := retryutil.Retry(clusterPingRetryInterval*time.Second, clusterPingRetries, cl.Ping)
if err != nil {
return errors.Wrapf(err, "failed to ping cluster for readiness")
return fmt.Errorf("pinging cluster for readiness: %w", err)
}

var ns *lokomotive.NodeStatus
Expand All @@ -58,7 +56,7 @@ func Verify(cl *lokomotive.Cluster) error {
return ns.Ready(), nil // Retry if not ready
})
if nsErr != nil {
return errors.Wrapf(nsErr, "error determining node status within the allowed time")
return fmt.Errorf("determining node status within allowed time: %w", nsErr)
}
if err != nil {
return fmt.Errorf("not all nodes became ready within the allowed time")
Expand Down
Loading

0 comments on commit f3b9137

Please sign in to comment.