[deliver] fix app previews always running during usage.#29989
Conversation
iBotPeaches
changed the title
| UI.important("Uploading App Preview videos...") | ||
| validate_path! | ||
|
|
||
| localizations = editable_version.get_app_store_version_localizations |
There was a problem hiding this comment.
Seems unrelated but looks ok
There was a problem hiding this comment.
yeah good call out. I first thought adding some defensive logic in case you can't get a version, but now that its probably gated - it was probably overkill.
| it 'sets up app previews folder in fastlane folder' do | ||
| expect(options[:app_previews_path]).to eq('./fastlane/app-previews') | ||
| it 'does not automatically set up app previews folder in fastlane folder even if it exists' do | ||
| FileUtils.mkdir_p('./fastlane/app-previews') |
There was a problem hiding this comment.
Creating and removing folders within tests is a bit brittle.
unless you want to mock the file system
- you could add an extra desc/before/after to ensure they get taken care of by rspsec
- you could also use a test specific temp folder (as in Deliver::UploadMetadata spec)
There was a problem hiding this comment.
makes sense. I adapted following the pattern of UploadMetadata (spawning a temp dir, working in there, cleaning up in after)
ccaruceru
left a comment
ccaruceru
left a comment
There was a problem hiding this comment.
this lgtm. the only suggestion I have is to remove the tests about app_previews_path since they became... irrelevant.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @iBotPeaches 👋
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 month, 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 🚀"
There was a problem hiding this comment.
Congratulations! 🎉 This was released as part of fastlane 2.233.1 🚀
github-actions
Bot
added
status: released
and removed
status: included-in-next-release
labels
4 participants
Problem
App Previews is gated by the config parameter
app_previews_path, which was silently populated like it was an output directory. So the logic always ran. Which would normally just "no-op" since no videos, but it first captures version number. If the IPA is purely internal and no public facing versions - it crashes.Solution
Gate App Previews only be the existence of the parameter. This make using App Previews "opt in".
fixes: #29983