Remove skip_glotpress and related cleanup

[iangmaia]

EDIT:
The initial PR had a few other related changes, but independent, so we decided to break it down into smaller PRs built on top of this one. See Related PRs.

What does it do?

1️⃣ Removes the skip_glotpress parameter from the ios_bump_version_release action

Fixing #372, this PR goes on the route of completely removing the skip_glotpress config item, given the latest localisation tooling doesn't rely on a local download_metadata.swift in the hosted project anymore.

2️⃣ Adds basic tests to ios_bump_version_release

In order to test the changes, a suite of basic unit tests were added to the ios_bump_version_release action.

Related PRs

• Remove legacy localisation script references and actions iangmaia/release-toolkit#2 -- includes the changes from @mokagio's old PR and builds on top of this one.

Checklist before requesting a review

• Run bundle exec rubocop to test for code style violations and recommendations
• Add Unit Tests (aka specs/*_spec.rb) if applicable
• Run bundle exec rspec to run the whole test suite and ensure all your tests pass
• Make sure you added an entry in the CHANGELOG.md file to describe your changes under the approprioate existing ### subsection of the existing ## Trunk section.

[AliSoftware]

@iangmaia Thanks for the PR. Took a quick look but then got pre-empted by another task so couldn't finish my review, so I'll just submit the few comments / thoughts I wrote but note that this is not a full review yet 😅 I'll try to look into it in more details tomorrow or the next day.

I liked that you added tests, though I wonder if they might not be testing more the implementation details than the final results? Though it's a tricky balance to find, also because most of the actions you're testing involve git operations (including push) and we definitively want to mock them. But still, I wondered what your thoughts were on that.

Also it feels like this PR packs many different things. I don't mind having a single PR to add the tests and do the changes, that makes sense, but I wonder if some of the small changes in that PR unrelated to the action removal—e.g. the small refactorings like env_project_root() instead of ENV['PROJDCT_ROOT_FOLDER'] etc—couldn't have been extracted in a separate PR stacked/built on top of this one, to not mix refactorings and removal of deprecated code.

Again, that's sometimes a tough balance to find because if the refactorings are very small renaming that doesn't warrant a dedicated PR just for so little. But my first impression when I read the PR and the PR description was "wow, 4 different things packed in one single PR" and then the diff shown by GitHub doesn't always help (depending on how it chooses to decide where to split the diffs of added/removed lines) + me being EOD already… probably didn't help 😄 Yet, if you think splitting in the changes in separate PR is doable and makes sense to you, then please do.

And of course, given the breakcrumbs of comments I left already (about Deliverfile and maybe more of the code also being obsolete), we will likely have subsequent PRs to also clean some more anyway, so…

[AliSoftware]

Thought: at that point I'm also wondering if the commit_version_bump is going to be useful very long.

I'm not sure we have many projects left that contain the version number as part of their Deliverfile (and that thus gets bumped there) instead of the more modern way we have of doing it in client projects that don't involve the version to be in Deliverfile anymore.
If we get rid of that too in a subsequent PR very soon, we won't need the include_deliverfile either, which will make that helper method kinda pointless if the remaining of its implementation is just "commit ./config/* files" (and that path to ./config/*.xcconfig should not be assumed/hardcoded in that helper anyway…)

Definitively not for this PR because its scope is already large enough, but to keep in mind in a subsequent cleanup after this lands (or stacked on top)

[iangmaia]

Got it! Yes, definitely for a subsequent PR. This is something more obviously in another domain as we're not talking about removing deprecated localisation related code, but anyway, relates to the affected files, thanks for pointing it out 👍

[iangmaia]

@AliSoftware thanks for the feedback, looking forward to the rest but this already gives me a few things to think about 🙂

I liked that you added tests, so I wonder if they might not be testing more the implementation details than the final results? Though it's a tricky balance to find, also because most of the actions you're testing involve git operations (including push) and we definitively want to mock them. But still, I wondered what your thoughts were on that.

It's indeed a tricky balance to find. I actually thought about this after sending the PR. My general line of thought, in this case, was that the dependencies could have their own unit tests and that these were unit tests for the method/action logic (checking the different scenarios, error cases, how data was going out to dependencies, etc) as opposed to something closer to an integration test (e.g. focused more on final results with all or mostly the real dependencies).
I think there is an argument to even have both, but often choices have to be made 🙂

Also it feels like this PR packs many different things. I don't mind having a single PR to add the tests and do the changes, that makes sense, but I wonder if some of the small changes in that PR unrelated to the action removal—e.g. the small refactorings like env_project_root() instead of ENV['PROJDCT_ROOT_FOLDER'] etc—couldn't have been extracted in a separate PR stacked/built on top of this one, to not mix refactorings and removal of deprecated code.

To me each change kinda led to the next one and they're all related (but not strictly dependent, as you point out) -- this small refactoring of ENV['PROJDCT_ROOT_FOLDER'] -> env_project_root(), for example, was made with the intent of making it easier to unit test commit_version_bump() from ios_git_helper.rb.
So it was like "remove skip_glotpress" led to -> "remove everything else referring to download_metadata.swift, adding tests for commit_version_bump() " led to -> "Remove /Scripts/update-translations.rb and the related action".
But yes I agree, they could have been split for an easier reviews 👍

Again, that's sometimes a tough balance to find because if the refactorings are very small renaming that doesn't warrant a dedicated PR just for so little. But my first impression when I read the PR and the PR description was "wow, 4 different things packed in one single PR" and then the diff shown by GitHub doesn't always help (depending on how it chooses to decide where to split the diffs of added/removed lines) + me being EOD already… probably didn't help 😄 Yet, if you think splitting in the changes in separate PR is doable and makes sense to you, then please do.
And of course, given the breakcrumbs of comments I left already (about Deliverfile and maybe more of the code also being obsolete), we will likely have subsequent PRs to also clean some more anyway, so…

Yes, when I checked the comment on Deliverfile, this made even more sense. I agree it's a difficult balance sometimes, and different teams / companies may have different "thresholds" on what is considered a big PR or not. As I said, in the beginning I thought "everything is related, so fine to keep it here", but the fact that they are independent changes with different goals is a good argument to split them into different PRs, IMO.

So I'm now thinking of keeping changes 1 & 2 here in this PR (so the main work fixing the issue is here) and 3 & 4 & any further cleanups can go to their own PRs.
Since this is clear, I'll just do it now to then make it easier already for everyone to continue on their review (this discussion is also gonna help with the fix for the next issue).

[mokagio]

If there's time left in the trial, it might be fun to remove Deliver file from the apps still using it. WooCommerce iOS is one of them.

[mokagio]

In regards to PR size and scope

I agree it's a difficult balance sometimes, and different teams / companies may have different "thresholds" on what is considered a big PR or not. As I said, in the beginning I thought "everything is related, so fine to keep it here", but the fact that they are independent changes with different goals is a good argument to split them into different PRs, IMO.

It's a bit of a science and a bit of an art, isn't it?

I find that in the context of a highly-distributed, async-first team like ours, it's best to err on the size of smaller PRs, ideally independent and parallel ones.

When the feedback cycle on a PR can take a full day because of the relative location of author and reviewer, having a single PR with various items in it might result in all the items being blocked from merging because of an issue found in only one of them. Splitting them allows unrelated changes not to block each other.

[AliSoftware]

Overall looks good, nice isolated PR, and adding tests that we were crucially missing 👍

I guess the next step will be to continue the cleanup by getting back on Gio's Draft PR you found earlier, then lastly check there are no other leftovers (of the our old way of relying on scripts that were expected to be in the client repo) to clean up after all that.

I'll let @mokagio confirm (here on or the P2 where you asked the question) if the blocker for his Draft PR back in the day (PocketCasts-iOS) is now lifted and let him tell you if it makes sense for you to take over his Draft PR 🙂

[AliSoftware]

My general line of thought, in this case, was that the dependencies could have their own unit tests and that these were unit tests for the method/action logic (checking the different scenarios, error cases, how data was going out to dependencies, etc)

I like that rationale, great answer, and I agree 👍

So I'm now thinking of keeping changes 1 & 2 here in this PR (so the main work fixing the issue is here) and 3 & 4 & any further cleanups can go to their own PRs.

Sounds like a good plan 👍

[iangmaia]

Thanks for your comments, @mokagio.

If there's time left in the trial, it might be fun to remove Deliver file from the apps still using it. WooCommerce iOS is one of them.

That would be interesting, yes, I hope there will be some time left 😬

It's a bit of a science and a bit of an art, isn't it?

I find that in the context of a highly-distributed, async-first team like ours, it's best to err on the size of smaller PRs, ideally independent and parallel ones.

When the feedback cycle on a PR can take a full day because of the relative location of author and reviewer, having a single PR with various items in it might result in all the items being blocked from merging because of an issue found in only one of them. Splitting them allows unrelated changes not to block each other.

Exactly, very good point -- larger PRs can potentially take much longer to be merged due to eventual issues found in the review, specially in an async fashion.

[mokagio]

I'll let @mokagio confirm (here on or the P2 where you asked the question) if the blocker for his Draft PR back in the day (PocketCasts-iOS) is now lifted and let him tell you if it makes sense for you to take over his Draft PR 🙂

Pocket Casts iOS no longer uses the legacy APIs.

@iangmaia I don't think you can push to #370 directly not being a contributor. Maybe it's easier to cherry-pick those changes and make a new PR from there. Or, given #370 is showing conflicts, only look at the diff as a starting point.