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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fb723b0
Remove `github.com/pkg/errors` as it's abandoned
MustafaSaber Aug 1, 2023
6732c03
Run `go mod tidy`
MustafaSaber Aug 1, 2023
a3075d3
Add test case for error wrapping
MustafaSaber Aug 1, 2023
8250265
Use yaml for test data
MustafaSaber Aug 1, 2023
e7342ee
Address comments
MustafaSaber Aug 1, 2023
0d46c77
Update github.com/sarslanhan/cronmask
AlexanderYastrebov Aug 2, 2023
8d9e6bb
Revert to JSON testdata and remove yaml unmarshalling cause it's buggy
MustafaSaber Aug 2, 2023
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
15 changes: 15 additions & 0 deletions dataclients/kubernetes/definitions/definitions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package definitions

import (
"strings"
"time"

"errors"
Expand Down Expand Up @@ -43,3 +44,17 @@ type WeightedBackend interface {
GetName() string
GetWeight() float64
}

// TODO: use https://pkg.go.dev/errors#Join with go1.21
func errorsJoin(errs ...error) error {
MustafaSaber marked this conversation as resolved.
Show resolved Hide resolved
var errVals []string
for _, err := range errs {
if err != nil {
errVals = append(errVals, err.Error())
}
}
if len(errVals) > 0 {
return errors.New(strings.Join(errVals, "\n"))
}
return nil
}
16 changes: 16 additions & 0 deletions dataclients/kubernetes/definitions/definitions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,27 @@
package definitions_test

import (
"os"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/zalando/skipper/dataclients/kubernetes/definitions"
"github.com/zalando/skipper/dataclients/kubernetes/kubernetestest"
)

func TestRouteGroupValidation(t *testing.T) {
kubernetestest.FixturesToTest(t, "testdata/validation")
}

func TestValidateRouteGroups(t *testing.T) {
data, err := os.ReadFile("testdata/errorwrapdata/routegroups.yaml")
require.NoError(t, err)

rgl, err := definitions.ParseRouteGroupsYAML(data)
require.NoError(t, err)

err = definitions.ValidateRouteGroups(&rgl)

assert.EqualError(t, err, "route group without name\nerror in route group default/rg1: route group without backend")
}
11 changes: 3 additions & 8 deletions dataclients/kubernetes/definitions/ingressv1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ package definitions

import (
"encoding/json"
"errors"
"fmt"
"strconv"

"github.com/pkg/errors"
"gopkg.in/yaml.v2"
)

Expand Down Expand Up @@ -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

}
}

if err != nil {
return err
}

return nil
return err
}

func GetHostsFromIngressRulesV1(ing *IngressV1Item) []string {
Expand Down
17 changes: 5 additions & 12 deletions dataclients/kubernetes/definitions/routegroups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import (
"encoding/json"
"fmt"

"github.com/pkg/errors"
"errors"

"github.com/zalando/skipper/eskip"
"github.com/zalando/skipper/loadbalancer"
"gopkg.in/yaml.v2"
Expand Down Expand Up @@ -460,18 +461,10 @@ func ValidateRouteGroup(rg *RouteGroupItem) error {

// ValidateRouteGroups validates a RouteGroupList
func ValidateRouteGroups(rl *RouteGroupList) error {
var err error
var errs []error
// avoid the user having to repeatedly validate to discover all errors
for _, i := range rl.Items {
nerr := ValidateRouteGroup(i)
if nerr != nil {
err = errors.Wrap(err, nerr.Error())
}
}

if err != nil {
return err
errs = append(errs, ValidateRouteGroup(i))
}

return nil
return errorsJoin(errs...)
}
21 changes: 21 additions & 0 deletions dataclients/kubernetes/definitions/testdata/errorwrapdata/routegroups.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
items:
- apiVersion: zalando.org/v1
kind: RouteGroup
spec:
hosts:
- test.example.com
routes:
- filters:
- inlineContent("/foo")
- path: /foo
- apiVersion: zalando.org/v1
kind: RouteGroup
metadata:
name: rg1
spec:
hosts:
- test.example.com
routes:
- filters:
- inlineContent("/foo")
- path: /foo
MustafaSaber marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ require (
github.com/oklog/ulid v1.3.1
github.com/opentracing/basictracer-go v1.1.0
github.com/opentracing/opentracing-go v1.2.0
github.com/pkg/errors v0.9.1
github.com/prometheus/client_golang v1.16.0
github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475
github.com/redis/go-redis/v9 v9.0.5
Expand Down Expand Up @@ -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

github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/pquerna/cachecontrol v0.0.0-20171018203845-0dec1b30a021 // indirect
github.com/prometheus/client_model v0.3.0 // indirect
Expand Down
4 changes: 0 additions & 4 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,6 @@ github.com/imdario/mergo v0.3.12 h1:b6R2BslTbIEToALKP7LxUvijTsNI9TAe80pLWN2g/HU=
github.com/imdario/mergo v0.3.12/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA=
github.com/influxdata/tdigest v0.0.1 h1:XpFptwYmnEKUqmkcDjrzffswZ3nvNeevbUSLPP/ZzIY=
github.com/influxdata/tdigest v0.0.1/go.mod h1:Z0kXnxzbTC2qrx4NaIzYkE1k66+6oEDQTvL95hQFh5Y=
github.com/instana/go-sensor v1.55.0 h1:9Dpo0S9hah1irJAkkpGMfiAoKMbLLOtYBbtdhJbGvUM=
github.com/instana/go-sensor v1.55.0/go.mod h1:19yQd89yv2d0O2+onnGL5WvtMS5c/HVzl14ko8eZgqo=
github.com/instana/go-sensor v1.55.2 h1:XdLN38mSFsHpaL+jIDkE/ZrW7pxgPeUC/bV9bSwVHyM=
github.com/instana/go-sensor v1.55.2/go.mod h1:Ks06EG9Da5O3hbdJiHIePG/vNmToovkaJjMlUBd70Yc=
github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU=
Expand Down Expand Up @@ -287,8 +285,6 @@ github.com/szuecs/rate-limit-buffer v0.9.0 h1:65fBCVsaJFh0E1G5C6/sInEPlYR6dXtF9J
github.com/szuecs/rate-limit-buffer v0.9.0/go.mod h1:BxqrsmnHsCnWcvbtdcaDLEBmjNEvRFU5LQ8edoZ9B0M=
github.com/testcontainers/testcontainers-go v0.20.1 h1:mK15UPJ8c5P+NsQKmkqzs/jMdJt6JMs5vlw2y4j92c0=
github.com/testcontainers/testcontainers-go v0.20.1/go.mod h1:zb+NOlCQBkZ7RQp4QI+YMIHyO2CQ/qsXzNF5eLJ24SY=
github.com/tidwall/gjson v1.14.4 h1:uo0p8EbA09J7RQaflQ1aBRffTR7xedD2bcIVSYxLnkM=
github.com/tidwall/gjson v1.14.4/go.mod h1:/wbyibRr2FHMks5tjHJ5F8dMZh3AcwJEMf5vlfC0lxk=
github.com/tidwall/gjson v1.15.0 h1:5n/pM+v3r5ujuNl4YLZLsQ+UE5jlkLVm7jMzT5Mpolw=
github.com/tidwall/gjson v1.15.0/go.mod h1:/wbyibRr2FHMks5tjHJ5F8dMZh3AcwJEMf5vlfC0lxk=
github.com/tidwall/match v1.1.1 h1:+Ho715JplO36QYgwN9PGYNhgZvoUSc9X2c80KVTi+GA=
Expand Down