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

improved create service error message #312

Merged
merged 7 commits into from
Aug 15, 2019
Merged

improved create service error message #312

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc31f2e
improved create service error message
odra Jul 26, 2019
7935c1f
error pkg
odra Aug 2, 2019
aad8a29
revisions error handling
odra Aug 2, 2019
b2383f0
adding license
odra Aug 2, 2019
79938ef
project rebuild
odra Aug 2, 2019
969bcbe
unit test improvements
odra Aug 8, 2019
4460940
build updates
odra Aug 8, 2019
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
3 changes: 3 additions & 0 deletions pkg/kn/commands/service/service_create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@rhuss rhuss Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

return errors.New("knative serving is not installed on the connected backend. Please verify your Knative installation")
}
return err
}
fmt.Fprintf(out, "Service '%s' successfully created in namespace '%s'.\n", service.Name, namespace)
Expand Down