-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Download store metadata when generating beta build #21623
Conversation
This should provide earlier feedback on the metadata translation, at least for the portion that has already been translated at the time a beta is made. This is consistent with what we do in iOS, see https://github.com/wordpress-mobile/WordPress-iOS/blob/b9a0deaf091739f33051f20cffdbe9a2097f645e/fastlane/lanes/release.rb#L219
Automatically pushing from lanes other than the root-release lanes (code freeze, new beta, finalize release, etc.) can result in undesired pushes, like it just happened to me in edcd40a
|
App Name | ![]() |
|
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr21623-83a65be | |
Commit | 83a65be | |
Direct Download | jetpack-prototype-build-pr21623-83a65be.apk |
|
App Name | ![]() |
|
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr21623-83a65be | |
Commit | 83a65be | |
Direct Download | wordpress-prototype-build-pr21623-83a65be.apk |
@@ -174,7 +174,7 @@ | |||
# | |||
# @return [void] | |||
# | |||
lane :download_metadata_strings do |app: nil, version: current_release_version, skip_release_notes: false, skip_commit: false, skip_git_push: false| | |||
lane :download_metadata_strings do |app: nil, version: current_release_version, skip_release_notes: false, skip_commit: false| |
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.
skip_git_push
is still mentioned in the comments above, and I noticed it is still used here 🤔
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.
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.
Hmm did you push these commits?
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.
Yes I did... now that you mentioned it 😳
@@ -173,6 +173,7 @@ | |||
|
|||
update_frozen_strings_for_translation | |||
download_translations | |||
download_metadata_strings(version: current_release_version) |
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.
Downloading store metadata when generating beta build should provide earlier feedback on the metadata translation
👍
@nbradbury 👋 I'm reopening this PR because we need the fix it implements during the release process. @oguzkocer I'm going to make it target the release branch, so that we can see if the next beta submission has the release notes that we expect. |
|
@mokagio Apologies, I did not intend to delete that branch. I was cleaning up stale branches and apparently got carried away! |
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.
👍
Plus, remove code that automatically pushed when running the metadata download lane.
Downloading store metadata when generating beta build should provide earlier feedback on the metadata translation, at
least for the portion that has already been translated at the time a beta is made.
The approach is consistent with what we do in iOS, see https://github.com/wordpress-mobile/WordPress-iOS/blob/b9a0deaf091739f33051f20cffdbe9a2097f645e/fastlane/lanes/release.rb#L219
As for the push removal, automatically pushing from lanes other than the root-release lanes (code
freeze, new beta, finalize release, etc.) can result in undesired pushes, like it happened to me in edcd40a .
Granted, I should have looked at the lane parameters before running it, but I think a case can be made for making the default behavior the desired one and for the desired behavior of a lane that "downloads metadata strings" to not include pushing to the remote.
Moreover, not pushing to the remote is consistent with what iOS does, see https://github.com/wordpress-mobile/WordPress-iOS/blob/b9a0deaf091739f33051f20cffdbe9a2097f645e/fastlane/lanes/localization.rb#L394-L466
Important
@wordpress-mobile/apps-infrastructure I operated on the above under the assumption that the automation script is as is because we never got to update it, and that there is no reason for skipping downloading the metadata during the beta process. Having never been the release manager for WordPress Android, I might be missing something obvious here, so please do let me know if that's the case. Thanks!
To Test:
These changes will be tested in the real world during the next release cycle, that is 25.7.
You can test the new lane behavior by running
bundle exec fastlane download_metadata_strings
and noticing it does not push to the remote.Example here's a screenshot of the run on my local, where you can see the logs do not include a git push and my terminal prompt shows
2⬆
which indicates my local branch is two commits ahead of the remote: the two metadata strings update commits, one for WP and one for JP.Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary. - N.A.Testing Checklist (strike-out the not-applying and unnecessary ones):
Not an app source PR.