Skip to content
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

[Tooling] Fix handling of empty release notes #21565

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

iangmaia
Copy link
Contributor

@iangmaia iangmaia commented Jan 7, 2025

Reference: p1733408151597889-slack-CC7L49W13

The main change is to better handle in the lane download_metadata_strings the scenario when the source release notes files (WordPress/metadata/release_notes.txt or WordPress/jetpack_metadata/release_notes.txt) are empty, clearing the localized files used by Fastlane (fastlane/metadata/android/*/changelogs/default.txt or fastlane/jetpack_metadata/android/*/changelogs/default.txt).

Additionally, I've also updated the lane download_metadata_strings lane to use keywords syntax for parameters.


To Test:

You can run the download_metadata_strings lane using the command below (avoiding commiting/pushing) to make sure the default.txt files are cleared when the source notes release_notes.txt are empty. When release_notes.txt has content, the default.txt files should be fetched from GlotPress using the specified version.

  • bundle exec fastlane download_metadata_strings version:25.2 skip_commit:true

@iangmaia iangmaia added this to the 25.7 milestone Jan 7, 2025
@iangmaia iangmaia requested review from oguzkocer and a team January 7, 2025 19:51
@iangmaia iangmaia self-assigned this Jan 7, 2025
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 25.7. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock, but left a suggestion to improve readability of implementation a bit if you want to apply it before merge.

File.write(File.join(changelog_dir, 'default.txt'), '')
end
# Skip downloading release notes translations since the source is empty
files.delete("release_note_#{version_suffix}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading this made me think at first that this was deleting a file with this name. Took me a minute to realize this was instead just removing the entry with that key from the files Hash instead.

Wouldn't it be more readable to do the check first, and only assign the value of files for that key (on line 201) if not empty?

In other words, write this block of code as:

if !File.exist?() || File.read().strip.empty?
  # Clear release notes if the source file is empty
  # …
else
  # Make sure to also download the release notes translations (given we had non-empty original copy)
  files["release_note_#{version_suffix}"] = {  }
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion 👍 Updated on b590c80

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 7, 2025

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr21565-b590c80
Commitb590c80
Direct Downloadjetpack-prototype-build-pr21565-b590c80.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 7, 2025

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr21565-b590c80
Commitb590c80
Direct Downloadwordpress-prototype-build-pr21565-b590c80.apk
Note: Google Login is not supported on these builds.

Copy link

sonarqubecloud bot commented Jan 7, 2025

@iangmaia iangmaia merged commit ff1fbb6 into trunk Jan 8, 2025
22 checks passed
@iangmaia iangmaia deleted the iangmaia/fix-release-notes-download branch January 8, 2025 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants