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.
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.
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.
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-androidas a binary imported library to fastlanevalidate_submodules_stringslane and its CI task. @loremattei mentioned thatlocalize_libslane 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.txtif necessary.