Skip to content
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.

Sign up for GitHub

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

removal of github.com/pkg/errors and commitment to go 1.13 errors #1107

Merged
merged 3 commits into from
Nov 27, 2019
Merged

removal of github.com/pkg/errors and commitment to go 1.13 errors #1107

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f64e15
removal of github.com/pkg/errors and commitment to go 1.13 errors
kensipe Nov 26, 2019
4e74660
remove of gh errors from go mods
kensipe Nov 26, 2019
1bb09c0
change no arg error messages from fmt.errorf to errors.new
kensipe Nov 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions pkg/engine/renderer/enhancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import (
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

"github.com/pkg/errors"

"gopkg.in/yaml.v2"
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/kustomize/k8sdeps/kunstruct"
Expand Down Expand Up @@ -48,7 +46,7 @@ func (k *KustomizeEnhancer) Apply(templates map[string]string, metadata Metadata
templateNames = append(templateNames, k)
err := fsys.WriteFile(fmt.Sprintf("%s/%s", basePath, k), []byte(v))
if err != nil {
return nil, errors.Wrapf(err, "error when writing templates to filesystem before applying kustomize")
return nil, fmt.Errorf("error when writing templates to filesystem before applying kustomize: %w", err)
}
}

Expand All @@ -75,12 +73,12 @@ func (k *KustomizeEnhancer) Apply(templates map[string]string, metadata Metadata

yamlBytes, err := yaml.Marshal(kustomization)
if err != nil {
return nil, errors.Wrapf(err, "error marshalling kustomize yaml")
return nil, fmt.Errorf("error marshalling kustomize yaml: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an agreement on when we expose the underlying error with %w and when not?

To quote Go 1.13 errors blog post:

In other words, wrapping an error makes that error part of your API. If you don't want to 
commit to supporting that error as part of your API in the future, you shouldn't wrap the error.

It’s important to remember that whether you wrap or not, the error text will be the same. A 
person trying to understand the error will have the same information either way; the choice to 
wrap is about whether to give programs additional information so they can make more informed 
decisions, or to withhold that information to preserve an abstraction layer.

I believe, in this particular case, it doesn't make a difference since our yaml.Marshal method already wraps underlying errors. Personally, I'm leaning towards:

  • Using %v by default to preserve abstractions boundaries
  • Using %w intentionally when exposing the underlying error type is necessary

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we're already having this discussion in this PR. I x-posted my comment there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great thoughts. The wrapping is because we wrapped it before... I'm not aware if this was incidental or intentional. It is worth pausing.. I definitely like your personally leanings and will take them on as well.

}

err = fsys.WriteFile(fmt.Sprintf("%s/kustomization.yaml", basePath), yamlBytes)
if err != nil {
return nil, errors.Wrapf(err, "error writing kustomization.yaml file")
return nil, fmt.Errorf("error writing kustomization.yaml file: %w", err)
}

ldr, err := loader.NewLoader(basePath, fsys)
Expand All @@ -96,28 +94,28 @@ func (k *KustomizeEnhancer) Apply(templates map[string]string, metadata Metadata
rf := resmap.NewFactory(resource.NewFactory(kunstruct.NewKunstructuredFactoryImpl()))
kt, err := target.NewKustTarget(ldr, rf, transformer.NewFactoryImpl())
if err != nil {
return nil, errors.Wrapf(err, "error creating kustomize target")
return nil, fmt.Errorf("error creating kustomize target: %w", err)
}

allResources, err := kt.MakeCustomizedResMap()
if err != nil {
return nil, errors.Wrapf(err, "error creating customized resource map for kustomize")
return nil, fmt.Errorf("error creating customized resource map for kustomize: %w", err)
}

res, err := allResources.EncodeAsYaml()
if err != nil {
return nil, errors.Wrapf(err, "error encoding kustomized files into yaml")
return nil, fmt.Errorf("error encoding kustomized files into yaml: %w", err)
}

objsToAdd, err = YamlToObject(string(res))
if err != nil {
return nil, errors.Wrapf(err, "error parsing kubernetes objects after applying kustomize")
return nil, fmt.Errorf("error parsing kubernetes objects after applying kustomize: %w", err)
}

for _, o := range objsToAdd {
err = setControllerReference(metadata.ResourcesOwner, o, k.Scheme)
if err != nil {
return nil, errors.Wrapf(err, "setting controller reference on parsed object")
return nil, fmt.Errorf("setting controller reference on parsed object: %w", err)
}
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/engine/workflow/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package workflow

import (
"fmt"
"reflect"
"testing"
"time"
Expand All @@ -9,7 +10,6 @@ import (

"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
"github.com/kudobuilder/kudo/pkg/engine"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -641,7 +641,7 @@ func (k *testEnhancer) Apply(templates map[string]string, metadata renderer.Meta
for _, t := range templates {
objsToAdd, err := renderer.YamlToObject(t)
if err != nil {
return nil, errors.Wrapf(err, "error parsing kubernetes objects after applying kustomize")
return nil, fmt.Errorf("error parsing kubernetes objects after applying kustomize: %w", err)
}
result = append(result, objsToAdd[0])
}
Expand Down
5 changes: 2 additions & 3 deletions pkg/kudoctl/cmd/get/get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"github.com/kudobuilder/kudo/pkg/kudoctl/env"
"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"

"github.com/pkg/errors"
"github.com/xlab/treeprint"
)

Expand All @@ -21,7 +20,7 @@ func Run(args []string, settings *env.Settings) error {

kc, err := env.GetClient(settings)
if err != nil {
return errors.Wrap(err, "creating kudo client")
return fmt.Errorf("creating kudo client: %w", err)
}

p, err := getInstances(kc, settings)
Expand Down Expand Up @@ -55,7 +54,7 @@ func getInstances(kc *kudo.Client, settings *env.Settings) ([]string, error) {

instanceList, err := kc.ListInstances(settings.Namespace)
if err != nil {
return nil, errors.Wrap(err, "getting instances")
return nil, fmt.Errorf("getting instances: %w", err)
}

return instanceList, nil
Expand Down
5 changes: 3 additions & 2 deletions pkg/kudoctl/cmd/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package cmd

import (
"fmt"

"github.com/kudobuilder/kudo/pkg/kudoctl/cmd/install"

"github.com/pkg/errors"
"github.com/spf13/afero"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -43,7 +44,7 @@ func newInstallCmd(fs afero.Fs) *cobra.Command {
var err error
options.Parameters, err = install.GetParameterMap(parameters)
if err != nil {
return errors.WithMessage(err, "could not parse arguments")
return fmt.Errorf("could not parse arguments: %w", err)
}

return install.Run(args, options, fs, &Settings)
Expand Down
9 changes: 5 additions & 4 deletions pkg/kudoctl/cmd/install/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
package install

import (
"fmt"

"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
"github.com/kudobuilder/kudo/pkg/kudoctl/env"
pkgresolver "github.com/kudobuilder/kudo/pkg/kudoctl/packages/resolver"
"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo"

"github.com/pkg/errors"
"github.com/spf13/afero"
)

Expand Down Expand Up @@ -54,23 +55,23 @@ func installOperator(operatorArgument string, options *Options, fs afero.Fs, set

repository, err := repo.ClientFromSettings(fs, settings.Home, options.RepoName)
if err != nil {
return errors.WithMessage(err, "could not build operator repository")
return fmt.Errorf("could not build operator repository: %w", err)
}
clog.V(4).Printf("repository used %s", repository)

kc, err := env.GetClient(settings)
clog.V(3).Printf("acquiring kudo client")
if err != nil {
clog.V(3).Printf("failed to acquire client")
return errors.Wrap(err, "creating kudo client")
return fmt.Errorf("creating kudo client: %w", err)
}

clog.V(3).Printf("getting package crds")

resolver := pkgresolver.New(repository)
pkg, err := resolver.Resolve(operatorArgument, options.PackageVersion)
if err != nil {
return errors.Wrapf(err, "failed to resolve package CRDs for operator: %s", operatorArgument)
return fmt.Errorf("failed to resolve package CRDs for operator: %s %w", operatorArgument, err)
}

return kudo.InstallPackage(kc, pkg.Resources, options.SkipInstance, options.InstanceName, settings.Namespace, options.Parameters)
Expand Down
5 changes: 2 additions & 3 deletions pkg/kudoctl/cmd/package_params_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo"

"github.com/gosuri/uitable"
"github.com/pkg/errors"
"github.com/spf13/afero"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -72,15 +71,15 @@ func (c *paramsListCmd) run(fs afero.Fs, settings *env.Settings) error {
}
repository, err := repo.ClientFromSettings(fs, settings.Home, c.RepoName)
if err != nil {
return errors.WithMessage(err, "could not build operator repository")
return fmt.Errorf("could not build operator repository: %w", err)
}
clog.V(4).Printf("repository used %s", repository)

clog.V(3).Printf("getting package pkg files for %v with version: %v", c.path, c.PackageVersion)
resolver := pkgresolver.New(repository)
pf, err := resolver.Resolve(c.path, c.PackageVersion)
if err != nil {
return errors.Wrapf(err, "failed to resolve package files for operator: %s", c.path)
return fmt.Errorf("failed to resolve package files for operator: %s: %w", c.path, err)
}

return displayParamsTable(pf.Files, c)
Expand Down
15 changes: 7 additions & 8 deletions pkg/kudoctl/cmd/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"github.com/kudobuilder/kudo/pkg/kudoctl/env"
"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"

"github.com/pkg/errors"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -43,7 +42,7 @@ func newUpdateCmd() *cobra.Command {
var err error
options.Parameters, err = install.GetParameterMap(parameters)
if err != nil {
return errors.WithMessage(err, "could not parse arguments")
return fmt.Errorf("could not parse arguments: %w", err)
}
return runUpdate(args, options, &Settings)
},
Expand All @@ -57,13 +56,13 @@ func newUpdateCmd() *cobra.Command {

func validateUpdateCmd(args []string, options *updateOptions) error {
if len(args) != 0 {
return errors.New("expecting no arguments provided for update. Only named flags are accepted")
return fmt.Errorf("expecting no arguments provided for update. Only named flags are accepted")
kensipe marked this conversation as resolved.
Show resolved Hide resolved
}
if options.InstanceName == "" {
return errors.New("--instance flag has to be provided to indicate which instance you want to update")
return fmt.Errorf("--instance flag has to be provided to indicate which instance you want to update")
}
if len(options.Parameters) == 0 {
return errors.New("need to specify at least one parameter to override via -p otherwise there is nothing to update")
return fmt.Errorf("need to specify at least one parameter to override via -p otherwise there is nothing to update")
}

return nil
Expand All @@ -78,7 +77,7 @@ func runUpdate(args []string, options *updateOptions, settings *env.Settings) er

kc, err := env.GetClient(settings)
if err != nil {
return errors.Wrap(err, "creating kudo client")
return fmt.Errorf("creating kudo client: %w", err)
}

return update(instanceToUpdate, kc, options, settings)
Expand All @@ -88,7 +87,7 @@ func update(instanceToUpdate string, kc *kudo.Client, options *updateOptions, se
// Make sure the instance you want to upgrade exists
instance, err := kc.GetInstance(instanceToUpdate, settings.Namespace)
if err != nil {
return errors.Wrapf(err, "verifying the instance does not already exist")
return fmt.Errorf("verifying the instance does not already exist: %w", err)
}
if instance == nil {
return fmt.Errorf("instance %s in namespace %s does not exist in the cluster", instanceToUpdate, settings.Namespace)
Expand All @@ -97,7 +96,7 @@ func update(instanceToUpdate string, kc *kudo.Client, options *updateOptions, se
// Update arguments
err = kc.UpdateInstance(instanceToUpdate, settings.Namespace, nil, options.Parameters)
if err != nil {
return errors.Wrapf(err, "updating instance %s", instanceToUpdate)
return fmt.Errorf("updating instance %s %w", instanceToUpdate, err)
}
fmt.Printf("Instance %s was updated.", instanceToUpdate)
return nil
Expand Down
9 changes: 4 additions & 5 deletions pkg/kudoctl/cmd/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo"
"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo"

"github.com/pkg/errors"
"github.com/spf13/afero"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -52,7 +51,7 @@ func newUpgradeCmd(fs afero.Fs) *cobra.Command {
var err error
options.Parameters, err = install.GetParameterMap(parameters)
if err != nil {
return errors.WithMessage(err, "could not parse arguments")
return fmt.Errorf("could not parse arguments: %w", err)
}
return runUpgrade(args, options, fs, &Settings)
},
Expand Down Expand Up @@ -86,18 +85,18 @@ func runUpgrade(args []string, options *options, fs afero.Fs, settings *env.Sett

kc, err := env.GetClient(settings)
if err != nil {
return errors.Wrap(err, "creating kudo client")
return fmt.Errorf("creating kudo client: %w", err)
}

// Resolve the package to upgrade to
repository, err := repo.ClientFromSettings(fs, settings.Home, options.RepoName)
if err != nil {
return errors.WithMessage(err, "could not build operator repository")
return fmt.Errorf("could not build operator repository: %w", err)
}
resolver := pkgresolver.New(repository)
pkg, err := resolver.Resolve(packageToUpgrade, options.PackageVersion)
if err != nil {
return errors.Wrapf(err, "failed to resolve package CRDs for operator: %s", packageToUpgrade)
return fmt.Errorf("failed to resolve package CRDs for operator: %s: %w", packageToUpgrade, err)
}

resources := pkg.Resources
Expand Down
7 changes: 3 additions & 4 deletions pkg/kudoctl/packages/package.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,24 @@ import (
"github.com/kudobuilder/kudo/pkg/engine/task"
"github.com/kudobuilder/kudo/pkg/util/kudo"

"github.com/pkg/errors"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func (p *Files) Resources() (*Resources, error) {
if p.Operator == nil {
return nil, errors.New("operator.yaml file is missing")
return nil, fmt.Errorf("operator.yaml file is missing")
}
if p.Params == nil {
return nil, errors.New("params.yaml file is missing")
return nil, fmt.Errorf("params.yaml file is missing")
}
var errs []string
for _, tt := range p.Operator.Tasks {
errs = append(errs, validateTask(tt, p.Templates)...)
}

if len(errs) != 0 {
return nil, errors.New(strings.Join(errs, "\n"))
return nil, fmt.Errorf(strings.Join(errs, "\n"))
}

operator := &v1beta1.Operator{
Expand Down
5 changes: 2 additions & 3 deletions pkg/kudoctl/packages/reader/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"strings"

"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
"github.com/pkg/errors"
"sigs.k8s.io/yaml"
)

Expand Down Expand Up @@ -43,7 +42,7 @@ func parsePackageFile(filePath string, fileBytes []byte, currentPackage *package
switch {
case isOperatorFile(filePath):
if err := yaml.Unmarshal(fileBytes, &currentPackage.Operator); err != nil {
return errors.Wrap(err, "failed to unmarshal operator file")
return fmt.Errorf("failed to unmarshal operator file: %w", err)
}
if currentPackage.Operator.APIVersion == "" {
currentPackage.Operator.APIVersion = APIVersion
Expand All @@ -58,7 +57,7 @@ func parsePackageFile(filePath string, fileBytes []byte, currentPackage *package
case isParametersFile(filePath):
paramsFile, err := readParametersFile(fileBytes)
if err != nil {
return errors.Wrapf(err, "failed to unmarshal parameters file: %s", filePath)
return fmt.Errorf("failed to unmarshal parameters file: %s: %w", filePath, err)
}
defaultRequired := true
for i := 0; i < len(paramsFile.Parameters); i++ {
Expand Down
Loading