-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[action][download_dsyms] use filter argument to /builds to find specific build #19670
Conversation
Unfortunately this did not work. I think only version can be passed in, not build number? The former is not specific enough to be useful in filtering. |
In my case I have 500 builds under v2020 and another 500 under v2021. So using Anyway I updated this PR so its functional now, but would be nice to find further performance improvements here. If the API let us also specify |
…d; break out of loop because we know results are sorted by -uploadedDate
Alright I managed to get this working. Now the same |
@tmm1 I think what you want is
Example of using Sooo... you could filter by |
You're right, that totally worked and is super fast. Updated this PR accordingly! |
Before:
After:
|
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.
This looks great! I added one commit to fix the tests but this is 🔥 Thanks you so much ❤️
@@ -101,7 +101,7 @@ def self.run(params) | |||
|
|||
if after_uploaded_date && after_uploaded_date >= uploaded_date | |||
UI.verbose("Upload date #{after_uploaded_date} not reached: #{uploaded_date}") | |||
break | |||
next |
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.
Using next here doesn't really make sense to me. I tried to reason about it in my commit message in dac7592
Since we know the results are sorted by uploadedDate, I think it should be safe to break out of the loop when we're past the expected date already.
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.
@tmm1 Yeah yeah, that does seem okay but I don't fully trust they will be returned in the right order 😬 I trust the official ASC API but we also use the proxy endpoint for Apple ID auth which is the one I don't really trust. So I think I'd feel safer here doing a next just to make sure all the correct ones are found. The slowest part was the API request so this shouldn't increase things too much 🤞
I hope my insane uneasiness makes sense 😛
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.
Fair enough ⛵
Hey @tmm1 👋 Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉 Please let us know if this change requires an immediate release by adding a comment here 👍 |
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.
Congratulations! 🎉 This was released as part of fastlane 2.199.0 🚀
fixes #19669
cc @joshdholtz