Deprecate Deliverfile in favor of Fastfile

[iangmaia]

Description

Similarly to what was done for WPiOS on wordpress-mobile/WordPress-iOS#16805, this PR removes the Deliverfile moving everything to a new lane in the Fastfile.
Additionally, the need for an app_version set in the Deliverfile has been deprecated and its usage is gonna soon be removed altogether from release-toolkit, so this PR already cleans this up.

Testing instructions

Testing the lane is similar to wordpress-mobile/WordPress-iOS#16805 (and is particularly difficult for me to do completely, so I'd also appreciate some help):

1. Checkout this branch
2. Run bundle exec fastlane update_metadata_on_app_store_connect
3. Verify the Preview.html contains valid data. In particular, that it's not attempting to upload screenshots
4. Cancel the upload 🙏 Does the Preview on path './fastlane/Preview.html' look okay for you? (y/n) n

Also good to test the use of the parameter with_screenshots:true to see that it will attempt to load the screenshots.

---

• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[iangmaia]

One question I had was related to the api_key_path: the Deliverfile originally had .configure-files/app_store_connect_fastlane_api_key.json, but the existing constant ASC_KEY_PATH has a completely different path to the same file, which is the one I'm using here.
Perhaps someone can shed some light on the reasoning behind the two different paths? Perhaps the files are redundant?

[AliSoftware]

Perhaps someone can shed some light on the reasoning behind the two different paths? Perhaps the files are redundant?

That's surprising that this path to .configure-files was still around in the Deliverfile to be honest. I think that was a leftover we overlooked when @mokagio migrated those configure files from inside the repo and into the user's $HOME

So I am not sure how the Release Managers have still been able to run the bundle exec fastlane deliver command and have it working lately, given the decrypted files don't live in the .configure-files dir of the repo anymore, but do live in $HOME (inside SECRETS_DIR) nowadays.

The best explanation that I have is that they had that file in their machine back from the time they were doing releases before #4576 landed (so back when that path was correct), and since those files are in .gitignore and they never ran git clean to remove those old files from their Mac, they happen to still have those files from before around (and since the API key hasn't changed since… the old file was still valid) and that's why it didn't break… while in shouldn't work if run from the machine of someone who never decrypted the secrets in their machine before #4576 landed. @spencertransier—as the current WCiOS's release manager—can you confirm that's what has been happening on your Mac so far when you ran the command during the past times you followed the release scenario?

[iangmaia]

The best explanation that I have is that they had that file in their machine back from the time they were doing releases before #4576 landed (so back when that path was correct), and since those files are in .gitignore and they never ran git clean to remove those old files from their Mac, they happen to still have those files from before around (and since the API key hasn't changed since… the old file was still valid) and that's why it didn't break

That's some serious 🕵️ work right there 😄 but yes, that would explain it, it makes sense and is a very good guess.

[iangmaia]

I didn't set the team_id which was being set in the Deliverfile, as I believe it isn't always necessary. I've seen some other lanes using get_required_env('EXT_EXPORT_TEAM_ID') but I wasn't sure whether it was safe to use it in this case, as the context in which this lane is run might be different.

Anyway, please let me know if team_id is in fact needed and I'll amend the PR.

[AliSoftware]

We do manage multiple teams in App Store Connect. Most of our apps, including WooCommerce, are in our "Automattic, Inc" team, but some other apps like DayOne and Tumblr are still in their own separate org/team on ASC (for historical reasons).

This means that some (if not most) of engineers in Apps Infra, who will at some point rotate in doing the release management for different apps, or cover for someone else in the team while they are away, will end up having multiple App Store Connect team registered with their Apple ID, and thus it's still important to pass an explicit value here as letting fastlane guess it implicitly won't work on a multiple-team setup like ours.

As for EXT_EXPORT_TEAM_ID:

• This is an env var with the value PZYM8XX95Q (corresponding to our "Automattic, Inc" team ID), which is stored alongside some of our secrets (in the file decoded from .configure-files/project.env.enc by our secrets tooling to somewhere in your $HOME, then read at the start of the Fastfile by this line). Hence why its value is then read with get_required_env in some places
• But in practice it's not really a secret value, and there is no real reason for us to store that value in an encrypted way. One day we should remove it from the project.env.enc encrypted secrets file and just move it as a constant in the Fastfile directly, with its value in clear because there is no reason for it to be considered as a secret
• That being said, iinm, upload_to_app_store/deliver and testflight actions expect a different kind of team_id, namely the numeric value 299112 that we had in the Deliverfile (which is thus different from the value of EXT_EXPORT_TEAM_ID).
• Again, that value is also stored in our .configure-files/project.env.enc file as a "secret", under an ENV var named FASTLANE_ITC_TEAM_ID… while there is no real reason for it to be considered a "secret" (in was already appearing in clear in our Deliverfile too, after all…)

So long story short, the one you want to use is not get_required_env('EXT_EXPORT_TEAM_ID') (which equals PZYM8XX95Q), but either use get_required_env('FASTLANE_ITC_TEAM_ID') instead to get the value 299112… or you might also use the 299112 constant directly as well (like we do in WPiOS)… because it's not really a secret value after all.

[iangmaia]

This means that some (if not most) of engineers in Apps Infra, who will at some point rotate in doing the release management for different apps, or cover for someone else in the team while they are away, will end up having multiple App Store Connect team registered with their Apple ID, and thus it's still important to pass an explicit value here as letting fastlane guess it implicitly won't work on a multiple-team setup like ours.

Got it! Yes, I wasn't 100% sure about that (plus, though WPiOS uses the team_id for testflight(), it doesn't use it for upload metadata 🤔). Thanks for clarifying!

And also, good to know the additional context and info on these vars and the secrets file, very helpful 👍

[AliSoftware]

I like that you used the existing constants in your new call site instead of just copy/pasting the string literals from the Deliverfile 👍

A couple of changes are needed though:

• To answer your question above, we do need to specify the team_id explicitly because most of us have multiple ASC teams registered with our AppleID, so Fastlane might not be able to guess which one to use magically.
• Since we will not use the Deliverfile anymore, we need to pass skip_deliver: true to the action that bumps the version, otherwise it will try to find the Deliverfile to update it and fail and crash (†)

(†) which, it turns out, is a detail that we also forgot about when we worked on wordpress-mobile/WordPress-iOS#16805 that you're referencing in your PR description, requiring us to fix the oversight later via wordpress-mobile/WordPress-iOS@6b88866 🙃

---

PS: Adding @spencertransier to the loop FYI, since once this PR lands, we'll need to update this line of the WCiOS release scenario to instruct the RM to call the new lane that was added in this PR, instead of calling the bare fastlane deliver command directly. And also update the explanations from this step now that we'd stop needing to update the Deliverfile.

[AliSoftware]

We do manage multiple teams in App Store Connect. Most of our apps, including WooCommerce, are in our "Automattic, Inc" team, but some other apps like DayOne and Tumblr are still in their own separate org/team on ASC (for historical reasons).

This means that some (if not most) of engineers in Apps Infra, who will at some point rotate in doing the release management for different apps, or cover for someone else in the team while they are away, will end up having multiple App Store Connect team registered with their Apple ID, and thus it's still important to pass an explicit value here as letting fastlane guess it implicitly won't work on a multiple-team setup like ours.

As for EXT_EXPORT_TEAM_ID:

• This is an env var with the value PZYM8XX95Q (corresponding to our "Automattic, Inc" team ID), which is stored alongside some of our secrets (in the file decoded from .configure-files/project.env.enc by our secrets tooling to somewhere in your $HOME, then read at the start of the Fastfile by this line). Hence why its value is then read with get_required_env in some places
• But in practice it's not really a secret value, and there is no real reason for us to store that value in an encrypted way. One day we should remove it from the project.env.enc encrypted secrets file and just move it as a constant in the Fastfile directly, with its value in clear because there is no reason for it to be considered as a secret
• That being said, iinm, upload_to_app_store/deliver and testflight actions expect a different kind of team_id, namely the numeric value 299112 that we had in the Deliverfile (which is thus different from the value of EXT_EXPORT_TEAM_ID).
• Again, that value is also stored in our .configure-files/project.env.enc file as a "secret", under an ENV var named FASTLANE_ITC_TEAM_ID… while there is no real reason for it to be considered a "secret" (in was already appearing in clear in our Deliverfile too, after all…)

So long story short, the one you want to use is not get_required_env('EXT_EXPORT_TEAM_ID') (which equals PZYM8XX95Q), but either use get_required_env('FASTLANE_ITC_TEAM_ID') instead to get the value 299112… or you might also use the 299112 constant directly as well (like we do in WPiOS)… because it's not really a secret value after all.

[AliSoftware]

Perhaps someone can shed some light on the reasoning behind the two different paths? Perhaps the files are redundant?

That's surprising that this path to .configure-files was still around in the Deliverfile to be honest. I think that was a leftover we overlooked when @mokagio migrated those configure files from inside the repo and into the user's $HOME

So I am not sure how the Release Managers have still been able to run the bundle exec fastlane deliver command and have it working lately, given the decrypted files don't live in the .configure-files dir of the repo anymore, but do live in $HOME (inside SECRETS_DIR) nowadays.

The best explanation that I have is that they had that file in their machine back from the time they were doing releases before #4576 landed (so back when that path was correct), and since those files are in .gitignore and they never ran git clean to remove those old files from their Mac, they happen to still have those files from before around (and since the API key hasn't changed since… the old file was still valid) and that's why it didn't break… while in shouldn't work if run from the machine of someone who never decrypted the secrets in their machine before #4576 landed. @spencertransier—as the current WCiOS's release manager—can you confirm that's what has been happening on your Mac so far when you ran the command during the past times you followed the release scenario?

[iangmaia]

• Since we will not use the Deliverfile anymore, we need to pass skip_deliver: true to the action that bumps the version, otherwise it will try to find the Deliverfile to update it and fail and crash (†)

Indeed, well noted (here and in the next app PRs) 👍
I actually briefly thought about this and the PR which removes this parameter completely, but in fact the idea was to first have the apps ready not using the Deliverfile to then later adapt them with this and other upgrades such as the removal of skip_glotpress and such. I'll push the changes in a bit!

[AliSoftware]

Looking good 👍

PS: @spencertransier FYI I've opened https://github.com/Automattic/mobile-toolbox/pull/314 to adjust the release scenario accordingly for once this lands.