-
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
Localization support for stories-android as a binary dependency #15368
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APKs: |
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.
When I run the localize_binary_deps
lane locally, I get the following error:
[12:57:32]: ----------------------------------------------
[12:57:32]: --- Step: android_download_file_by_version ---
[12:57:32]: ----------------------------------------------
[12:57:32]: Downloading stories/src/main/res/values/strings.xml from Automattic/stories-android at version 713-bb1d624a566bba82693b6f68fa44f5bbe8212e6d to /var/folders/29/wn6913pn73gf2l7j5n4n56mw0000gn/T
[12:57:32]: Can't download strings file for Stories Library.
Strings for this library won't get translated.
Do you want to continue anyway? (y/n)
I think what's happening is that the lane is looking for a 713-bb1d624a566bba82693b6f68fa44f5bbe8212e6d
tag on GitHub, but can't find it.
I tried using the most recent tag from GitHub, but the process fails there, too. My guess is that in this case it fails because there is no |
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.
I finally pushed a test tag, pointed to it, and verified that localized_binary_deps
updates the strings.xml
file using the values from the stories library.
In hindsight, I wonder wether I spent too much time testing this... After all, the tooling that does the strings merge has already been tested and proved it works in production many times over.
Still, I got something out of it which is understanding how the tool works, as I never had a chance to work with it, so it's a net positive 😄
I don't think the issues I mentioned with the process not finding the strings when the version is not a tag should be a blocker, because (my guess without checking the code is that) this process is meant to run only during the release flow, in which case we should always be pointing to a tag 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.
I haven't tested it, but I trust the tests @mokagio did.
Code changes look good to me.
Btw: I think we can deprecate and eventually remove the an_validate_lib_strings
action from release-toolkit
. AFAIK, it isn't used anywhere else and I think that since we developed the automatic import, the check is not useful anymore.
As discussed in Slack (p1633018037416900/1632506656.347400-slack-CC7L49W13), moving the milestone of those to 18.5, and we should be ready to merge these after today's code-freeze 🙂 |
This PR is targeting #15301 which is not yet reviewed. I wanted to separate the localization changes so it can be reviewed and tested in isolation. I'm going to leave this as a draft so the base PR will be easier to review, but I'd appreciate a review so it's ready to be merged.
This PR:
stories-android
as a binary imported library to fastlanevalidate_submodules_strings
lane and its CI task. @loremattei mentioned thatlocalize_libs
lane already covers our needs and developers don't need to make any changes anymore. He mentioned that we could have possibly removed this a while back 😅To test:
I'll leave the testing instructions to @loremattei. I believe they should be very similar to this PR.
Regression Notes
Potential unintended areas of impact
Localization issues for stories-android strings
What I did to test those areas of impact (or what existing automated tests I relied on)
Asked our localization master a.k.a @loremattei to review & test it 😅
What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txt
if necessary.