Handle 429 and prevent 301 in `gp_downloadmetadata` by mokagio · Pull Request #406 · wordpress-mobile/release-toolkit

mokagio · 2022-09-06T03:27:20Z

During the WordPress 20.6 finalization, we noticed that gp_downloadmetadata didn't offer to retry the GlotPress request when receiving a 429 (see details in this comment and this commit)

This PR adds a crude retry mechanism to the action.

While working on this, I noticed that the URL used was lacking the /, consistently resulting in a 301 from the API. I addressed that, too.

gp_downloadmetadata is set to be removed in favor of a better alternative as soon as we'll have time for it. I tried to write new code that was good, but kept the refactoring and improvements to a minimum.

Testing

I tested this by pointing my WordPress iOS to my local copy of the repo.

--- a/fastlane/Pluginfile
+++ b/fastlane/Pluginfile
@@ -8,7 +8,8 @@ end

 # This comment avoids typing to switch to a development version for testing.
 # gem 'fastlane-plugin-wpmreleasetoolkit', git: 'git@github.com:wordpress-mobile/release-toolkit', branch: 'tru>
-gem 'fastlane-plugin-wpmreleasetoolkit', '~> 5.1'
+# gem 'fastlane-plugin-wpmreleasetoolkit', '~> 5.1'
+gem 'fastlane-plugin-wpmreleasetoolkit', path: '../../release-toolkit'

I then run bundle exec fastlane download_wordpress_localized_app_store_metadata till I got a 429.

mokagio · 2022-09-06T03:39:01Z

lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_downloadmetadata_action.rb

          if loc.is_a?(Array)
-            puts "Downloading language: #{loc[1]}"
-            complete_url = "#{params[:project_url]}#{loc[0]}/default/export-translations?filters[status]=current&format=json"
+            UI.message "Downloading language: #{loc[1]}"


Converted the puts to UI.message for more consistent logs.

mokagio · 2022-09-06T03:40:59Z

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

+          if UI.confirm("Retry downloading `#{locale}` after receiving 429 from the API?")
+            download(locale, response.uri, is_source)
+          else
+            UI.error("Abandoning `#{locale}` download as requested.")


I decided to make this error so it would print red to mark the difference in behavior.

(The 301 is because I hadn't updated the URLs yet)

I can see how error/red might be misleading though, and look like a wholesale failure. If you think that's too confusing, I can update to a message:

Suggested change

UI.error("Abandoning `#{locale}` download as requested.")

UI.message("Abandoning `#{locale}` download as requested.")

Given the error message is pretty clear and it's in reaction of us explicitly replying n in the prompt on the previous line, I vote for keeping it red with UI.error 👍

crazytonyli · 2022-09-06T04:02:34Z

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

@@ -16,12 +16,7 @@ def initialize(target_folder, target_files)
      def download(target_locale, glotpress_url, is_source)
        uri = URI(glotpress_url)
        response = Net::HTTP.get_response(uri)


Just a thought, not related to this PR: it might be good to use faraday which has features like following redirects and retry with an interval (which could be a replacement for handling 429 error?).

Yeah... I don't know about faraday or other Ruby networking clients, but given the number of network requests we do with this toolkit, having a feature-rich way to do requests would be handy.

iinm Farady is also the network client that fastlane itself uses, so it should already be part of our ruby dependencies anyway.

This is one of those items part of a long list of little things I'd love to improve / refactor to clean up the release-toolkit in many areas and modernize it… but never got the bandwidth to 😅

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

AliSoftware

You're missing a CHANGELOG.md entry, so I'm blocking the merge to avoid accidentally landing this without it 😉
But apart from that, the rest LGTM 👍 So should be good to ship once you add that.

AliSoftware · 2022-09-06T14:19:55Z

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

+          if UI.confirm("Retry downloading `#{locale}` after receiving 429 from the API?")
+            download(locale, response.uri, is_source)
+          else
+            UI.error("Abandoning `#{locale}` download as requested.")


Given the error message is pretty clear and it's in reaction of us explicitly replying n in the prompt on the previous line, I vote for keeping it red with UI.error 👍

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

Suggestions have been implemented

AliSoftware · 2022-09-09T10:53:39Z

CHANGELOG.md

 ### Breaking Changes

-_None_
+- Propose to retry when `gp_downloadmetadata` receives a `429 - Too Many Requests` error. [#406]


@mokagio I'm not sure this is really a breaking change — as in, that won't require any change to client repos adopting the new release-toolkit to update their call sites or config or whatnot, will it?

I think it's not worth a major version bump on the next release, while a New Feature and minor bump would make more sense for this change 🙃

Yep, not at all a breaking change. That was an oversight which I realized only after merging this. It's fixed in 39808da.

Oh, perfect, thanks for following-up 👍

mokagio added 3 commits September 6, 2022 13:20

Update gp_downloadmetadata to offer to retry when receiving a 429

b8c44c4

Update a couple of puts in gp_downloadmetadata to UI.message

80af526

Add more UI messages to gp_downloadmetadata

31daea9

mokagio force-pushed the mokagio/gp_downloadmetadata_429 branch from d5e27fb to 31daea9 Compare September 6, 2022 03:29

Add trailing / to URL used by gp_downloadmetadata to avoid 301

0af9cfa

mokagio changed the title ~~Update gp_downloadmetadata to offer to retry when receiving a 429~~ Handle 429 and prevent 301 in gp_downloadmetadata Sep 6, 2022

mokagio commented Sep 6, 2022

View reviewed changes

mokagio marked this pull request as ready for review September 6, 2022 03:44

mokagio requested a review from a team September 6, 2022 03:44

crazytonyli reviewed Sep 6, 2022

View reviewed changes

AliSoftware previously requested changes Sep 6, 2022

View reviewed changes

crazytonyli approved these changes Sep 6, 2022

View reviewed changes

mokagio and others added 2 commits September 7, 2022 09:13

Show user unhandled gp_downloadmetadata errors and ask to continue

60106a5

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

Add CHANGELOG.md entry for #406

eafb2ed

mokagio merged commit 7abf936 into trunk Sep 6, 2022

mokagio deleted the mokagio/gp_downloadmetadata_429 branch September 6, 2022 23:23

mokagio added a commit that referenced this pull request Sep 6, 2022

Add CHANGELOG.md entry for #407 + Fix #406 entry

39808da

AliSoftware reviewed Sep 9, 2022

View reviewed changes

mokagio mentioned this pull request Sep 14, 2022

Release 5.5.0 into trunk #408

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle 429 and prevent 301 in `gp_downloadmetadata`#406

Handle 429 and prevent 301 in `gp_downloadmetadata`#406
mokagio merged 6 commits intotrunkfrom
mokagio/gp_downloadmetadata_429

mokagio commented Sep 6, 2022 •

edited

Loading

Uh oh!

mokagio Sep 6, 2022

Uh oh!

mokagio Sep 6, 2022 •

edited

Loading

Uh oh!

AliSoftware Sep 6, 2022

Uh oh!

crazytonyli Sep 6, 2022

Uh oh!

mokagio Sep 6, 2022

Uh oh!

AliSoftware Sep 6, 2022

Uh oh!

Uh oh!

AliSoftware left a comment

Uh oh!

AliSoftware Sep 6, 2022

Uh oh!

AliSoftware Sep 9, 2022 •

edited

Loading

Uh oh!

mokagio Sep 12, 2022 •

edited

Loading

Uh oh!

AliSoftware Sep 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	UI.error("Abandoning `#{locale}` download as requested.")
	UI.message("Abandoning `#{locale}` download as requested.")

Conversation

mokagio commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

mokagio Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

mokagio Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

crazytonyli Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

mokagio Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

AliSoftware Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AliSoftware left a comment

Choose a reason for hiding this comment

Uh oh!

AliSoftware Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

AliSoftware Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mokagio commented Sep 6, 2022 •

edited

Loading

mokagio Sep 6, 2022 •

edited

Loading

AliSoftware Sep 9, 2022 •

edited

Loading

mokagio Sep 12, 2022 •

edited

Loading