Ensure all downloaded translations and metadata are committed #18199

mokagio · 2022-03-24T03:37:34Z

I run the new_beta_release lane today and, while all the translations were downloaded correctly, the were not committed.

This PR has just enough code to address the committing issues. I did not track the result running the updated lane, i.e. the translated files, because I thought I'd ship a new beta on purpose for that, so we can see the diff in the usual "merge beta n" context. You can still see the commits either by creating a new branch from this one and running download_localized_strings_and_metadata, or by looking at this demo PR.

mokagio · 2022-03-24T03:49:26Z

fastlane/Fastfile

+    git_commit(
+      path: File.join(parent_dir_for_lprojs, '*.lproj', 'Localizable.strings'),
+      message: 'Update app translations – `Localizable.strings`',
+      allow_nothing_to_commit: true
+    )


If GitHub doesn't expand the lines on top of this diff, it might not be clear why this commit is here.

This commit is here to track the changes made by the ios_download_strings_files_from_glotpress. I haven't dug the history of how the action changed, but I'm guessing either that or its predecessor used to commit for us and we, rightly, removed it.

mokagio · 2022-03-24T03:50:15Z

fastlane/Fastfile

@@ -477,13 +477,25 @@ platform :ios do
      download_dir: parent_dir_for_lprojs
    )

-    # Then redispatch the appropriate subset of translations back to the `InfoPlist.strings` and `Sites.strings` files in corresponding `*.lproj` dirs
-    parent_dir_for_intents_target_lprojs = File.join('WordPress', 'WordPressIntents')


My editor told me about this unused variable 😄

Notice that the WordPressIntents app extension which the variable referred to is part of the MANUALLY_MAINTAINED_STRINGS_FILES Hash which we pass to current iteration of the extractor action.

This was already fixed by my "cleanup Fastfile" PR (which is build on top of my "split Fastfile" PR)… so committing this change here will likely lead to conflicts in both PR 😓

mokagio · 2022-03-24T03:52:23Z

fastlane/Fastfile

+
+    git_commit(
+      path: modified_files,
+      message: 'Update app translations – Other `.strings`',


I decided to use the same "root" for this commit as the one above and to differentiate them using "Localizable.strings" vs. "Other .strings".

There might be a nifty way to do it all in one commit, but I actually think it might be useful to do it separately. Apart from being the result of two different actions, it might also be useful to compare how a strings changed from GlotPress in the first commit with how it was extracted in the second.

In #18200, there are no changes to the extracted .strings. I verified this works locally by editing a prefixed string in Localizable.strings and running the action.

The reason for it is that those were the only files committed. You can see it worked well in the main release branch: 5fa5005 . Also notice how that commits doesn't have any Localizable.strings files, which further shows the necessity for the other git_commit call above.

mokagio · 2022-03-24T03:53:58Z

fastlane/Fastfile

@@ -515,7 +527,7 @@ platform :ios do
    FileUtils.cp(release_notes_source, File.join(metadata_directory, 'en-US', 'release_notes.txt'))

    git_commit(
-      path: Dir.glob('**/*.txt', base: metadata_directory),
+      path: File.join(metadata_directory, '**', '*.txt'),


Dir.glob('**/*.txt', base: metadata_directory) didn't work. From what I understand and saw locally, there are two reasons.

First, even if we use base:, the paths returned by glob are still relative. From the docs:

The optional base keyword argument specifies the base directory for interpreting relative pathnames instead of the current working directory. As the results are not prefixed with the base directory name in this case, you will need to prepend the base directory name if you want real paths.

Second, Dir.glob returns the files that match the given pattern, but that excludes deleted files. If a file is not there anymore, it can't match the given pattern. This would have made the commit incomplete.

We could argue whether our code should delete the existing translations when it doesn't find new ones. Even if we concluded that behavior is not desirable, we'd still need to account for it locally until we update it in the release toolkit. Otherwise, our local copies would remain dirty after downloading the release notes metadata if no new ones are available.

@mokagio One thing I just thought about with using git_commit with fastlane/metadata/**/*.txt is: is there a risk that this might also commit especially the review_information file — which contains secret information, especially credentials that Apple reviewers should use to test the app? 🤔

I think not, given this file is .gitignore'd in the first place, so I'm rather confident this is all good. But just wanted to flag this anyway as something to double-check next time you'll run deliver and all.
See also p91TBi-4Le-p2#comment-4004 — which is not the same thing (the file did not get commited, but that did not prevent to being updated and changed in our local Macs during a past deliver download) but tangential.

peril-wordpress-mobile · 2022-03-24T03:58:18Z

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

wpmobilebot · 2022-03-24T04:00:03Z

You can test the Jetpack changes on this Pull Request by downloading it from AppCenter here with build number: pr18199-2e3dbf0. IPA is available here. If you need access to this, you can ask a maintainer to add you.

wpmobilebot · 2022-03-24T04:01:33Z

You can test the WordPress changes on this Pull Request by downloading it from AppCenter here with build number: pr18199-2e3dbf0. IPA is available here. If you need access to this, you can ask a maintainer to add you.

AliSoftware · 2022-03-24T08:06:27Z

Thanks for the PR. I'd have preferred that you merge #18140 an #18192 first though, or build this PR on top of #18192, because as it is we can't merge this PR without breaking those other two (and also at least one of these changes were already fixed by those)

I'll try to rebase the PR and transcribe the changes yours made in the main Fastfile to be applied in the corresponding lanes/*.rb instead during my day.

And update other "`.strings` update" commit message to differentiate the two steps

The previous version used `Dir.glob('**/*.txt', base: metadata_directory)`, but that didn't work for two reasons. First, even if we use `base:`, the paths returned by `glob` are still relative. From the docs: > The optional base keyword argument specifies the base directory for > interpreting relative pathnames instead of the current working > directory. As the results are not prefixed with the base directory name > in this case, you will need to prepend the base directory name if you > want real paths. See https://ruby-doc.org/core-2.7.4/Dir.html#method-c-glob . Second, `Dir.glob` returns the files that match the given pattern, but that excludes _deleted_ files. If a file is not there anymore, it can't match the given pattern. This would have made the commit incomplete. We could argue whether our code should delete the existing translations when it doesn't find new ones. Even if we concluded that behavior is not desirable, we'd still need to account for it locally until we update it in the release toolkit. Otherwise, our local copies would remain dirty after downloading the release notes metadata if no new ones are available.

AliSoftware · 2022-03-24T18:11:50Z

@mokagio Hope you don't mind, I took the liberty to rebase your branch and force-push it on top of tooling/fastfiles-cleanup from #18192 because your changes were made on the pre-split Fastfile, and would have otherwise created a conflict. In practice, there would thus have been a big chance for those changes to be overwritten by the "Fastfile split" PR (which, in the eyes of git and GitHub, is just a "delete some lines and add some more" and not "cut some lines and paste the exact same ones elsewhere"… so it would probably have been hard to spot the overwrite 😉 )

I basically manually recreated your changes in new commits (instead of doing a git rebase, which would have been a mess anyway given the split which happened since) to reproduce the same changes you did, keeping your new git commit messages as well.

Since this is all part of a telescoping train of PRs, I suggest the following action and merge order:

I'll let you approve and merge [Tooling] Split Fastfile for readability and move lanes around #18140 first
After that, you can merge [Tooling] Additional cleanup of Fastfile lanes (DRYing + QR code support for Installable Builds) #18192 next (which you already approved, but I couldn't really merge until 18140 was approved and landed first)
Lately, if you approve of my rework of your commits in this PR, merge this one.
- Since you're the author for this PR and thus won't be able to approve your own PR, I've approved it on my end (after all, I agree with the changes that you made in your PR, and merely moved them to the right new place) so that you can hit the merge button yourself when everything is ready
Finally, once all 3 have landed in release/19.5, you'll be able to do a new beta to test it all (including the missing translations commit that this PR aims to fix) whenever you want

                  ╔════════════════════════════════════════╗
                  ║ release/19.5-localization-commit-fixes ║
                  ╚══╦═════════════════════════════════════╝
          ┌──────┐   │
3)        │#18199│   │
          └──────┘   │
              ╔══════▽════════════════════╗
              ║ tooling/fastfiles-cleanup ║
              ╚══╦═══╦════════════════════╝
       ┌──────┐  │   │
2)     │#18192│  │   │
       └──────┘  │   │
          ╔══════▽═══╩══════════════════════╗
          ║ tooling/fastfile-split-refactor ║
          ╚══╦═══╦═══╦══════════════════════╝
   ┌──────┐  │   │   │
1) │#18140│  │   │   │
   └──────┘  ▼   ▼   ▼
      ╔═══════════════════════════╗
      ║       release/19.5        ║
      ╚═══════════════════════════╝

wpmobilebot · 2022-03-24T18:23:28Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below
Then installing the build number pr18199-9c557da from App Center on your iPhone

The .ipa file can also be downloaded directly here.
If you need access to App Center, please ask a maintainer to add you.

wpmobilebot · 2022-03-24T18:24:07Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below
Then installing the build number pr18199-9c557da from App Center on your iPhone

The .ipa file can also be downloaded directly here.
If you need access to App Center, please ask a maintainer to add you.

AliSoftware

Approving after reworking the PR myself to make it be built on top of #18192 (i.e. post-fastlane-split), so it's a bit cheating (kinda approving my own changes here…)

@mokagio I'll let you handle the telescoping-merging of the 3 PRs as I described in #18199 (comment) if all look good to you.

mokagio · 2022-03-25T00:53:47Z

@AliSoftware your suggestion in regards to the order in which to merge the PRs so that they eventually all land in release/19.5 sounds good to me. I'll proceed with it now.

mokagio force-pushed the release/19.5-localization-commit-fixes branch from 9eb1900 to 2e3dbf0 Compare March 24, 2022 03:46

mokagio commented Mar 24, 2022

View reviewed changes

mokagio requested review from AliSoftware and a team March 24, 2022 03:57

mokagio marked this pull request as ready for review March 24, 2022 03:57

mokagio changed the title ~~Split a comment to multiline in Fastfile~~ Ensure all downloaded translations and metadata are committed Mar 24, 2022

mokagio mentioned this pull request Mar 24, 2022

Merge changes from beta 19.5.0.1 and editorialized release notes #18201

Merged

AliSoftware added 2 commits March 24, 2022 18:56

Ensure downloaded .strings files are committed

9eb7407

And update other "`.strings` update" commit message to differentiate the two steps

AliSoftware force-pushed the release/19.5-localization-commit-fixes branch from 2e3dbf0 to 9c557da Compare March 24, 2022 17:59

AliSoftware changed the base branch from release/19.5 to tooling/fastfiles-cleanup March 24, 2022 18:00

AliSoftware approved these changes Mar 24, 2022

View reviewed changes

Base automatically changed from tooling/fastfiles-cleanup to release/19.5 March 25, 2022 00:56

mokagio merged commit 110a6ec into release/19.5 Mar 25, 2022

mokagio deleted the release/19.5-localization-commit-fixes branch March 25, 2022 00:57

mokagio added this to the 19.5 ❄️ milestone Mar 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure all downloaded translations and metadata are committed #18199

Ensure all downloaded translations and metadata are committed #18199

mokagio commented Mar 24, 2022

mokagio Mar 24, 2022

mokagio Mar 24, 2022

AliSoftware Mar 24, 2022 •

edited

Loading

mokagio Mar 24, 2022

mokagio Mar 24, 2022 •

edited

Loading

mokagio Mar 24, 2022

AliSoftware Mar 25, 2022

peril-wordpress-mobile bot commented Mar 24, 2022 •

edited

Loading

wpmobilebot commented Mar 24, 2022 •

edited

Loading

wpmobilebot commented Mar 24, 2022 •

edited

Loading

AliSoftware commented Mar 24, 2022

AliSoftware commented Mar 24, 2022 •

edited

Loading

wpmobilebot commented Mar 24, 2022

wpmobilebot commented Mar 24, 2022

AliSoftware left a comment

mokagio commented Mar 25, 2022

Ensure all downloaded translations and metadata are committed #18199

Ensure all downloaded translations and metadata are committed #18199

Conversation

mokagio commented Mar 24, 2022

mokagio Mar 24, 2022

Choose a reason for hiding this comment

mokagio Mar 24, 2022

Choose a reason for hiding this comment

AliSoftware Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

mokagio Mar 24, 2022

Choose a reason for hiding this comment

mokagio Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

mokagio Mar 24, 2022

Choose a reason for hiding this comment

AliSoftware Mar 25, 2022

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Mar 24, 2022 • edited Loading

wpmobilebot commented Mar 24, 2022 • edited Loading

wpmobilebot commented Mar 24, 2022 • edited Loading

AliSoftware commented Mar 24, 2022

AliSoftware commented Mar 24, 2022 • edited Loading

wpmobilebot commented Mar 24, 2022

wpmobilebot commented Mar 24, 2022

AliSoftware left a comment

Choose a reason for hiding this comment

mokagio commented Mar 25, 2022

AliSoftware Mar 24, 2022 •

edited

Loading

mokagio Mar 24, 2022 •

edited

Loading

peril-wordpress-mobile bot commented Mar 24, 2022 •

edited

Loading

wpmobilebot commented Mar 24, 2022 •

edited

Loading

wpmobilebot commented Mar 24, 2022 •

edited

Loading

AliSoftware commented Mar 24, 2022 •

edited

Loading