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] add a check in app_version.setup_screenshots to also check if there is an app_preview already uploaded #14738
Conversation
…view alredy uploaded
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. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
The comment at the beginning of the method might need an update as well. Can you also explain the bigger picture here? What would a user do to trigger this code? |
Hey @xpy, looks like a great PR. What’s the fastest way to test this? Can I directly test it with spaceship? |
@max-ott Hello! I have to admit that I only tested around my case and I didn't dive into other cases. What I do is |
hi! we're encountering the same issue, is there any chance this will be merged? thank you! |
It needs more tests. We can’t simply merge each PR without enough tests and reviews from maintainers. As there were no other ways presented to easily test and debug this, its still waiting for more input from the author. |
@max-ott I already mentioned that I tested for the very case that the PR is about. I also described the way to reproduce, if it seems difficult to reproduce, it probably is because it is indeed a very specific case that is difficult to reproduce. I am not sure what else you expect from me to do to validate this PR. |
@xpy as this changes Easiest way would be to provide steps for the |
EDIT: This fixed indeed a small problem. To Do (Other PR): Display @joshdholtz This is ready to get merged. I spent ~ half a day testing this directly with |
This fix, fixes our CI has anyone have any issue with it ? |
fixed #15036 for me as well |
Hi @joshdholtz, |
Ah yes yes! I will do some review on this this week! Thanks for friendly ping ❤️ |
How could we help with this PR ? Did you get time to review it @joshdholtz ? |
@mackoj Thanks for ping on this! Got lost in the shuffle 😔 Bumping this to top of list for rest of this week |
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! Sorry it took so long to get through 😔 I'm really trying to improve my personal processes to keep conversations and merges going. Sorry to everyone that needed this but really appreciate the contribution! ❤️
Hey @xpy 👋 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.144.0 🚀
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 validMotivation and Context
When trying to do any update, fastlane will first update the
scaled
flags of the display_families totrue
unless they have a screenshot. This doesn't take in account that a trailer (app preview) might be present instead. As a result, thescaled
flag is set totrue
for said display_family and upon saving, the server complains that Your app preview can't be uploaded because you have selected to use an app preview from a larger display size.This happens when a user tries to do a deliver, even with the
skip_screenshots
flags set totrue
. And that is because Fastlane will send the whole configuration again in order to update any part of it. The prerequisites are that there should be only video previews in a display_family that can be set as scalable. It might also happen if theoverwrite_screenshots
flag is also set totrue
.Description
There is one more check added to check if the
trailers
key is present and thevalue
key of it has contents.