removal of github.com/pkg/errors and commitment to go 1.13 errors #1107
Conversation
|
@kensipe perfect now if you could remove the dependency on Kubernetes that'd be great. 😂 |
| @@ -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) | |||
There was a problem hiding this comment.
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
%vby default to preserve abstractions boundaries - Using
%wintentionally when exposing the underlying error type is necessary
Thoughts?
There was a problem hiding this comment.
It seems we're already having this discussion in this PR. I x-posted my comment there.
There was a problem hiding this comment.
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.
|
merging as the required approvals are there... updates / changes have been made and I will be on PTO for an extended weekend |
What this PR does / why we need it:
Removes a dependency
Because... reasons...
Fixes #