Skip to content

Improve the API ping in configure command#679

Merged
kytrinyx merged 1 commit intomasterfrom
api-error-configure-cmd
Jul 25, 2018
Merged

Improve the API ping in configure command#679
kytrinyx merged 1 commit intomasterfrom
api-error-configure-cmd

Conversation

@kytrinyx
Copy link
Member

This distinguishes between an error in the HTTP request (e.g.
no service at URL), and an error returned from the API (e.g. 500 error).

Previous output in the case of an HTTP 500

$ ./testercism configure --api=http://localhost:3000/api/v1
Error: The base API URL 'http://localhost:3000/api/v1' cannot be reached.



Now:

$ ./testercism configure --api=http://localhost:3000/api/v1
Error: The base API URL 'http://localhost:3000/api/v1' cannot be reached.

API returned 500 Internal Server Error

@kytrinyx kytrinyx requested a review from nywilken July 25, 2018 14:58
cmd/configure.go Outdated
@@ -93,7 +93,11 @@ func runConfigure(configuration config.Configuration, flags *pflag.FlagSet) erro

ok, err := client.IsPingable()
if !ok || err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The IsPingable method seems to imply if there is an error than the client is not technically not reachable. If that is always going to be the case should we just check if err != nil and drop the !ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, now that the IsPingable() returns an error, we can simplify.

This distinguishes between an error in the HTTP request (e.g.
no service at URL), and an error returned from the API (e.g. 500 error).
@kytrinyx kytrinyx force-pushed the api-error-configure-cmd branch from 1a9aa41 to 239f303 Compare July 25, 2018 21:39
@kytrinyx
Copy link
Member Author

I've tweaked based on your feedback, @nywilken

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Looks good!

@kytrinyx kytrinyx merged commit 13e6181 into master Jul 25, 2018
@kytrinyx kytrinyx deleted the api-error-configure-cmd branch July 25, 2018 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants