Add opt-in failures for translation downloads - #771
Conversation
8e71f24 to
99b2ad2
Compare
Generated by 🚫 Danger |
There was a problem hiding this comment.
Pull request overview
Ensures GlotPress download and metadata parsing failures propagate, causing translation CI jobs to fail instead of silently producing incomplete output.
Changes:
- Adds explicit download errors for HTTP, network, redirect, and retry failures.
- Converts metadata parsing and iOS write failures into fatal errors.
- Adds coverage across shared, Android, iOS, and metadata download paths.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents the corrected failure behavior. |
lib/.../helper/glotpress_downloader.rb |
Raises explicit errors after failed downloads. |
lib/.../helper/metadata_download_helper.rb |
Makes invalid JSON fatal. |
lib/.../helper/ios/ios_l10n_helper.rb |
Propagates downloads and write failures. |
lib/.../helper/android/android_localize_helper.rb |
Prevents partial filter results. |
spec/glotpress_downloader_spec.rb |
Tests shared failure behavior. |
spec/metadata_download_helper_spec.rb |
Tests metadata failures. |
spec/ios_l10n_helper_spec.rb |
Tests iOS helper failures. |
spec/ios_download_strings_files_from_glotpress_spec.rb |
Tests action-level propagation. |
spec/android_localize_helper_spec.rb |
Tests prevention of partial exports. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
iangmaia
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_download_strings_files_from_glotpress.rb:78
plutilcan successfully parse a syntactically valid property list that is not a string-to-string dictionary (for example, a root array or an integer value). In those cases this line raisesNoMethodErrorvia.keysorvalue.empty?, outside the new rescue block. That breaks the default mode's legacy error-only handling and gives strict mode a raw exception instead of the intended Fastlane validation error. Validate the parsed shape/values, or route the entire post-parse validation throughreport_validation_error.
empty_keys = translations.select { |_, value| value.nil? || value.empty? }.keys.sort
| unless params[:fail_on_error] | ||
| Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( | ||
| project_url: params[:project_url], | ||
| locale: glotpress_locale, | ||
| filters: params[:filters], | ||
| destination: destination | ||
| ) | ||
| validate_strings_file(destination) unless params[:skip_file_validation] | ||
| next | ||
| end | ||
|
|
||
| if File.exist?(destination) && !File.file?(destination) | ||
| UI.user_error!("The destination `#{destination}` exists but is not a regular file") | ||
| end | ||
|
|
||
| destination_mode = File.exist?(destination) ? File.stat(destination).mode & 0o7777 : 0o644 | ||
| Tempfile.create([params[:table_basename], '.strings'], lproj_dir) do |temporary_file| | ||
| downloaded = Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file( | ||
| project_url: params[:project_url], | ||
| locale: glotpress_locale, | ||
| filters: params[:filters], | ||
| destination: temporary_file, | ||
| fail_on_error: true | ||
| ) | ||
| next unless downloaded | ||
|
|
||
| temporary_file.flush | ||
| # Do a quick check of the downloaded `.strings` file to ensure it looks valid | ||
| validate_strings_file(temporary_file.path, display_path: destination, fail_on_error: true) unless params[:skip_file_validation] | ||
| File.chmod(destination_mode, temporary_file.path) | ||
| temporary_file.close | ||
| File.rename(temporary_file.path, destination) | ||
| end |
There was a problem hiding this comment.
Feels like a lot of repetition (similar call to Fastlane::Helper::Ios::L10nHelper.download_glotpress_export_file in both branches) while this could probably be DRY'd in a single branch?
I mean, I'd be ok to use the atomic replacement approach via a temp file even if fail_on_error was kept as false, thus doing the same thing here regardless of fail_on_error, and only add the condition for the relevant lines (i.e. next unless downloaded || !fail_on_error + pass fail_on_error: fail_on_error to the call to validate_strings_file)
There was a problem hiding this comment.
👍
Improved in 21f993a, consolidating both modes in the atomic tempfile path.
| rescue StandardError => e | ||
| UI.error "Error downloading locale `#{locale}` — #{e.message} (#{url})" | ||
| nil | ||
| prefix = fail_on_error ? 'Error writing downloaded locale' : 'Error downloading locale' |
There was a problem hiding this comment.
If you end up DRYing the calls like I suggested above while keeping the atomic writing logic via temp file regardless of fail_on_error, then we can switch back to a fixed message prefix "Error while downloading/validating locale"
94dad0d to
ef11903
Compare
ef11903 to
35fa64a
Compare
| OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS = { | ||
| key: :fail_on_error, | ||
| description: 'Whether handled errors should fail the lane instead of using the action-specific fallback behavior', | ||
| type: FastlaneCore::Boolean, | ||
| optional: true, | ||
| default_value: false | ||
| }.freeze |
There was a problem hiding this comment.
Why not expose this as a method returning the ConfigItem instance directly, rather than having to do FastlaneCore::ConfigItem.new(**Fastlane::Helper::ConfigItemHelper::OPT_IN_FAIL_ON_ERROR_CONFIG_ITEM_OPTIONS) at every call site?
Fixes AINFRA-2204
What does it do?
Adds opt-in
fail_on_errorhandling to Android, iOS, and metadata translation actions while preserving existing default behavior.When enabled, it:
.strings, and metadata JSON before replacing files, preventing malformed metadata responses from deleting existing files.It also improves redirect handling.
Checklist before requesting a review
bundle exec rubocopto test for code style violations and recommendations.specs/*_spec.rb) if applicable.bundle exec rspecto run the whole test suite and ensure all your tests pass.CHANGELOG.mdfile to describe your changes under the appropriate existing###subsection of the existing## Trunksection.MIGRATION.mdfile to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider. Not applicable.