Share GlotPress 429 auto retry from ASC metadata to iOS strings download #516

mokagio · 2023-10-04T10:33:45Z

What does it do?

After benefitting from the GlotPress metadata downloads automatically retrying upon receiving a 429, it felt particularly painful, not to mention time consuming, to manually retry when the iOS .strings got a 429.

This PR extracts the "retry upon 429" logic in a dedicated GlotpressDownloader object, and updates the iOS strings and generic metadata download logic to use it.

I added as much test coverage as I could think of to guide my changes, but I didn't try to provide full coverage given the uncertain future of the metadata download component.

Checklist before requesting a review

Run bundle exec rubocop to test for code style violations and recommendations
Add Unit Tests (aka specs/*_spec.rb) if applicable
Run bundle exec rspec to run the whole test suite and ensure all your tests pass
Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
If applicable, add an entry in the MIGRATION.md file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider. — N.A.

I thought this might help with an issue I'm experiencing where there the uri of a stubbed response is unexpectedly nil, but it did not.

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb

See bblimke/webmock#469

This way, the tests can run fast, instead of waiting for the default 20s

lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb

lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb

mokagio · 2023-10-06T02:54:46Z

spec/glotpress_downloader_spec.rb

+      context 'when GlotPress returs a 200 code' do
+        it 'returns the response, without retrying' do
+          downloader = described_class.new(auto_retry: true)
+          fake_url = 'https://test.com'


fake_url is misleading... dummy_url might be better?

mokagio · 2023-10-06T02:56:03Z

spec/metadata_download_helper_spec.rb

+require 'spec_helper'
+
+describe Fastlane::Helper::MetadataDownloader do
+  describe 'downloading from GlotPress' do


This test would benefit with ensuring the files are saved.

I'm not sure I'm following, isn't it was those tests already do, by using an in_tmp_dir then testing expect(File.read(destination_path_with_locale)).to eq(…)?

While I was at it... Also useful to ensure the new downloader object didn't break the existing download logic.

mokagio · 2023-10-06T03:29:52Z

.ruby-version

This and the WebMock update were attempts to solve the tests having a missing uri in the stubbed response.

That turned out to be a bug in WebMock (see spec_helper diff). I decided to keep them because they don't seem to hurt, but I can remove this one if you think it should be a changed for a dedicated PR.

AliSoftware · 2023-10-09T13:25:03Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb

-            IO.copy_stream(uri.open(options), destination)
+            IO.copy_stream(StringIO.new(response.body), destination)


iirc, the reason I initially used IO.copy_stream was because uri.open happened to be a stream in the first place, and as such this would allow memory optimization (i.e. to avoid having to read the whole, large file content into memory all at once only to write it to the destination file in one block, and instead use copy_stream to write data to the destination as it arrived from the socket).

Now that we don't use uri.open anymore, I'm not sure it makes sense to continue using copy_stream then wrapping the whole response body inside a StringIO.new. I think we can probably instead:

Either just use File.write(destination, response.body) directly (which means we lose the memory footprint optimization of write-as-you-receive that copy_stream had, but is it a big deal?)

Or try using something like File.open(destination, mode: 'w') { |file| response.read_body(file) } (which I didn't try, but based on this doc I think that's how we'd be supposed to use it?) to still benefit from the "write-as-you-receive" behavior that would allow reducing the memory footprint for very large files.

wdyt?

AliSoftware · 2023-10-09T13:26:22Z

spec/glotpress_downloader_spec.rb

+      context 'when GlotPress returs a 200 code' do
+        it 'returns the response, without retrying' do
+          downloader = described_class.new(auto_retry: true)
+          fake_url = 'https://test.com'


AliSoftware · 2023-10-09T13:27:10Z

lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb

+        auto_retry_sleep_time: 20,
+        auto_retry_max_attempts: 30


Suggested change

auto_retry_sleep_time: 20,

auto_retry_max_attempts: 30

auto_retry_sleep_time: AUTO_RETRY_SLEEP_TIME,

auto_retry_max_attempts: MAX_AUTO_RETRY_ATTEMPTS

AliSoftware · 2023-10-09T13:29:28Z

spec/metadata_download_helper_spec.rb

+require 'spec_helper'
+
+describe Fastlane::Helper::MetadataDownloader do
+  describe 'downloading from GlotPress' do


I'm not sure I'm following, isn't it was those tests already do, by using an in_tmp_dir then testing expect(File.read(destination_path_with_locale)).to eq(…)?

AliSoftware · 2023-10-09T13:29:37Z

spec/metadata_download_helper_spec.rb

+      end
+    end
+
+    context 'when GlotPress returs a 429 code' do


Suggested change

context 'when GlotPress returs a 429 code' do

context 'when GlotPress returns a 429 code' do

AliSoftware · 2023-10-09T13:29:51Z

spec/glotpress_downloader_spec.rb

+        end
+      end
+
+      context 'when GlotPress returs a 429 code' do


Suggested change

context 'when GlotPress returs a 429 code' do

context 'when GlotPress returns a 429 code' do

AliSoftware · 2023-10-09T13:29:59Z

spec/glotpress_downloader_spec.rb

+    end
+
+    context 'when auto retry is disabled' do
+      context 'when GlotPress returs a 200 code' do


Suggested change

context 'when GlotPress returs a 200 code' do

context 'when GlotPress returns a 200 code' do

AliSoftware · 2023-10-09T13:30:11Z

spec/glotpress_downloader_spec.rb

+        end
+      end
+
+      context 'when GlotPress returs a 429 code' do


Suggested change

context 'when GlotPress returs a 429 code' do

context 'when GlotPress returns a 429 code' do

AliSoftware · 2023-10-09T13:30:21Z

spec/glotpress_downloader_spec.rb

+describe Fastlane::Helper::GlotpressDownloader do
+  describe 'downloading' do
+    context 'when auto retry is enabled' do
+      context 'when GlotPress returs a 200 code' do


Suggested change

context 'when GlotPress returs a 200 code' do

context 'when GlotPress returns a 200 code' do

mokagio added 3 commits October 3, 2023 16:59

Implement a prototype version of 429-auto-retry for iOS strings

010074e

Use Ruby 3.2.2

6bac21d

Use latest WebMock, 3.19.1

5d1b52a

I thought this might help with an issue I'm experiencing where there the uri of a stubbed response is unexpectedly nil, but it did not.

wpmobilebot reviewed Oct 5, 2023

View reviewed changes

lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb Outdated Show resolved Hide resolved

mokagio added 13 commits October 5, 2023 16:10

Add WebMock monkey-patch to work around nil uri in response

f088131

See bblimke/webmock#469

Add test for current metadata_download_helper behavior

6326cf7

Extract retry logic in GlotPressDownloader helper

ee2bb8b

Remove useless parameters from extracted GlotPressDownloader

fe9a93b

Add test for GlotPressDownaloader

aa6bf79

Move GlotpressDownloader tests in dedicated file

975dc46

Use label for auto_retry GlotpressDownloader init parameter

ed269cf

Make sleep time and retries configurable in GlotpressDownloader

0c1b71b

Add test for max retries and refine the implementation

cd87e7b

Add test for GlotPress 429 with manual retry

8433810

Add test for 200 behavior in GlotPress download

7f3ccb5

Make sleep time configurable in MetadataDownloader

cd5ef51

This way, the tests can run fast, instead of waiting for the default 20s

Move GlotpressDownaloder in dedicated file

d35d027

mokagio force-pushed the mokagio/auto-retry-on-strings-glotpress-429 branch from 28e8f7c to d35d027 Compare October 5, 2023 05:10

Use new GlotpressDownloader in ios_l10n_helper

80ddf81

mokagio commented Oct 6, 2023

View reviewed changes

mokagio and others added 4 commits October 6, 2023 14:00

Add missing options in recursive GlotpressDownloader call

71077a8

Remove an unused autoretry_count parameter

48896c8

Remove an unused auto_retry_max_attempts parameter

0330b3b

Add test for metadata downloader file saving behavior

0847fc9

While I was at it... Also useful to ensure the new downloader object didn't break the existing download logic.

mokagio marked this pull request as ready for review October 6, 2023 03:26

Add changelog entry for new 429 auto retry behavior

65801ac

mokagio commented Oct 6, 2023

View reviewed changes

mokagio requested review from AliSoftware and iangmaia October 6, 2023 03:30

AliSoftware reviewed Oct 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share GlotPress 429 auto retry from ASC metadata to iOS strings download #516

Share GlotPress 429 auto retry from ASC metadata to iOS strings download #516

mokagio commented Oct 4, 2023 •

edited

Loading

mokagio Oct 6, 2023

AliSoftware Oct 9, 2023

mokagio Oct 6, 2023

AliSoftware Oct 9, 2023

mokagio Oct 6, 2023

AliSoftware Oct 9, 2023

AliSoftware Oct 9, 2023

AliSoftware Oct 9, 2023

AliSoftware Oct 9, 2023

AliSoftware Oct 9, 2023

AliSoftware Oct 9, 2023

AliSoftware Oct 9, 2023

AliSoftware Oct 9, 2023

AliSoftware Oct 9, 2023

		IO.copy_stream(uri.open(options), destination)
		IO.copy_stream(StringIO.new(response.body), destination)

	context 'when GlotPress returs a 429 code' do
	context 'when GlotPress returns a 429 code' do

	context 'when GlotPress returs a 200 code' do
	context 'when GlotPress returns a 200 code' do

Share GlotPress 429 auto retry from ASC metadata to iOS strings download #516

Are you sure you want to change the base?

Share GlotPress 429 auto retry from ASC metadata to iOS strings download #516

Conversation

mokagio commented Oct 4, 2023 • edited Loading

What does it do?

Checklist before requesting a review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mokagio commented Oct 4, 2023 •

edited

Loading