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
[pilot] Fix ASC API error when reject_build_waiting_for_review: true
#21995
Conversation
…view Fix #18408 Applying the fix from #18408 (comment) Kudos to @nid90 for the fix
@@ -368,12 +368,17 @@ def should_update_localized_build_information?(options) | |||
end | |||
|
|||
def reject_build_waiting_for_review(build) | |||
waiting_for_review_build = build.app.get_builds(filter: { "betaAppReviewSubmission.betaReviewState" => "WAITING_FOR_REVIEW" }, includes: "betaAppReviewSubmission,preReleaseVersion").first | |||
waiting_for_review_build = build.app.get_builds( | |||
filter: { "betaAppReviewSubmission.betaReviewState" => "WAITING_FOR_REVIEW,IN_REVIEW", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Not sure if the scope is the same or if the visibility between spaceship
and pilot
would allow it, but if it is, would you consider DRYing these "magic strings" in favor of
fastlane/spaceship/lib/spaceship/connect_api/models/app_info.rb
Lines 44 to 55 in df12128
module State | |
ACCEPTED = "ACCEPTED" | |
DEVELOPER_REJECTED = "DEVELOPER_REJECTED" | |
IN_REVIEW = "IN_REVIEW" | |
PENDING_RELEASE = "PENDING_RELEASE" | |
PREPARE_FOR_SUBMISSION = "PREPARE_FOR_SUBMISSION" | |
READY_FOR_DISTRIBUTION = "READY_FOR_DISTRIBUTION" | |
READY_FOR_REVIEW = "READY_FOR_REVIEW" | |
REJECTED = "REJECTED" | |
REPLACED_WITH_NEW_INFO = "REPLACED_WITH_NEW_INFO" | |
WAITING_FOR_REVIEW = "WAITING_FOR_REVIEW" | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the scope is the same
After looking at the connect_api/models
folder, I don't think the scope is the same. It's likely that the words used by the API would not change, but it would not be appropriate to use one for the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's likely that the words used by the API would not change
@mokagio You've clearly never worked with Apple's ASC API 😅😁 (they've changed the API without warning—nor updating their doc—at multiple occasions in the past sadly, requiring us to update fastlane in a hurry sometimes to account for the sudden change 😑)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the DRY refactors for another PR? It slows down the adoption of fixes.
We can limit the scope of this PR to the actual fix.
Would love to see this change in, it's been a problem for ages.
Fixes #18408
Checklist
bundle exec rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validci/circleci
builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)Motivation and Context
The ASC API to delete TestFlight pending builds doesn't exist, so when fastlane's pilot is trying to
reject_build_waiting_for_review
, we get this error:Description
Applying the fix from #18408 (comment) to delete pending submissions properly — kudos to @nid90 for the code of the fix.
Testing Steps
I've just tested the fix with one of our apps which has a TestFlight build still in "Waiting for Review" state.
DELETE
not being allowed.Gemfile
to this branch of thefastlane
repo, this time it passed with the following logs in our CI:and the previous build
345001
was properly marked as "Expired" in TestFlight after that: