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

Fix wrapping errors #2492

Merged
merged 7 commits into from
Aug 2, 2023
Merged

Fix wrapping errors #2492

merged 7 commits into from
Aug 2, 2023

Conversation

MustafaSaber
Copy link
Member

@MustafaSaber MustafaSaber commented Aug 1, 2023 •
edited
Loading

Updates #2488
For #2484

err = errors.Wrap(err, nerr.Error()) also seems wrong as if error is nil it returns nil

Running go mod tidy won't remove github.com/pkg/errors as cron predicate uses https://pkg.go.dev/github.com/sarslanhan/cronmask which uses it (fixed with sarslanhan/cronmask#3) and now github.com/uber/jaeger-client-go/config, maybe will create a PR there to remove it.

@MustafaSaber
Remove github.com/pkg/errors as it's abandoned
fb723b0 
Signed-off-by: Mustafa Abdelrahman <[email protected]>
@MustafaSaber
Run go mod tidy
6732c03 
Signed-off-by: Mustafa Abdelrahman <[email protected]>
AlexanderYastrebov
AlexanderYastrebov reviewed Aug 1, 2023
View reviewed changes
dataclients/kubernetes/definitions/ingressv1.go Outdated
@@ -101,15 +101,10 @@ func ValidateIngressesV1(ingressList IngressV1List) error {
name := i.Metadata.Name
namespace := i.Metadata.Namespace
nerr = fmt.Errorf("%s/%s: %w", name, namespace, nerr)
err = errors.Wrap(err, nerr.Error())
err = errorsJoin(err, nerr)
Copy link
Member

Choose a reason for hiding this comment

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

Lets update // ValidateIngresses is a no-op comment as its definitely not a no-op

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually no-op

Copy link
Member Author

Choose a reason for hiding this comment

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

ValidateIngressV1 always return nil.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but this ValidateIngressesV1 does not have to assume it

Copy link
Member

Choose a reason for hiding this comment

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

This also should collect errors in a slice first.

Copy link
Member

Choose a reason for hiding this comment

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

Please fix the comment to // ValidateIngressesV1 validates an IngressV1List or simply remove it

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be resolved

@AlexanderYastrebov
Copy link
Member

Running go mod tidy won't remove github.com/pkg/errors as cron predicate uses

Changed fixes #2488 to Updates #2488

@AlexanderYastrebov
Copy link
Member

Running go mod tidy won't remove github.com/pkg/errors as cron predicate uses

Also created sarslanhan/cronmask#2

@MustafaSaber
Copy link
Member Author

Created sarslanhan/cronmask#3

@MustafaSaber
Add test case for error wrapping
a3075d3 
Signed-off-by: Mustafa Abdelrahman <[email protected]>
@MustafaSaber
Use yaml for test data
8250265 
Signed-off-by: Mustafa Abdelrahman <[email protected]>
@MustafaSaber
Address comments
e7342ee 
Signed-off-by: Mustafa Abdelrahman <[email protected]>
AlexanderYastrebov
AlexanderYastrebov reviewed Aug 2, 2023
View reviewed changes
go.mod
@@ -99,6 +98,7 @@ require (
github.com/opencontainers/go-digest v1.0.0 // indirect
github.com/opencontainers/image-spec v1.1.0-rc2 // indirect
github.com/opencontainers/runc v1.1.5 // indirect
github.com/pkg/errors v0.9.1 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

$ go mod why -m github.com/pkg/errors
# github.com/pkg/errors
github.com/zalando/skipper/tracing/tracers/jaeger
github.com/uber/jaeger-client-go/config
github.com/pkg/errors

Related to #2153 and #2104

@AlexanderYastrebov
Copy link
Member

👍

@AlexanderYastrebov AlexanderYastrebov mentioned this pull request Aug 2, 2023
Closed
@MustafaSaber MustafaSaber self-assigned this Aug 2, 2023
@RomanZavodskikh
Copy link
Member

👍

@MustafaSaber MustafaSaber merged commit 4d664c9 into master Aug 2, 2023
6 checks passed
@MustafaSaber MustafaSaber deleted the fix-wrapping-errors branch August 2, 2023 13:25
This was referenced Aug 2, 2023
Merged
Merged
@MustafaSaber MustafaSaber mentioned this pull request Aug 27, 2024
Merged
MustafaSaber added a commit that referenced this pull request Aug 27, 2024
@MustafaSaber
use standard errors.Join since we already use go1.22 (#3209)
3815a13 
check https://pkg.go.dev/errors#Join
follow up on #2492
Signed-off-by: Mustafa Abdelrahman <[email protected]>
Pushpalanka pushed a commit that referenced this pull request Oct 11, 2024
@MustafaSaber @Pushpalanka
use standard errors.Join since we already use go1.22 (#3209)
58f9eb1 
check https://pkg.go.dev/errors#Join
follow up on #2492
Signed-off-by: Mustafa Abdelrahman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexanderYastrebov AlexanderYastrebov AlexanderYastrebov left review comments

Assignees

@MustafaSaber MustafaSaber

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MustafaSaber @AlexanderYastrebov @RomanZavodskikh