-
Notifications
You must be signed in to change notification settings - Fork 263
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
improved create service error message #312
Changes from 3 commits
cc31f2e
7935c1f
aad8a29
b2383f0
79938ef
969bcbe
4460940
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package errors | ||
|
||
import ( | ||
"fmt" | ||
"strings" | ||
) | ||
|
||
func newInvalidCRD(apiGroup string) *KNError { | ||
parts := strings.Split(apiGroup, ".") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like some feedback on this approach, I am using the first item in the resource group as the "api name" - both serving and revision return |
||
name := parts[0] | ||
msg := fmt.Sprintf("no Knative %s API found on the backend. Please verify the installation.", name) | ||
|
||
return NewKNError(msg) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package errors | ||
|
||
import ( | ||
"gotest.tools/assert" | ||
"testing" | ||
) | ||
|
||
func TestNewInvalidCRD(t *testing.T) { | ||
err := newInvalidCRD("serving.knative.dev") | ||
assert.Error(t, err, "no Knative serving API found on the backend. Please verify the installation.") | ||
|
||
err = newInvalidCRD("serving") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But “serving” would be a valid API group name, no? Why not use something like “fake-serving”? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this function just creates an error struct based on the api group string that is sent as argument - those tests verifies if the api name was properly parsed. |
||
assert.Error(t, err, "no Knative serving API found on the backend. Please verify the installation.") | ||
|
||
err = newInvalidCRD("") | ||
assert.Error(t, err, "no Knative API found on the backend. Please verify the installation.") | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the last extra line? |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
package errors | ||
|
||
import ( | ||
api_errors "k8s.io/apimachinery/pkg/api/errors" | ||
"k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"strings" | ||
) | ||
|
||
func isCRDError(status api_errors.APIStatus) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added it here since we only have one for now |
||
for _, cause := range status.Status().Details.Causes { | ||
if strings.HasPrefix(cause.Message, "404") && cause.Type == v1.CauseTypeUnexpectedServerResponse { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I inspected the APIStatus error returned by kubernetes and saw that it returned a 404 error within the |
||
return true | ||
} | ||
} | ||
|
||
return false | ||
} | ||
|
||
func Build(err error) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created a error package to deal with this, it returns a valid error interface + the APIStatus object as well. It returns the original error in case of a casting error or if the error could not be identified by the error package There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. “Build” seems a misnomer for this function? Also it’s public so add a comment. |
||
apiStatus, ok := err.(api_errors.APIStatus) | ||
if !ok { | ||
return err | ||
} | ||
|
||
var knerr *KNError | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New line not needed... |
||
|
||
if isCRDError(apiStatus) { | ||
knerr = newInvalidCRD(apiStatus.Status().Details.Group) | ||
knerr.Status = apiStatus | ||
return knerr | ||
} | ||
|
||
return err | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package errors | ||
|
||
import ( | ||
"github.com/pkg/errors" | ||
"gotest.tools/assert" | ||
api_errors "k8s.io/apimachinery/pkg/api/errors" | ||
"k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/runtime/schema" | ||
"testing" | ||
) | ||
|
||
func TestBuild(t *testing.T) { | ||
//default non api error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would separate these cases into their own t.Run(“default non API error”, func(t *testing.T){
}) This allows some parallelism, clarifies the output (and error) since each test gets the title added to it’s run output. You might need to be clever for setup and tear down. See some of the other tests for examples, e.g., tests in plugin or in |
||
defaultError := errors.New("my-custom-error") | ||
err := Build(defaultError) | ||
assert.Error(t, err, "my-custom-error") | ||
|
||
gv := schema.GroupResource{ | ||
Group: "serving.knative.dev", | ||
Resource: "service", | ||
} | ||
|
||
//api error containing expected error when knative crd is not available | ||
apiError := api_errors.NewNotFound(gv, "serv") | ||
apiError.Status().Details.Causes = []v1.StatusCause{ | ||
{ | ||
Type: "UnexpectedServerResponse", | ||
Message: "404 page not found", | ||
}, | ||
} | ||
err = Build(apiError) | ||
assert.Error(t, err, "no Knative serving API found on the backend. Please verify the installation.") | ||
|
||
//api error not registered in error factory | ||
apiError = api_errors.NewAlreadyExists(gv, "serv") | ||
err = Build(apiError) | ||
assert.Error(t, err, "service.serving.knative.dev \"serv\" already exists") | ||
|
||
//default not found api error | ||
apiError = api_errors.NewNotFound(gv, "serv") | ||
err = Build(apiError) | ||
assert.Error(t, err, "service.serving.knative.dev \"serv\" not found") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
package errors | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is a new package needed here? If this ends up being the only error, I would say no, if not then definitely. Not clear we had any new errors until now. Might be something to poll or get feedback from the group on. |
||
|
||
func NewKNError(msg string) *KNError { | ||
return &KNError{ | ||
msg: msg, | ||
} | ||
} | ||
|
||
func (kne *KNError) Error() string { | ||
return kne.msg | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
package errors | ||
|
||
import ( | ||
"gotest.tools/assert" | ||
"testing" | ||
) | ||
|
||
func TestNewKNError(t *testing.T) { | ||
err := NewKNError("myerror") | ||
assert.Error(t, err, "myerror") | ||
|
||
err = NewKNError("") | ||
assert.Error(t, err, "") | ||
} | ||
|
||
func TestKNError_Error(t *testing.T) { | ||
err := NewKNError("myerror") | ||
assert.Equal(t, err.Error(), "myerror") | ||
|
||
err = NewKNError("") | ||
assert.Equal(t, err.Error(), "") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package errors | ||
|
||
import api_errors "k8s.io/apimachinery/pkg/api/errors" | ||
|
||
type KNError struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using a generic error struct for now, each error could use its own struct if neededin the future |
||
Status api_errors.APIStatus | ||
msg string | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decided to make to more generic to be used for more types if needed