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.

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

Add informative error #602

Merged
merged 6 commits into from
Oct 23, 2020

Conversation

curzolapierre
Copy link
Member

@curzolapierre curzolapierre commented Oct 2, 2020

  • Add a changelog entry in the section "To Be Released" of CHANGELOG.md

Now errors are printed like:

scalingo --app sample-go-martini scale M:0 Test:0
 !     An error occurred:
       * M → containers type not found
       * Test → containers type not found
       
       Your available container types are: 'web', 'other'.
       
       Example of usage:
       scalingo --app my-app scale web:2 worker:1
       scalingo --app my-app scale web:1:XL
       scalingo --app my-app scale web:+1 worker:-1
       
       Use 'scalingo scale --help' for more information

Related to https://github.com/Scalingo/api/pull/1799
Related to https://github.com/Scalingo/one-stop-shop/issues/191
Related to OPSB-49

@curzolapierre curzolapierre self-assigned this Oct 2, 2020
@curzolapierre curzolapierre requested a review from EtienneM October 2, 2020 13:54
CHANGELOG.md Outdated Show resolved Hide resolved
apps/scale.go Outdated Show resolved Hide resolved
apps/scale.go Outdated Show resolved Hide resolved
apps/scale.go Outdated Show resolved Hide resolved
@curzolapierre curzolapierre force-pushed the fix/one-stop-shop/191/explicit_error_on_app_scaling branch from fc6e04a to ce87828 Compare October 2, 2020 14:39
@curzolapierre curzolapierre requested a review from EtienneM October 2, 2020 15:56
apps/scale.go Outdated
@@ -107,6 +109,13 @@ func Scale(app string, sync bool, types []string) error {
res, err := c.AppsScale(app, scaleParams)
if err != nil {
if !utils.IsPaymentRequiredAndFreeTrialExceededError(err) {
reqestFailedError, _ := errors.ErrgoRoot(err).(*http.RequestFailedError)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
reqestFailedError, _ := errors.ErrgoRoot(err).(*http.RequestFailedError)
reqestFailedError, ok := errors.ErrgoRoot(err).(*http.RequestFailedError)

Shouldn't you test the ok value? What happens if the error is not of type http.RequestFailedError? 🤔

apps/scale.go Outdated
@@ -107,6 +109,13 @@ func Scale(app string, sync bool, types []string) error {
res, err := c.AppsScale(app, scaleParams)
if err != nil {
if !utils.IsPaymentRequiredAndFreeTrialExceededError(err) {
reqestFailedError, _ := errors.ErrgoRoot(err).(*http.RequestFailedError)

// In case of unprocessable, format and return an clear error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// In case of unprocessable, format and return an clear error
// In case of unprocessable, format and return a clear error


// In case of unprocessable, format and return an clear error
if reqestFailedError.Code == 422 {
return formatContainerTypesError(c, app, reqestFailedError)
Copy link
Member

Choose a reason for hiding this comment

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

In case of error formatting the error message, I wouldn't return the error. I think it would be best to:

  • Log the error
  • Display the error message contained in requestFailedError. This error message is already a good error message. If we fail to enrich it with more informative information, we don't want to lose the source of the error message. Don't you think?

@Soulou
Copy link
Member

Soulou commented Oct 19, 2020

Related to OPSB-49

@Soulou Soulou closed this Oct 19, 2020
@Soulou Soulou reopened this Oct 19, 2020
@johnsudaar johnsudaar requested a review from EtienneM October 19, 2020 16:09
apps/scale.go Outdated Show resolved Hide resolved
@johnsudaar johnsudaar requested a review from EtienneM October 23, 2020 14:55
Co-authored-by: Étienne M. <EtienneM@users.noreply.github.com>
@EtienneM EtienneM merged commit 0fba05e into master Oct 23, 2020
@EtienneM EtienneM deleted the fix/one-stop-shop/191/explicit_error_on_app_scaling branch October 23, 2020 14:58
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.

4 participants