-
Notifications
You must be signed in to change notification settings - Fork 9
Share GlotPress 429 auto retry from ASC metadata to iOS strings download #516
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
010074e
6bac21d
5d1b52a
f088131
6326cf7
ee2bb8b
fe9a93b
aa6bf79
975dc46
ed269cf
0c1b71b
cd87e7b
8433810
7f3ccb5
cd5ef51
d35d027
80ddf81
71077a8
48896c8
0330b3b
0847fc9
65801ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
2.7.4 | ||
3.2.2 | ||
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,52 @@ | ||||||||||
require 'net/http' | ||||||||||
|
||||||||||
module Fastlane | ||||||||||
module Helper | ||||||||||
class GlotpressDownloader | ||||||||||
AUTO_RETRY_SLEEP_TIME = 20 | ||||||||||
MAX_AUTO_RETRY_ATTEMPTS = 30 | ||||||||||
|
||||||||||
def initialize( | ||||||||||
auto_retry: true, | ||||||||||
auto_retry_sleep_time: 20, | ||||||||||
auto_retry_max_attempts: 30 | ||||||||||
Comment on lines
+11
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
) | ||||||||||
@auto_retry = auto_retry | ||||||||||
@auto_retry_sleep_time = auto_retry_sleep_time | ||||||||||
@auto_retry_max_attempts = auto_retry_max_attempts | ||||||||||
@auto_retry_attempt_counter = 0 | ||||||||||
end | ||||||||||
|
||||||||||
def download(glotpress_url, options = {}) | ||||||||||
uri = URI(glotpress_url) | ||||||||||
response = Net::HTTP.get_response(uri, options) | ||||||||||
|
||||||||||
case response.code | ||||||||||
when '200' # All good pass the result along | ||||||||||
response | ||||||||||
when '301' # Follow the redirect | ||||||||||
UI.message("Received 301 for `#{response.uri}`. Following redirect...") | ||||||||||
download(response.header['location']) | ||||||||||
when '429' # We got rate-limited, auto_retry or offer to try again with a prompt | ||||||||||
if @auto_retry | ||||||||||
if @auto_retry_attempt_counter < @auto_retry_max_attempts | ||||||||||
UI.message("Received 429 for `#{response.uri}`. Auto retrying in #{@auto_retry_sleep_time} seconds...") | ||||||||||
sleep(@auto_retry_sleep_time) | ||||||||||
@auto_retry_attempt_counter += 1 | ||||||||||
download(response.uri, options) | ||||||||||
else | ||||||||||
UI.error("Abandoning `#{response.uri}` download after #{@auto_retry_attempt_counter} retries.") | ||||||||||
end | ||||||||||
elsif UI.confirm("Retry downloading `#{response.uri}` after receiving 429 from the API?") | ||||||||||
download(response.uri, options) | ||||||||||
else | ||||||||||
UI.error("Abandoning `#{response.uri}` download as requested.") | ||||||||||
end | ||||||||||
else | ||||||||||
message = "Received unexpected #{response.code} from request to URI #{response.uri}." | ||||||||||
UI.abort_with_message!(message) unless UI.confirm("#{message} Continue anyway?") | ||||||||||
end | ||||||||||
end | ||||||||||
end | ||||||||||
end | ||||||||||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -161,19 +161,33 @@ def self.generate_strings_file_from_hash(translations:, output_path:) | |
# Typical examples include `{ status: 'current' }` or `{ status: 'review' }`. | ||
# @param [String, IO] destination The path or `IO`-like instance, where to write the downloaded file on disk. | ||
# | ||
def self.download_glotpress_export_file(project_url:, locale:, filters:, destination:) | ||
def self.download_glotpress_export_file( | ||
project_url:, | ||
locale:, | ||
filters:, | ||
destination:, | ||
autoretry: true, | ||
autoretry_max: Fastlane::Helper::GlotpressDownloader::MAX_AUTO_RETRY_ATTEMPTS, | ||
autoretry_sleep: Fastlane::Helper::GlotpressDownloader::AUTO_RETRY_SLEEP_TIME | ||
) | ||
query_params = (filters || {}).transform_keys { |k| "filters[#{k}]" }.merge(format: 'strings') | ||
uri = URI.parse("#{project_url.chomp('/')}/#{locale}/default/export-translations/?#{URI.encode_www_form(query_params)}") | ||
|
||
downloader = Fastlane::Helper::GlotpressDownloader.new( | ||
auto_retry: autoretry, | ||
auto_retry_sleep_time: autoretry_sleep, | ||
auto_retry_max_attempts: autoretry_max | ||
) | ||
|
||
# Set an unambiguous User Agent so GlotPress won't rate-limit us | ||
options = { 'User-Agent' => Wpmreleasetoolkit::USER_AGENT } | ||
|
||
response = downloader.download(uri.to_s, options) | ||
|
||
begin | ||
IO.copy_stream(uri.open(options), destination) | ||
IO.copy_stream(StringIO.new(response.body), destination) | ||
Comment on lines
-172
to
+188
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iirc, the reason I initially used Now that we don't use
wdyt? |
||
rescue StandardError => e | ||
UI.error "Error downloading locale `#{locale}` — #{e.message} (#{uri})" | ||
retry if e.is_a?(OpenURI::HTTPError) && UI.confirm("Retry downloading `#{locale}`?") | ||
return nil | ||
UI.error "Error saving locale `#{locale}` — #{e.message} (#{uri})" | ||
end | ||
end | ||
end | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,116 @@ | ||||||
require 'spec_helper' | ||||||
|
||||||
describe Fastlane::Helper::GlotpressDownloader do | ||||||
describe 'downloading' do | ||||||
context 'when auto retry is enabled' do | ||||||
context 'when GlotPress returs a 200 code' do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
it 'returns the response, without retrying' do | ||||||
downloader = described_class.new(auto_retry: true) | ||||||
fake_url = 'https://test.com' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed |
||||||
|
||||||
stub_request(:get, fake_url).to_return(status: 200, body: 'OK') | ||||||
|
||||||
response = downloader.download(fake_url) | ||||||
|
||||||
expect(response.code).to eq('200') | ||||||
expect(response.body).to eq('OK') | ||||||
# Make sure there was no retry | ||||||
assert_requested(:get, fake_url, times: 1) | ||||||
end | ||||||
end | ||||||
|
||||||
context 'when GlotPress returs a 429 code' do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
it 'retries automatically' do | ||||||
sleep_time = 0.1 | ||||||
downloader = described_class.new(auto_retry: true, auto_retry_sleep_time: sleep_time) | ||||||
fake_url = 'https://test.com' | ||||||
|
||||||
count = 0 | ||||||
stub_request(:get, fake_url).to_return do | ||||||
count += 1 | ||||||
if count == 1 | ||||||
{ status: 429, body: 'Too Many Requests' } | ||||||
else | ||||||
{ status: 200, body: 'OK' } | ||||||
end | ||||||
end | ||||||
|
||||||
expect(Fastlane::UI).to receive(:message) | ||||||
.with(/Received 429 for `#{fake_url}`. Auto retrying in #{sleep_time} seconds.../) | ||||||
|
||||||
response = downloader.download(fake_url) | ||||||
|
||||||
assert_requested(:get, fake_url, times: 2) | ||||||
expect(response.code).to eq('200') | ||||||
end | ||||||
|
||||||
context 'when the maximum number of retries is reached' do | ||||||
it 'aborts' do | ||||||
sleep_time = 0.1 | ||||||
max_retries = 3 | ||||||
downloader = described_class.new(auto_retry: true, auto_retry_sleep_time: sleep_time, auto_retry_max_attempts: max_retries) | ||||||
fake_url = 'https://test.com' | ||||||
|
||||||
count = 0 | ||||||
stub_request(:get, fake_url).to_return do | ||||||
count += 1 | ||||||
{ status: 429, body: 'Too Many Requests' } | ||||||
end | ||||||
|
||||||
expect(Fastlane::UI).to receive(:message) | ||||||
.with(/Received 429 for `#{fake_url}`. Auto retrying in #{sleep_time} seconds.../) | ||||||
.exactly(max_retries).times | ||||||
|
||||||
expect(Fastlane::UI).to receive(:error) | ||||||
.with(/Abandoning `#{fake_url}` download after #{max_retries} retries./) | ||||||
|
||||||
downloader.download(fake_url) | ||||||
|
||||||
assert_requested(:get, fake_url, times: max_retries + 1) # the original request plus the retries | ||||||
end | ||||||
end | ||||||
end | ||||||
end | ||||||
|
||||||
context 'when auto retry is disabled' do | ||||||
context 'when GlotPress returs a 200 code' do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
it 'returns the response, without retrying' do | ||||||
downloader = described_class.new(auto_retry: false) | ||||||
fake_url = 'https://test.com' | ||||||
|
||||||
stub_request(:get, fake_url).to_return(status: 200, body: 'OK') | ||||||
|
||||||
response = downloader.download(fake_url) | ||||||
|
||||||
expect(response.code).to eq('200') | ||||||
expect(response.body).to eq('OK') | ||||||
# Make sure there was no retry | ||||||
assert_requested(:get, fake_url, times: 1) | ||||||
end | ||||||
end | ||||||
|
||||||
context 'when GlotPress returs a 429 code' do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
it 'prompt the user for confirmation, ignoring the max auto retry parameter' do | ||||||
sleep_time = 0.1 | ||||||
max_retries = 2 | ||||||
downloader = described_class.new(auto_retry: false, auto_retry_sleep_time: sleep_time, auto_retry_max_attempts: max_retries) | ||||||
fake_url = 'https://test.com' | ||||||
|
||||||
stub_request(:get, fake_url).to_return(status: 429, body: 'Too Many Requests') | ||||||
|
||||||
count = 0 | ||||||
allow(Fastlane::UI).to receive(:confirm).with("Retry downloading `#{fake_url}` after receiving 429 from the API?") do | ||||||
count += 1 | ||||||
# Simulate user retrying more times that the max retries, then aborting | ||||||
count <= max_retries | ||||||
end | ||||||
|
||||||
# When the user aborts, with finish with an error | ||||||
expect(Fastlane::UI).to receive(:error).with(/Abandoning `#{fake_url}` download as requested./) | ||||||
|
||||||
downloader.download(fake_url) | ||||||
end | ||||||
end | ||||||
end | ||||||
end | ||||||
end |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,64 @@ | ||||||
require 'spec_helper' | ||||||
|
||||||
describe Fastlane::Helper::MetadataDownloader do | ||||||
describe 'downloading from GlotPress' do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test would benefit with ensuring the files are saved. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I'm following, isn't it was those tests already do, by using an |
||||||
it 'save the downloaded data to the target folder and file, including locale name' do | ||||||
in_tmp_dir do |tmp_dir| | ||||||
destination_name = 'target-file-name.txt' | ||||||
dummy_url = 'https://test.com' | ||||||
metadata_downloader = described_class.new(tmp_dir, { key: { desc: destination_name } }, true) | ||||||
|
||||||
stub_request(:get, dummy_url).to_return( | ||||||
status: 200, | ||||||
# GlotPress responses have a custom format. | ||||||
body: { "key\u0004test metadata" => ['test metadata'] }.to_json | ||||||
) | ||||||
|
||||||
metadata_downloader.download('en-AU', dummy_url, false) | ||||||
|
||||||
destination_path_with_locale = File.join(tmp_dir, 'en-AU', destination_name) | ||||||
expect(File.exist?(destination_path_with_locale)).to be true | ||||||
# We also expect a trailing new line. | ||||||
expect(File.read(destination_path_with_locale)).to eq("test metadata\n") | ||||||
end | ||||||
end | ||||||
|
||||||
context 'when GlotPress returs a 429 code' do | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
it 'automatically retries' do | ||||||
in_tmp_dir do |tmp_dir| | ||||||
metadata_downloader = described_class.new( | ||||||
tmp_dir, | ||||||
{ key: { desc: 'target-file-name.txt' } }, | ||||||
true, | ||||||
0.1 | ||||||
) | ||||||
|
||||||
fake_url = 'https://test.com' | ||||||
|
||||||
count = 0 | ||||||
stub_request(:get, fake_url).to_return do | ||||||
count += 1 | ||||||
if count == 1 | ||||||
{ status: 429, body: 'Too Many Requests' } | ||||||
else | ||||||
{ status: 200, body: 'OK' } | ||||||
end | ||||||
end | ||||||
|
||||||
expect(Fastlane::UI).to receive(:message) | ||||||
.with(/Received 429 for `#{fake_url}`. Auto retrying in 0.1 seconds.../) | ||||||
|
||||||
expect(Fastlane::UI).to receive(:message) | ||||||
.with(/No translation available for en-AU/) | ||||||
|
||||||
expect(Fastlane::UI).to receive(:success) | ||||||
.with(/Successfully downloaded `en-AU`./) | ||||||
|
||||||
metadata_downloader.download('en-AU', fake_url, false) | ||||||
|
||||||
assert_requested(:get, fake_url, times: 2) | ||||||
end | ||||||
end | ||||||
end | ||||||
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 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.