Skip to content

Deprecate Deliverfile in favor of Fastfile #8831

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 10, 2023
Merged

Deprecate Deliverfile in favor of Fastfile #8831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f6a4ba
Deprecate `Deliverfile` in favor of `Fastfile`
iangmaia Feb 6, 2023
616fb46
Add `team_id` parameter when uploading metadata to ASC
iangmaia Feb 10, 2023
fb64010
Update the bump version call to reflect that we no longer use the Del…
iangmaia Feb 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 0 additions & 30 deletions fastlane/Deliverfile
View file
Open in desktop

This file was deleted.

28 changes: 27 additions & 1 deletion fastlane/Fastfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ platform :ios do
lane :code_freeze do |options|
ios_codefreeze_prechecks(options)

ios_bump_version_release(skip_glotpress: true)
ios_bump_version_release(skip_glotpress: true, skip_deliver: true)
new_version = ios_get_app_version
extract_release_notes_for_version(
version: new_version,
Expand Down Expand Up @@ -585,6 +585,32 @@ platform :ios do
)
end

# Upload the localized metadata (from `fastlane/metadata/`) to App Store Connect
#
# @option [Boolean] with_screenshots (default: false) If true, will also upload the latest screenshot files to ASC
#
desc 'Upload the localized metadata to App Store Connect, optionally including screenshots.'
lane :update_metadata_on_app_store_connect do |options|
# Skip screenshots by default. The naming is "with" to make it clear that
# callers need to opt-in to adding screenshots. The naming of the deliver
# (upload_to_app_store) parameter, on the other hand, uses the skip verb.
with_screenshots = options.fetch(:with_screenshots, false)
skip_screenshots = !with_screenshots

upload_to_app_store(
app_identifier: APP_STORE_VERSION_BUNDLE_IDENTIFIER,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

app_version: ios_get_app_version,
team_id: '299112',
skip_binary_upload: true,
screenshots_path: File.join(FASTLANE_DIR, 'promo_screenshots'),
skip_screenshots: skip_screenshots,
overwrite_screenshots: true, # won't have effect if `skip_screenshots` is true
phased_release: true,
precheck_include_in_app_purchases: false,
api_key_path: ASC_KEY_PATH
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@iangmaia iangmaia Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
end

#####################################################################################
# register_new_device
# -----------------------------------------------------------------------------------
Expand Down