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

Fix pilot 504ing on build submission for review on testflight #12426

Merged
merged 3 commits into from May 15, 2018

Conversation

Projects
None yet
5 participants
@thibaudrobelain
Contributor

thibaudrobelain commented May 1, 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

We're running into a 504 error that seems related in some way to #11828 when trying to upload and submit a build to our nightly testers group. When the build is submitted for review, the call times out with a 504 but the build manages to get in an approved state. However, because an error is thrown, execution is stopped and pilot never tries to add groups to the build, requiring manual input from a team member to make it happen.

Additional context:
Changing the version and submitting a new build for review seems to work fine, only subsequent builds for the same version are hitting the 504.
The 504 doesn't always happen, but it's unpredictable.

Description

This change is a workaround around the iTunesConnect issue until it gets resolved. I just wanted to put this up for review so that we could discuss if that's a fix that could be made available widely within Fastlane, or to see if we should instead rely on other means such as:

  • Checking our CI logs for such 504 and then triggering lanes meant to only distribute the build to a group (instead of restarting the whole upload process)
  • Using that fork internally until the timeout gets fixed and regularly rebasing on the tip of fastlane's master.

"Clean" options on this issue seem somewhat limited so if you have other ideas I'd love to hear them.

If this change seems reasonable, I can add test coverage!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot May 1, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

googlebot commented May 1, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no label May 1, 2018

@thibaudrobelain

This comment has been minimized.

Show comment
Hide comment
@thibaudrobelain

thibaudrobelain May 1, 2018

Contributor

I signed it!

Contributor

thibaudrobelain commented May 1, 2018

I signed it!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot May 1, 2018

CLAs look good, thanks!

googlebot commented May 1, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels May 1, 2018

@joshdholtz

This fix makes sense! Just one little question 😊

raise ex unless ex.to_s.include?("504")
UI.message("Submitting the build for review timed out, trying to recover.")
updated_build = Spaceship::TestFlight::Build.find(app_id: uploaded_build.app_id, build_id: uploaded_build.id)
raise ex unless updated_build.approved?

This comment has been minimized.

@joshdholtz
@joshdholtz

This comment has been minimized.

@snatchev

snatchev May 2, 2018

Contributor

My understanding is that approved implies it had already been ready_to_test

@snatchev

snatchev May 2, 2018

Contributor

My understanding is that approved implies it had already been ready_to_test

This comment has been minimized.

@thibaudrobelain

thibaudrobelain May 2, 2018

Contributor

Hmmm good point. I have not experienced first hand the submission timing out and the build going to the ready to test state. I think until there are reports of it it might be better to omit it?

@thibaudrobelain

thibaudrobelain May 2, 2018

Contributor

Hmmm good point. I have not experienced first hand the submission timing out and the build going to the ready to test state. I think until there are reports of it it might be better to omit it?

@snatchev

This comment has been minimized.

Show comment
Hide comment
@snatchev

snatchev May 2, 2018

Contributor

Workaround the 504 seems reasonable here. Please include a test if possible, especially with the 504 response included as a fixture.

Contributor

snatchev commented May 2, 2018

Workaround the 504 seems reasonable here. Please include a test if possible, especially with the 504 response included as a fixture.

@thibaudrobelain

This comment has been minimized.

Show comment
Hide comment
@thibaudrobelain

thibaudrobelain May 3, 2018

Contributor

@snatchev, I tried adding basic tests and I don't think they necessarily required a fixture for the 504 response, let me know if you think otherwise! Also, I have close to zero experience writing tests in ruby / rspec so let me know if there are shortcuts I could have taken or if the outputs I'm expecting should be better :)
Thanks!

Contributor

thibaudrobelain commented May 3, 2018

@snatchev, I tried adding basic tests and I don't think they necessarily required a fixture for the 504 response, let me know if you think otherwise! Also, I have close to zero experience writing tests in ruby / rspec so let me know if there are shortcuts I could have taken or if the outputs I'm expecting should be better :)
Thanks!

@snatchev

This comment has been minimized.

Show comment
Hide comment
@snatchev

snatchev May 3, 2018

Contributor

The reason I was asking about the fixture is because I think it could be valuable to have a copy of what a 504 response looks like and is reproducible in test. Especially on intermittent or hard-to-reproduce errors.

I am also a little nervous pinning the entire behavior on the exception string containing the character sequence "504". Is there anything additional we could add? For example checking the exception type?

If you can capture and add a response fixture, I would be happy to help with the spec for it.

Contributor

snatchev commented May 3, 2018

The reason I was asking about the fixture is because I think it could be valuable to have a copy of what a 504 response looks like and is reproducible in test. Especially on intermittent or hard-to-reproduce errors.

I am also a little nervous pinning the entire behavior on the exception string containing the character sequence "504". Is there anything additional we could add? For example checking the exception type?

If you can capture and add a response fixture, I would be happy to help with the spec for it.

@thibaudrobelain

This comment has been minimized.

Show comment
Hide comment
@thibaudrobelain

thibaudrobelain May 7, 2018

Contributor

@snatchev Sorry for the late reply, was busy and will be for the next week as I'll be travelling. I'll get back to this PR after that.
re: the 504 response though, I'm not sure it gives much, especially as it's basically just giving us "response.status == 504".
I did briefly think about introducing a new transient error type in handle_response that would not be "InternalServerError" but something else specific for this request. I think the logic might be too disrupted by that, so I'd be onboard to just specify the error type we're looking for that contains the 504 string!

Also, the worst case scenario of detecting 504 is checking to see if the build's managed to switch states before re-raising the exception if needed, so nothing major there!

Contributor

thibaudrobelain commented May 7, 2018

@snatchev Sorry for the late reply, was busy and will be for the next week as I'll be travelling. I'll get back to this PR after that.
re: the 504 response though, I'm not sure it gives much, especially as it's basically just giving us "response.status == 504".
I did briefly think about introducing a new transient error type in handle_response that would not be "InternalServerError" but something else specific for this request. I think the logic might be too disrupted by that, so I'd be onboard to just specify the error type we're looking for that contains the 504 string!

Also, the worst case scenario of detecting 504 is checking to see if the build's managed to switch states before re-raising the exception if needed, so nothing major there!

@snatchev

@thibaudrobelain you make a good point that a false positive is not really that bad in this case. I think that satisfies my concerns. The specs look good too.
LGTM :)

@thibaudrobelain

This comment has been minimized.

Show comment
Hide comment
@thibaudrobelain

thibaudrobelain May 15, 2018

Contributor

Thanks @snatchev! What's the next step to get this PR merged?

Contributor

thibaudrobelain commented May 15, 2018

Thanks @snatchev! What's the next step to get this PR merged?

@snatchev snatchev merged commit 180ada5 into fastlane:master May 15, 2018

6 checks passed

ci/circleci: Execute tests on Ubuntu Your tests passed on CircleCI!
Details
ci/circleci: Execute tests on macOS Your tests passed on CircleCI!
Details
ci/circleci: Lint Source Code Your tests passed on CircleCI!
Details
ci/circleci: Validate Documentation Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@fastlane-bot

This comment has been minimized.

Show comment
Hide comment
@fastlane-bot

fastlane-bot May 15, 2018

Hey @thibaudrobelain 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

fastlane-bot commented May 15, 2018

Hey @thibaudrobelain 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

@snatchev

This comment has been minimized.

Show comment
Hide comment
@snatchev

snatchev May 15, 2018

Contributor

Thanks for the ping @thibaudrobelain! This PR is getting 🚢'd.

Contributor

snatchev commented May 15, 2018

Thanks for the ping @thibaudrobelain! This PR is getting 🚢'd.

@fastlane-bot

This comment has been minimized.

Show comment
Hide comment
@fastlane-bot

fastlane-bot May 22, 2018

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

fastlane-bot commented May 22, 2018

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

@fastlane fastlane locked and limited conversation to collaborators Jul 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.