-
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
[supply] introduce a new synchronization logic for screenshots #21498
Conversation
Compare the checksum of the images to be uploaded with the ones already in remote, and skip upload if they already match
uri = URI.parse(url) | ||
clean_url = [ | ||
uri.scheme, | ||
uri.userinfo, | ||
uri.host, | ||
uri.port, | ||
uri.path | ||
].join | ||
|
||
UI.verbose("Initial URL received: '#{url}'") | ||
UI.verbose("Removed params ('#{uri.query}') from the URL") | ||
UI.verbose("URL after removing params: '#{clean_url}'") |
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.
All those lines were dead code even before my change: as you can see, clean_url
was never used in the returned value after all.
60a1530
to
5bad9ce
Compare
supply/spec/uploader_spec.rb
Outdated
it 'should delete screenshots that are out of order and re-upload them in the correct order' do | ||
allow(Digest::SHA256).to receive(:file) { |file| instance_double(Digest::SHA256, hexdigest: "sha256-of-#{file}") } | ||
local_images = %w[image0.png image1.png image2.png image4.png image3.png image5.png image6.png] # those will be sorted after Dir.glob | ||
allow(Dir).to receive(:glob).and_return(local_images) | ||
|
||
# Record the mocked deletions and uploads in list of remote images to check the final state at the end | ||
final_remote_images_ids = {} | ||
allow(client).to receive(:clear_screenshot) do |**args| | ||
image_type = args[:image_id].split('_')[1] | ||
final_remote_images_ids[image_type].delete(args[:image_id]) | ||
end | ||
allow(client).to receive(:upload_image) do |**args| | ||
path = File.basename(args[:image_path]) | ||
image_type = args[:image_type] | ||
final_remote_images_ids[image_type] << "new-id_#{image_type}_#{path}" | ||
end | ||
|
||
Supply::SCREENSHOT_TYPES.each do |screenshot_type| | ||
remote_images = local_images.map do |path| | ||
Supply::ImageListing.new("id_#{screenshot_type}_#{path}", '_unused_', "sha256-of-#{path}", '_unused_') | ||
end # remote images will be in order 0124356 though | ||
allow(client).to receive(:fetch_images).with(image_type: screenshot_type, language: language).and_return(remote_images) | ||
|
||
final_remote_images_ids[screenshot_type] = remote_images.map(&:id) | ||
|
||
# We should skip image0, image1, image2 from remote as they are the same as the first local images, | ||
# But also skip image3 (which was in-between 2 and 3 in remote listing, but is still present in local images) | ||
# While deleting image4 (because it was in-between image2 and image3 in the `remote_images`, so out of order) | ||
# And finally deleting image5 and image6, before re-uploading image4, image5 and image6 in the right order | ||
local_images.sort[0..3].each do |path| | ||
expect(client).not_to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: "id_#{screenshot_type}_#{path}") | ||
expect(client).not_to receive(:upload_image).with(image_path: File.expand_path(path), image_type: screenshot_type, language: language) | ||
end | ||
local_images.sort[4..6].each do |path| | ||
expect(client).to receive(:clear_screenshot).with(image_type: screenshot_type, language: language, image_id: "id_#{screenshot_type}_#{path}") | ||
expect(client).to receive(:upload_image).with(image_path: File.expand_path(path), image_type: screenshot_type, language: language) | ||
end | ||
end | ||
|
||
uploader = Supply::Uploader.new | ||
uploader.upload_screenshots(language) | ||
|
||
# Check the final order of the remote images after the whole skip/delete/upload dance | ||
Supply::SCREENSHOT_TYPES.each do |screenshot_type| | ||
expected_final_images_ids = %W[ | ||
id_#{screenshot_type}_image0.png | ||
id_#{screenshot_type}_image1.png | ||
id_#{screenshot_type}_image2.png | ||
id_#{screenshot_type}_image3.png | ||
new-id_#{screenshot_type}_image4.png | ||
new-id_#{screenshot_type}_image5.png | ||
new-id_#{screenshot_type}_image6.png | ||
] | ||
expect(final_remote_images_ids[screenshot_type]).to eq(expected_final_images_ids) | ||
end | ||
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.
This unit test is the most interesting case I wanted to get right, as it shows a case where you have the same images locally and remotely, but you swapped the order of some of your screenshots:
- On the Play Store you initially had the images that correspond to
image0.png
,image1.png
,image2.png
,image4.png
,image3.png
,image5.png
,image6.png
, currently appearing in that order in the Play Store - But since then, you decided to swap the order of
image4.png
andimage3.png
and now you want to re-upload the images (which will be uploaded in alphabetical order, assupply
has always done) - This will thus:
- Keep
image0.png
,image1.png
andimage2.png
untouched - Delete the
image4.png
, which is currently afterimage2.png
in PlayStore — because it's now out of order compared to the local files - But keep
image3.png
in PlayStore (that came afterimage4.png
, but will now come afterimage2.png
sinceimage4.png
was deleted from the PlayStore edit) - And then delete
image5.png
andimage6.png
, to finally re-uploadimage4.png
,image5.png
andimage6.png
in the correct order.
- Keep
This looks like this when running this spec with DEBUG=1 bundle exec rspec supply/spec/uploader_spec.rb
:
…
[22:30:47]: 🔍 Checking phoneScreenshots checksums...
[22:30:47]: 🟰 Skipping upload of screenshot image0.png as remote sha256 matches.
[22:30:47]: 🟰 Skipping upload of screenshot image1.png as remote sha256 matches.
[22:30:47]: 🟰 Skipping upload of screenshot image2.png as remote sha256 matches.
[22:30:47]: 🚮 Deleting pt-BR screenshot id #id_phoneScreenshots_image4.png as it does not exist locally or is out of order...
[22:30:47]: 🟰 Skipping upload of screenshot image3.png as remote sha256 matches.
[22:30:47]: 🚮 Deleting pt-BR screenshot id #id_phoneScreenshots_image5.png as it does not exist locally or is out of order...
[22:30:47]: 🚮 Deleting pt-BR screenshot id #id_phoneScreenshots_image6.png as it does not exist locally or is out of order...
[22:30:47]: ⬆️ Uploading screenshot image4.png...
[22:30:47]: ⬆️ Uploading screenshot image5.png...
[22:30:47]: ⬆️ Uploading screenshot image6.png...
…
existing_images.each do |image| | ||
if image.sha256 == first_path_checksum | ||
UI.message("🟰 Skipping upload of screenshot #{paths.first} as remote sha256 matches.") | ||
paths.shift # Remove first path from the list of paths to be uploaded | ||
first_path_checksum = paths.empty? ? nil : Digest::SHA256.file(paths.first).hexdigest | ||
else | ||
UI.message("🚮 Deleting #{language} screenshot id ##{image.id} as it does not exist locally or is out of order...") | ||
client.clear_screenshot(image_type: screenshot_type, language: language, image_id: image.id) | ||
end | ||
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.
Sadly, there doesn't seem to be an Google API on AndroidPublisherService
that allows to just reorder the screenshots, hence the need to delete the screenshots that are out of order and re-upload them, so that when new uploaded screenshots go at the end of the list, the final order is still correct in Google Play.
This is why I only compare with the checksum of the next local file (well, in practice, checksum of paths.first
, while paths
items are being shift
'ed and removed from the paths
array as they get skipped), instead of using something like all_local_checksums.include?(image.sha256)
to just check if the remote image matches an existing local image, which would not guarantee that we'd end up with the expected order of the screenshots in the 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.
@AliSoftware more of an operational question... With deliver
, if you pass skip_screenshots: false
, it will always delete all screenshots and upload them all again even if they're still the same images as before — right? That's what I observe when testing, at least... So what I've been doing for the past few years was to pas skip_screenshots: true
when I want to update any screenshot, and false otherwise.
First, is your workflow for deliver
different than this? Does it behave differently for you? 🤔
Second, wouldn't it make sense for you to apply the same behavior I just described to your Android workflow, and thus pass skip_upload_images: true
and/or skip_upload_screenshots: true
when the images haven't changed, and false
otherwise?
From your PR description alone it sounds like both deliver
and supply
are on the same page in terms of implementation (except ASC allows us to sort the screenshots, but that's besides the point here haha).
WDYT? Could you clarify if I understood something wrong? :)
Thanks!
Ah I should have clarified in the PR description sorry: on our end we use the This is the behavior of deliver that this PR is bringing to supply. PS: Note about feature flagNote that in deliver that
|
Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
# Upload an image or screenshot of a specific type for a given language to your store listing | ||
# | ||
# @param [String] image_type (e.g. phoneScreenshots, sevenInchScreenshots, ...) | ||
# @param [String] language localization code (i.e. BCP-47 language tag as in `pt-BR`) |
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.
🇧🇷 🥳
full_url | ||
(result.images || []).map do |row| | ||
full_url = "#{row.url}=s0" # '=s0' param ensures full image size is returned (https://github.com/fastlane/fastlane/pull/14322#issuecomment-473012462) | ||
ImageListing.new(row.id, row.sha1, row.sha256, full_url) |
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.
Really nice find to see it has all this info (sha256, etc) 🔥
@@ -210,6 +210,11 @@ def self.available_options | |||
description: "Whether to skip uploading SCREENSHOTS", | |||
type: Boolean, | |||
default_value: false), | |||
FastlaneCore::ConfigItem.new(key: :sync_image_upload, |
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.
Should we give it same name as deliver
's option name (and a similar name to the env var)? I think that would make the API more familiar to devs that maintain fastlane for both platforms, or are just curious reading the Fastfile, etc. That'd make this sync_screenshots
and SUPPLY_SYNC_SCREENSHOTS
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.
The reason I didn't name it sync_screenshots
is deliberate: this is because this not only applies to screenshots, but also to Play Store artwork — typically the featuredGraphic
and icon
images that are not screenshots but rather the banner and large icon shown in the Google Play's download page for the app.
I thought calling it sync_screenshots
would make people think that this only applied to screenshots (as in skip_upload_screenshots: false
) while it also applies to skip_upload_images: false
too.
Arguably, because the app artwork is referenced by the term …images…
in the skip_upload_images
option, one could argue that calling my new parameter sync_image_upload
would make people think that it applies to that option only (i.e. where image
term here would be interpreted as "the artwork images uploaded as part of skip_upload_images: false
), while the intent of the term …image…
in sync_image_upload
here is more to say "sync the upload of every kind of [PNG, JPEG, …] image file".
But I couldn't find a term that would encompass both "each image file, be it a screenshot or an artwork image" other than just the term "image", hence settling for this.
As for trying to make the deliver
and supply
option names in sync, that ship as sadly already sailed: For example, deliver
uses skip_screenshots
while supply
uses skip_upload_screenshots
(and skip_upload_images
), with _upload_
in that option name for supply
while deliver
doesn't have it 🤷
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.
Good points! 😅 I think you named it well 😊 Thanks for clarifying!
@@ -270,7 +270,17 @@ def upload_images(language) | |||
path = Dir.glob(search, File::FNM_CASEFOLD).last | |||
next unless path | |||
|
|||
UI.message("Uploading image file #{path}...") | |||
if Supply.config[:sync_image_upload] | |||
UI.message("🔍 Checking #{image_type} checksum...") |
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.
I wouldn't call it a checksum I think. The term is usually used to check whether the fetched is what the sender sent (checking data integrity, so see if it didn't get corrupted during the transfer, or altered by a middleman, etc). Here I'd probably just call "Checking […] hash…" or "Comparing existing image with remote one…" or something like that?
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 point.
I've already merged the PR though (in the hope that we'd include this PR in the new release that @joshdholtz said he'd do later today to fix the Google API random errors as we talked about in Slack), so it's a bit too late 😅.
Not sure if it's worth opening a subsequent PR just to fix this wording? (but if you feel strongly about it I don't mind us doing so to fix it)
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.
Either way works for me! Not strongly opinionated, just think it'd be more befitting for what's actually happening :) I'll leave this up to you! If you decide to open the PR, tag me to review and I'll approve and merge it asap 😊
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)I've updated the documentation if necessary.Motivation and Context
Similar to the optimization we do with
deliver
andsync_screenshots: true
for ASC, this change intents to allow optimizing the upload of images and screenshots to avoid re-uploading screenshots that already exist in Play Store Console.This is especially useful if you call
upload_to_play_store
withskip_upload_images: false
and/orskip_upload_screenshots: false
every time you release a new version… but in practice don't change your app screenshots orfeatureGraphic
/icon
images on every release.In addition to optimizing the screenshots upload and the number of upload requests done to the Google API, this also avoids having a long list of those screenshot updates showing up in the "Publishing Overview" of the PlayStore Console on every release when we didn't change the screenshot images since last time. While up until now, since we did a delete-and-reupload-all-screenshots on every call to
upload_to_play_store
, the PlayStore Console showed them as changes in "Publishing Overview > Changes Ready to Publish" even if the images were the same before/after.Description
The new algorithm replaces the call to
clear_screenshots
—which was previously done unconditionally before uploading all the screenshots again—with a selective call toclear_screenshot(…, image.id)
for images that are present in PSC but not present locally anymore (based on their sha256), or for images that are out of order. All while skipping upload for the ones that already present remotely and still in the right order, then finally only uploading the ones that are new locally, or re-uploading the ones that were out of order at the end.sync_image_upload
parameter toSupply::Options
to enable this new feature. Defaults tofalse
to keep backwards compatibility.ImageListing
model to represent the data from theImage
type returned by the Google API.Supply::Client#fetch_images
to now return anArray<ImageListing>
instead of anArray<String>
of URLs, so that we also have access to thesha256
andid
fields of the existing images in PSCfetch_images
implementationSupply::Setup#download_images
, to callmap(&:url)
on the result, to keep working as beforeSupply::Client#clear_screenshot(image_type:, language:, image_id:)
to delete single screenshots (as opposed toclear_screenshots
which deletes them all at once)Testing Steps
supply/spec/uploader_spec.rb
to test tricky cases withsync_image_upload: true
upload_to_play_store
withskip_upload_images: false, skip_upload_screenshots: false, sync_image_upload: true, validate_only: true
on the following situations, with our original screenshots being[A,B,C,D,E]
:B
locally ➡️ that screenshot got deleted in the edit; all other screenshots were skipped.A,C,B,D,E
) ➡️ screenshotsB
,D
andE
got deleted from the edit (leaving onlyA
andC
in PSC), then screenshotsB
,D
andE
got re-uploaded.F
locally ➡️ all screenshotsA–E
got skipped, new screenshotF
got uploaded.