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

[spaceship] also catch and handle "Gateway Timeout" #13255

Merged
merged 2 commits into from
Sep 3, 2018

Conversation

tmm1
Copy link
Contributor

@tmm1 tmm1 commented Aug 30, 2018

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

fixes #13241

@janpio janpio changed the title [spaceship] catch more internal server errors [spaceship] also catch and handle "Gateway Timeout" Aug 31, 2018
@janpio janpio requested a review from joshdholtz August 31, 2018 08:13
@tmm1 tmm1 mentioned this pull request Sep 1, 2018
4 tasks
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

Can we have this actually raise a new error called GatewayTimeoutError? 😊 A gateway error and internal service error will get handled the same way but it feels a bit dangerous to have the gateway error map to the InternalServerError and have the same error message as the InternalServerError.

I think it would be ideal if we could get this mapped to a new error 💪

@tmm1
Copy link
Contributor Author

tmm1 commented Sep 3, 2018

I don't understand what's dangerous about it? They're both the same "in read" error.

@joshdholtz
Copy link
Member

@tmm1 A gateway timeout error (504) is inherently different from an internal server error (500) where an 500 happens at an application level and a 504 happens at some network level.

I would suggest adding a new conditional with something like...

raise GatewayTimeoutInReadError, "Received an \"Gateway Timeout - In Read\" from App Store Connect / Developer Portal, please try again later"

as this error message will get printed out to the user. We do not want to map a "Gateway Timeout" as an "Internal Server Error".

To then account for this in the retry, we would add GatewayTimeoutInRead error to this rescue so that it gets retried - https://github.com/fastlane/fastlane/blob/master/spaceship/lib/spaceship/client.rb#L530-L535

This should have the same end result as your initial fix but this will present the proper error message to the end user if the retries are not successful

@tmm1
Copy link
Contributor Author

tmm1 commented Sep 3, 2018

Okay I can make those changes this week. Feel free to commit on this PR directly if you would like.

@joshdholtz
Copy link
Member

@tmm1 Can you give me permission to push to this branch? There should be some permission option on the right side of the page I believe 🤔 Thanks!

@tmm1
Copy link
Contributor Author

tmm1 commented Sep 3, 2018

It's enabled already. Are you not able to push to my branch?

@tmm1
Copy link
Contributor Author

tmm1 commented Sep 3, 2018

I guess maybe it only works for "Maintainers"?

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Sep 3, 2018
@joshdholtz
Copy link
Member

@tmm1 Whoops! Was pushing to apple_errors instead of apple-errors 😝

@joshdholtz
Copy link
Member

@tmm1 @janpio Pushed some changes here! Added new GatewayTimeoutError and also added a bunch of tests around these parts that should have existed before but didn't 😇

@tmm1
Copy link
Contributor Author

tmm1 commented Sep 3, 2018

👍🏼 LGTM

@joshdholtz joshdholtz merged commit 9d370e7 into fastlane:master Sep 3, 2018
@fastlane-bot
Copy link

Congratulations! 🎉 This was released as part of fastlane 2.103.0 🚀

janpio pushed a commit to janpio/fastlane that referenced this pull request Sep 11, 2018
* [spaceship] catch more internal server errors

* Added new GatewayTimeoutError and tests for catching common errors
@fastlane fastlane locked and limited conversation to collaborators Nov 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

download_dsyms does not retry errors
5 participants