Extract GlotPress download retry logic and apply to all cases

[iangmaia]

Related to p1760606044394999-slack-CC7L49W13
Fixes AINFRA-1419

What does it do?

This PR extracts the GlotPress download code with retry logic originally from metadata_download_helper.rb to a separate helper GlotPressDownloader, and use that same helper also in android_localize_helper.rb and ios_l10n_helper.rb.

Testing

I've created a draft PR where I ran lanes that download GlotPress: wordpress-mobile/WordPress-Android#22291 and triggered two builds:

• First, running the lanes without the change: https://buildkite.com/automattic/wordpress-android/builds/23657
• Then running the lanes with the changes in this PR: https://buildkite.com/automattic/wordpress-android/builds/23659

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.

[AliSoftware]

Preliminary review, I didn't check the whole code in details nor the unit tests just yet, but leaving my initial high-level thoughts still as a first pass.

[AliSoftware]

Is there no way to propagate that locale to each method call that might need it, instead of using an instance variable, to avoid that risk altogether?
For example I think just passing it as a parameter to make_request (the only method which seems to reference that @current_locale apparently?) would be better?

Especially, I don't think we use Parallel.map right now to download multiple locales in parallel—but instead we just use a .each serial loop, right?—…but if we ever do, and thus one GlotPressDownloader might be reused for multiple loop iterations in parallel, storing that in an instance variable would lead to race condition issues.

[AliSoftware]

Also, the fact that we have to remember to reset the auto-retry counter once we get a 200 response (line 64) so that we're ready to handle the next call to download in the next caller loop iteration… makes it brittle too as well given the reuse context.

So I wonder if we shouldn't instead require to create a new instance for every single download, i.e. not have a single GlotpressDownloader instance created outside of the caller's loop and calling downloader.download(url, locale) on the same instance inside each loop iteration, but instead have the url + locale be on the constructor (def initialize) and download take no argument at all, to enfoce each loop iteration having to create a separate instance, so that we're guaranteed that there won't be any instance variables with old values we forgot to reset between loops, and that we wouldn't risk having race conditions from reuse (especially if we migrate to Parallel.map one day…)

[iangmaia]

Great suggestion, thanks! 👍 it makes the state management easier to follow. Updated on 19fc711.

[AliSoftware]

I wonder if is_ci? is the right condition for it here, as opposed to "is the current terminal interactive"… which I think Fastlane's UI module already have a helper for?

[EDIT] Yes, UI.interactive? is the one I had in mind 💭

[iangmaia]

Good one! 👍 Updated on 5f11e78.

[AliSoftware]

Approving to unblock, as the 2 comments are nitpicks not blockers.

PS: Probably worth testing this on a real project (e.g. making wordpress-android point its Gemfile to this PR's branch and run a test lane calling this to validate it works in real world use case) before merging, just to be extra sure.

[AliSoftware]

Given that all call sites will now basically always call downloader = GlotpressDownloader.new(…) followed immediately by downloader.download, I wonder if it would be worth adding a convenience class method too to make all the call sites that use that pattern even simpler?

def self.download(url:, locale:, auto_retry: false, &)
  downloader = self.new(url: url, locale: locale, auto_retry: auto_retry)
  downloader.download(&)
end

[iangmaia]

PS: Probably worth testing this on a real project

Done, updated the description with a reference to a draft PR.