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

Jump to bottom

[Build] Add retry_count to #all_processing_builds #10656

Merged
merged 1 commit into from Oct 25, 2017
Merged

[Build] Add retry_count to #all_processing_builds #10656

merged 1 commit into from Oct 25, 2017

Conversation

UnsafePointer
Copy link
Contributor

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

Same motivations and context explained in #9331

Today fastlane raised Spaceship::Client::UnexpectedResponse with the following trace:

  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/spaceship/lib/spaceship/test_flight/client.rb:227:in `handle_response'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/spaceship/lib/spaceship/test_flight/client.rb:27:in `get_build_trains'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/spaceship/lib/spaceship/test_flight/build_trains.rb:13:in `all'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/spaceship/lib/spaceship/test_flight/build.rb:84:in `all'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/spaceship/lib/spaceship/test_flight/build.rb:95:in `all_processing_builds'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane_core/lib/fastlane_core/build_watcher.rb:26:in `watching_build'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane_core/lib/fastlane_core/build_watcher.rb:7:in `wait_for_build_processing_to_be_complete'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/pilot/lib/pilot/build_manager.rb:38:in `upload'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/actions/pilot.rb:16:in `run'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/runner.rb:256:in `block (2 levels) in execute_action'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/actions/actions_helper.rb:50:in `execute_action'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/runner.rb:234:in `block in execute_action'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/runner.rb:230:in `chdir'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/runner.rb:230:in `execute_action'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/runner.rb:148:in `trigger_action_by_name'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/fast_file.rb:146:in `method_missing'
  Fastfile:67:in `block (2 levels) in parsing_binding'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/lane.rb:33:in `call'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/lane.rb:33:in `call'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/runner.rb:49:in `block in execute'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/runner.rb:45:in `chdir'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/runner.rb:45:in `execute'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/lane_manager.rb:54:in `cruise_lane'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/command_line_handler.rb:30:in `handle'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/commands_generator.rb:104:in `block (2 levels) in run'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/commander-fastlane-4.4.5/lib/commander/command.rb:178:in `call'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/commander-fastlane-4.4.5/lib/commander/command.rb:178:in `call'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/commander-fastlane-4.4.5/lib/commander/command.rb:153:in `run'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/commander-fastlane-4.4.5/lib/commander/runner.rb:476:in `run_active_command'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane_core/lib/fastlane_core/ui/fastlane_runner.rb:66:in `run!'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/commander-fastlane-4.4.5/lib/commander/delegates.rb:15:in `run!'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/commands_generator.rb:303:in `run'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/commands_generator.rb:42:in `start'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/fastlane/lib/fastlane/cli_tools_distributor.rb:66:in `take_off'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/fastlane-04a84564ed32/bin/fastlane:20:in `<top (required)>'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bin/fastlane:23:in `load'
  /Users/bldadmin/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bin/fastlane:23:in `<top (required)>'

The retry logic implemented in #9331 helps with FastlaneCore::BuildWatcher#matching_build, but not with #watching_build.

Description

This PR adds retry_count argument to Spaceship::TestFlight::Build#all_processing_builds

@mpirri
Copy link
Contributor

mpirri commented Oct 20, 2017

@Ruenzuo Sorry about this, but we had an issue in our unit tests, which has since been fixed on master. Do you mind rebasing your PR off of master and then re-pushing? That should fix the CI error. Thanks!

@UnsafePointer
Copy link
Contributor Author

@mpirri sorry for the late reply, rebased now.

ohayon
ohayon reviewed Oct 24, 2017
View reviewed changes
Copy link
Contributor

@ohayon ohayon left a comment

Choose a reason for hiding this comment

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

Hey @Ruenzuo - thanks for this! Its definitely a nice change, I'm just wondering about the one question above, and thinking out loud, should we have an option to pass in for this? Why do we think that retrying twice is enough?

This is definitely an awesome PR, just want to be sure we are making the right choice there before merging.

Thanks!

fastlane_core/lib/fastlane_core/build_watcher.rb
@@ -23,7 +23,7 @@ def wait_for_build_processing_to_be_complete(app_id: nil, platform: nil, poll_in
private

def watching_build(app_id: nil, platform: nil)
processing_builds = Spaceship::TestFlight::Build.all_processing_builds(app_id: app_id, platform: platform)
processing_builds = Spaceship::TestFlight::Build.all_processing_builds(app_id: app_id, platform: platform, retry_count: 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason for having 2 as the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the default value FastlaneCore::BuildWatcher#matching_build is using. I kept the same default value but besides of that no other reason.

@UnsafePointer
Copy link
Contributor Author

@ohayon it would be nice to have a global fastlane configuration (i.e. environment variable) for all retry_count values.

I can only find one usage of this, although I'm not sure if my query is correct. If so, this one would be the second one.

Would a different PR for that make sense?

@ohayon
Copy link
Contributor

ohayon commented Oct 25, 2017

Hey @Ruenzuo, it would be cool to have some more config around this, but I worry that we would need to change a bit of code to make sure certain places maintained their same behavior. I think you made a good point by using the same number of retries as the matching build situation. Thanks for this PR 🚀

@ohayon ohayon merged commit 388b02f into fastlane:master Oct 25, 2017
@fastlane-bot
Copy link

Hey @Ruenzuo 👋

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 🚀

@ohayon ohayon mentioned this pull request Oct 31, 2017
Merged
@fastlane-bot
Copy link

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

RishabhTayal pushed a commit to RishabhTayal/fastlane that referenced this pull request Dec 22, 2017
@RishabhTayal
[Build] Add retry_count to #all_processing_builds (fastlane#10656)
1908e8b
@fastlane fastlane locked and limited conversation to collaborators Dec 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ohayon ohayon ohayon approved these changes

Assignees
No one assigned
Labels
cla: yes status: released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@UnsafePointer @mpirri @ohayon @fastlane-bot @googlebot