improved create service error message #312

odra · 2019-07-26T22:07:24Z

Fixes #159

Proposed Changes

Checks if the error when creating a service is a "NotFound" error and returns a custom error message

Release Note

NONE

knative-prow-robot

@odra: 0 warnings.

In response to this:

Fixes #159

Proposed Changes

Checks if the error when creating a service is a "NotFound" error and returns a custom error message

Release Note
NONE

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.

knative-prow-robot · 2019-07-26T22:07:39Z

Hi @odra. Thanks for your PR.

I'm waiting for a knative 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.

navidshaikh · 2019-07-29T15:26:12Z

pkg/kn/commands/service/service_create.go

@@ -134,6 +134,9 @@ func flush(out io.Writer) {
 func createService(client v1alpha1.KnClient, service *serving_v1alpha1_api.Service, namespace string, out io.Writer) error {
 	err := client.CreateService(service)
 	if err != nil {
+		if api_errors.IsNotFound(err) {


Fixes for service create, but other commands would still report the error mentioned in issue, I think this needs to be fixed in more generic way which applies to other commands as well.

I could implement it in the KnClient level for each method if that makes sense

Good idea. Maybe rephrase to a shorter sentence like (and to put knative later in the sentence to allow the proper capitalization):

no Knative serving API found on the backend. Please verify the installation.

open for other suggestions, though. I wonder whether we should also wrap the original error ? But I think there is no extra context information in that error which goes beyond that information.

I thought about wrapping it into a custom error struct but there is not a lot of information returned by the original error struct itself besides the error message.. could still be useful in the future, idk :)

maximilien

/ok-to-test

odra · 2019-08-02T14:52:37Z

pkg/errors/errors.go

+	"strings"
+)
+
+func newInvalidCRD(apiGroup string) *KNError {


decided to make to more generic to be used for more types if needed

odra · 2019-08-02T14:52:56Z

pkg/errors/factory.go

+	"strings"
+)
+
+func isCRDError(status api_errors.APIStatus) bool {


I added it here since we only have one for now

odra · 2019-08-02T14:53:39Z

pkg/errors/factory.go

+	return false
+}
+
+func Build(err error) error {


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

odra · 2019-08-02T14:55:07Z

pkg/errors/types.go

+
+import api_errors "k8s.io/apimachinery/pkg/api/errors"
+
+type KNError struct {


using a generic error struct for now, each error could use its own struct if neededin the future

odra · 2019-08-02T14:57:05Z

pkg/errors/factory.go

+
+func isCRDError(status api_errors.APIStatus) bool {
+	for _, cause := range status.Status().Details.Causes {
+		if strings.HasPrefix(cause.Message, "404")  && cause.Type == v1.CauseTypeUnexpectedServerResponse {


I inspected the APIStatus error returned by kubernetes and saw that it returned a 404 error within the causes array, this does not happen in case it is a usual not found error.

odra · 2019-08-02T15:00:16Z

pkg/errors/errors.go

+)
+
+func newInvalidCRD(apiGroup string) *KNError {
+	parts := strings.Split(apiGroup, ".")


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 serving

odra · 2019-08-02T15:02:59Z

/retest

rhuss · 2019-08-05T08:52:22Z

/retest

maximilien · 2019-08-07T22:10:09Z

pkg/errors/errors_test.go

+
+	err = newInvalidCRD("")
+	assert.Error(t, err, "no Knative  API found on the backend. Please verify the installation.")
+


Why the last extra line?

maximilien · 2019-08-07T22:10:48Z

pkg/errors/errors_test.go

+	err := newInvalidCRD("serving.knative.dev")
+	assert.Error(t, err, "no Knative serving API found on the backend. Please verify the installation.")
+
+	err = newInvalidCRD("serving")


But “serving” would be a valid API group name, no? Why not use something like “fake-serving”?

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.

maximilien · 2019-08-07T22:12:33Z

pkg/errors/factory.go

+		return err
+	}
+
+	var knerr *KNError


New line not needed...

maximilien · 2019-08-07T22:13:34Z

pkg/errors/factory.go

+	return false
+}
+
+func Build(err error) error {


“Build” seems a misnomer for this function? Also it’s public so add a comment.

maximilien · 2019-08-07T22:16:53Z

pkg/errors/factory_test.go

+)
+
+func TestBuild(t *testing.T) {
+	//default non api error


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 core package.

maximilien · 2019-08-07T22:18:34Z

pkg/errors/knerror.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package errors


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.

maximilien

Overall, I don’t hate this. Left some comments that should be easy to address. The main concern would be in adding new package. For me, I’d be OK as long as this package does not end up being the sole one for the package. Since that would make it “lonely and sad” but seriously an abuse of packaging :)

knative-prow-robot · 2019-08-07T22:21:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, odra

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:

~~OWNERS~~ [maximilien]

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

odra · 2019-08-08T13:58:35Z

I wasn't sure if it needed a package on its own as well but I thought it makes sense if more errors get added in there - I could add it in a errors.go file with the client package if you want to.

knative-metrics-robot · 2019-08-08T21:58:45Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/errors/errors.go	Do not exist	100.0%
pkg/errors/factory.go	Do not exist	92.3%
pkg/errors/knerror.go	Do not exist	100.0%
pkg/serving/v1alpha1/client.go	86.0%	84.1%	-1.9

maximilien

I like this better. Still ambivalent about creating new errors package.

Would be good to have some other opinions on this? @rhuss or @navidshaikh or others.

maximilien · 2019-08-09T19:39:39Z

I wasn't sure if it needed a package on its own as well but I thought it makes sense if more errors get added in there - I could add it in a errors.go file with the client package if you want to.

Left a message asking the others for their opinions. Let's see if anyone feels strongly one way or the other. Thanks for your patience.

sixolet · 2019-08-14T22:19:29Z

/lgtm

Overall I like it. Gives us a place for error translation in The Future, which we can refactor as needed.

maximilien · 2019-08-14T23:28:34Z

/test pull-knative-client-integration-tests-latest-release

This is required when running the script from Prow jobs, to establish the authentication. Bonuses: * update documentation * used the term "gcr" instead of project to clearly differentiate from the GCP projects parsed from a YAML file * separated the deletion functions for testing purposes * assorted code refactoring, documentation, simplification, fixing and nitpicking

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 26, 2019

knative-prow-robot reviewed Jul 26, 2019

View reviewed changes

knative-prow-robot requested review from rhuss and sixolet July 26, 2019 22:07

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 26, 2019

knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 26, 2019

odra force-pushed the issue-159_create-svc-error branch from 4361a48 to 5fec4bf Compare July 26, 2019 22:15

improved create service error message

cc31f2e

odra force-pushed the issue-159_create-svc-error branch from 5fec4bf to cc31f2e Compare July 26, 2019 22:19

navidshaikh reviewed Jul 29, 2019

View reviewed changes

maximilien reviewed Jul 30, 2019

View reviewed changes

error pkg

7935c1f

odra force-pushed the issue-159_create-svc-error branch from cde74fb to 7935c1f Compare August 2, 2019 08:49

revisions error handling

aad8a29

odra commented Aug 2, 2019

View reviewed changes

odra added 2 commits August 2, 2019 16:11

adding license

b2383f0

project rebuild

79938ef

odra force-pushed the issue-159_create-svc-error branch from 0a18252 to 79938ef Compare August 2, 2019 15:32

maximilien reviewed Aug 7, 2019

View reviewed changes

pkg/errors/factory.go

return err

}

var knerr *KNError

Copy link

Contributor

maximilien Aug 7, 2019

Choose a reason for hiding this comment

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

New line not needed...

maximilien reviewed Aug 7, 2019

View reviewed changes

maximilien approved these changes Aug 7, 2019

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2019

odra added 2 commits August 8, 2019 22:56

unit test improvements

969bcbe

build updates

4460940

rhuss removed their request for review August 9, 2019 09:20

maximilien reviewed Aug 9, 2019

View reviewed changes

knative-prow-robot assigned sixolet Aug 14, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2019

knative-prow-robot merged commit c4e8d5a into knative:master Aug 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improved create service error message #312

improved create service error message #312

odra commented Jul 26, 2019

knative-prow-robot left a comment

knative-prow-robot commented Jul 26, 2019

navidshaikh Jul 29, 2019

odra Jul 30, 2019

rhuss Jul 31, 2019 •

edited

Loading

odra Jul 31, 2019

maximilien left a comment

odra Aug 2, 2019

odra Aug 2, 2019

odra Aug 2, 2019 •

edited

Loading

odra Aug 2, 2019

odra Aug 2, 2019

odra Aug 2, 2019 •

edited

Loading

odra commented Aug 2, 2019

rhuss commented Aug 5, 2019

maximilien Aug 7, 2019

maximilien Aug 7, 2019 •

edited

Loading

odra Aug 8, 2019

maximilien Aug 7, 2019

maximilien Aug 7, 2019

maximilien Aug 7, 2019 •

edited

Loading

maximilien Aug 7, 2019

maximilien left a comment

knative-prow-robot commented Aug 7, 2019

odra commented Aug 8, 2019

knative-metrics-robot commented Aug 8, 2019

maximilien left a comment

maximilien commented Aug 9, 2019

sixolet commented Aug 14, 2019

maximilien commented Aug 14, 2019


		import api_errors "k8s.io/apimachinery/pkg/api/errors"

		type KNError struct {


		err = newInvalidCRD("")
		assert.Error(t, err, "no Knative API found on the backend. Please verify the installation.")

improved create service error message #312

improved create service error message #312

Conversation

odra commented Jul 26, 2019

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Jul 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odra Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odra Aug 2, 2019 • edited Loading

Choose a reason for hiding this comment

odra commented Aug 2, 2019

rhuss commented Aug 5, 2019

Choose a reason for hiding this comment

maximilien Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Aug 7, 2019

odra commented Aug 8, 2019

knative-metrics-robot commented Aug 8, 2019

maximilien left a comment

Choose a reason for hiding this comment

maximilien commented Aug 9, 2019

sixolet commented Aug 14, 2019

maximilien commented Aug 14, 2019

rhuss Jul 31, 2019 •

edited

Loading

odra Aug 2, 2019 •

edited

Loading

odra Aug 2, 2019 •

edited

Loading

maximilien Aug 7, 2019 •

edited

Loading

maximilien Aug 7, 2019 •

edited

Loading