[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.
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.
AliSoftware
force-pushed
the
sync-supply-screenshots
branch
from
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.
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.pngandimage3.pngand now you want to re-upload the images (which will be uploaded in alphabetical order, assupplyhas always done) - This will thus:
- Keep
image0.png,image1.pngandimage2.pnguntouched - Delete the
image4.png, which is currently afterimage2.pngin PlayStore — because it's now out of order compared to the local files - But keep
image3.pngin PlayStore (that came afterimage4.png, but will now come afterimage2.pngsinceimage4.pngwas deleted from the PlayStore edit) - And then delete
image5.pngandimage6.png, to finally re-uploadimage4.png,image5.pngandimage6.pngin 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.
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.
@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>
rogerluan
changed the title
| # 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`) |
| 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.
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.
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.
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.
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.
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.
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.
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 rspecfrom the root directory to see all new and existing tests passbundle exec rubocop -ato ensure the code style is validci/circlecibuilds 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
deliverandsync_screenshots: truefor 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_storewithskip_upload_images: falseand/orskip_upload_screenshots: falseevery time you release a new version… but in practice don't change your app screenshots orfeatureGraphic/iconimages 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_uploadparameter toSupply::Optionsto enable this new feature. Defaults tofalseto keep backwards compatibility.ImageListingmodel to represent the data from theImagetype returned by the Google API.Supply::Client#fetch_imagesto now return anArray<ImageListing>instead of anArray<String>of URLs, so that we also have access to thesha256andidfields of the existing images in PSCfetch_imagesimplementationSupply::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_screenshotswhich deletes them all at once)Testing Steps
supply/spec/uploader_spec.rbto test tricky cases withsync_image_upload: trueupload_to_play_storewithskip_upload_images: false, skip_upload_screenshots: false, sync_image_upload: true, validate_only: trueon the following situations, with our original screenshots being[A,B,C,D,E]:Blocally ➡️ that screenshot got deleted in the edit; all other screenshots were skipped.A,C,B,D,E) ➡️ screenshotsB,DandEgot deleted from the edit (leaving onlyAandCin PSC), then screenshotsB,DandEgot re-uploaded.Flocally ➡️ all screenshotsA–Egot skipped, new screenshotFgot uploaded.