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

Jump to bottom

Implement Wrapper interface for errors #1349

Merged
merged 1 commit into from
Apr 2, 2020
Merged

Implement Wrapper interface for errors #1349

merged 1 commit into from
Apr 2, 2020

Conversation

Adirio
Copy link
Contributor

@Adirio Adirio commented Feb 4, 2020 •
edited
Loading

Description

Most of the errors in kubebuilder are wrapping other errors. By implementing the Wrapper interface defined here we can make gomega.MatchError be able to check wrapped errors.

This behavior has been implemented for several wrapper errors. Gomega needs to be upgraded to v1.8.0+ to benefit from this behavior. Latest version as of today is v1.9.0.

Motivation

This PR is scoped under #1218

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 4, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @Adirio. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 4, 2020
@Adirio Adirio mentioned this pull request Feb 4, 2020
Closed
@mengqiy mengqiy added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 6, 2020
@Adirio Adirio force-pushed the scaffold-enhancement/wrapper-errors branch 2 times, most recently from 03840bd to 680fabd Compare February 21, 2020 12:05
@Adirio
Copy link
Contributor Author

Adirio commented Feb 21, 2020

@DirectXMan12 this is the PR I mentioned in #1375 (comment)

@Adirio Adirio mentioned this pull request Feb 21, 2020
Merged
@DirectXMan12
Copy link
Contributor

This is the same interface for Go 1.13 errors, so it'll get us Is and As support in the main codebase as well.

Seems conceptually reasonable to me.

/ok-to-test

DirectXMan12
DirectXMan12 requested changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

skimmed. one obvious comment, otherwise generally seems good.

pkg/scaffold/internal/filesystem/errors.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2020
@Adirio Adirio force-pushed the scaffold-enhancement/wrapper-errors branch from 680fabd to 2ffb426 Compare March 2, 2020 09:10
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2020
@Adirio Adirio force-pushed the scaffold-enhancement/wrapper-errors branch from 2ffb426 to f74cc02 Compare March 2, 2020 09:37
djzager
djzager suggested changes Mar 11, 2020
View reviewed changes
Copy link

@djzager djzager left a comment

Choose a reason for hiding this comment

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

Awesome work! I do think return errors.As(err, SomeError{}) is the way to go, other than that this looks great.

pkg/scaffold/internal/filesystem/errors.go Outdated Show resolved Hide resolved
pkg/scaffold/internal/filesystem/errors.go Show resolved Hide resolved
pkg/scaffold/internal/machinery/errors.go Show resolved Hide resolved
@Adirio
Implement Wrapper interface for errors
efc58e1 
Signed-off-by: Adrian Orive <adrian.orive.oneca@gmail.com>
@Adirio Adirio force-pushed the scaffold-enhancement/wrapper-errors branch from f74cc02 to efc58e1 Compare March 23, 2020 07:35
djzager
djzager approved these changes Mar 23, 2020
View reviewed changes
Copy link

@djzager djzager left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2020
estroz
estroz approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@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

@djzager
Copy link

djzager commented Mar 24, 2020

/assign @DirectXMan12

I believe this is ready.

camilamacedo86
camilamacedo86 reviewed Apr 1, 2020
View reviewed changes
go.mod Show resolved Hide resolved
camilamacedo86
camilamacedo86 requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@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.

Just revert the go.mod changes and a typo nit.
Then, all show greats for we move forward with.

camilamacedo86
camilamacedo86 approved these changes Apr 2, 2020
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/approve

Moving forward with.
I have just a nit concern about the go.mod but we can work to clean up it in #1454
PS.; The requested made for @DirectXMan12 was addressed as well.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, camilamacedo86

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2020
@k8s-ci-robot k8s-ci-robot merged commit 29d1dc1 into kubernetes-sigs:master Apr 2, 2020
@Adirio Adirio deleted the scaffold-enhancement/wrapper-errors branch November 11, 2020 07:51
@camilamacedo86 camilamacedo86 mentioned this pull request Feb 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djzager djzager djzager approved these changes

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@estroz estroz estroz approved these changes

@droot droot Awaiting requested review from droot

@mengqiy mengqiy Awaiting requested review from mengqiy

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

Assignees

@DirectXMan12 DirectXMan12

@djzager djzager

@estroz estroz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Adirio @k8s-ci-robot @DirectXMan12 @djzager @camilamacedo86 @estroz @mengqiy