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.

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

*: use fmt.Errorf with %w/%v for Go 1.13 error wrapping #2355

Merged
merged 9 commits into from
Jan 16, 2020

Conversation

nrchakradhar
Copy link
Contributor

Use fmt.Errorf as per 1.13. Changed errors.Wrap and errors.Wrapf to use fmt.Errorf.

Fixes #2308
Closes #2308

Description of the change:
As the code is upgraded to go 1.13, error handling has been changed. The PR addresses the same.

  1. errors.Wrap(), errors.Wrapf(), are replaced with fmt.Errorf() with %w.
  2. fmt.Errorf() calls that wrap an error, but use %s or %v, are replaced with fmt.Errorf() with %w
  3. Uses of the github.com/pkg/errors package is transitioned over to Go's standard library errors package.

All fmt.Errorf() calls that wrap an error are changed as below:

fmt.Errorf("some context about my error: %w", err)

Motivation for the change:
Upgrade to go 1.13 version

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 28, 2019
@nrchakradhar
Copy link
Contributor Author

@joelanford @jmrodri @jmccormick2001
Can you please take a look. I had made multiple commits but have now squashed them and CI has passed.
Let me know if there are any changes to be done.

cmd/operator-sdk/new/cmd.go Outdated Show resolved Hide resolved
cmd/operator-sdk/add/api.go Show resolved Hide resolved
internal/scorecard/scorecard.go Outdated Show resolved Hide resolved
internal/util/operator-registry/package_manifest.go Outdated Show resolved Hide resolved
pkg/ansible/watches/watches.go Outdated Show resolved Hide resolved
pkg/helm/release/manager.go Outdated Show resolved Hide resolved
pkg/helm/release/manager.go Outdated Show resolved Hide resolved
internal/generate/gen/rules.go Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@nrchakradhar nrchakradhar requested a review from estroz December 31, 2019 06:32
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 1, 2020
@nrchakradhar
Copy link
Contributor Author

@estroz can you please check the changes and let me know if additional changes are needed. Thanks

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Great collab 🥇. Really tks for the contribution.
It shows fine, however, needs to be rebased with the master.
Could you please rebase for we are able to check if all are changed as requested?

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 8, 2020
@nrchakradhar
Copy link
Contributor Author

nrchakradhar commented Jan 8, 2020

@camilamacedo86 I have rebased and pre-submit checks have passed. PTAL.
Let me know if any additional changes are needed. Thanks for accepting the contribution.
I had missed one change and realised after commenting. I have updated the PR.

@@ -126,7 +126,7 @@ func verifyLeader(t *testing.T, namespace string, f *framework.Framework, labels
return true, nil
})
if err != nil {
return nil, fmt.Errorf("error getting leader lock configmap: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

@nrchakradhar also nothing in test/ should be wrapped. Otherwise LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes done.

CHANGELOG.md Outdated Show resolved Hide resolved
@nrchakradhar
Copy link
Contributor Author

I hope I have addressed all the changes. @estroz @camilamacedo86 PTAL. Thanks.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2020
CHANGELOG.md Outdated
@@ -22,7 +22,7 @@
- Added support for override values with environment variable expansion in the `watches.yaml` file for Helm-based operators. ([#2325](https://github.com/operator-framework/operator-sdk/pull/2325))

### Changed

- Changed error wrapping according to Go version 1.13+ [error handling](https://blog.golang.org/go1.13-errors). ([#2355](https://github.com/operator-framework/operator-sdk/pull/2355))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @nrchakradhar,

We had a new release so it needs to be updated with the master and the CHANGELOG properly updated. Could you please do that for we are able to merge?

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Has /lgtm
Needs just rebase with master and updated properly the CHANGELOG.

@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2020
@nrchakradhar
Copy link
Contributor Author

@estroz @camilamacedo86 Travis error shows that there is a mismatch in go module versions.
I am not sure why the build is failing and how to fix it. Can you please help?

@estroz
Copy link
Member

estroz commented Jan 16, 2020

@nrchakradhar merge your branch with master, that should resolve the issue.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020
@nrchakradhar
Copy link
Contributor Author

nrchakradhar commented Jan 16, 2020

The new failure seems to be different than the changes.
https://travis-ci.org/operator-framework/operator-sdk/jobs/637862112?utm_medium=notification

Found broken url (#1571 -> #1571) at ./CHANGELOG.md:221 : 502 Bad Gateway
Found broken url (#2298 -> #2298) at ./CHANGELOG.md:81 : 502 Bad Gateway

May be there is some network issue during the build.

How do I re-trigger the build? @estroz @camilamacedo86

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/gtm
/approved

@estroz estroz requested a review from camilamacedo86 January 16, 2020 21:50
@estroz estroz changed the title Go 1.13 error wrapping: Use fmt.Errorf as per 1.13. *: use fmt.Errorf with %w/%v for Go 1.13 error wrapping Jan 16, 2020
@estroz estroz merged commit 3dec6d2 into operator-framework:master Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Go 1.13 error wrapping
4 participants