-
Notifications
You must be signed in to change notification settings - Fork 133
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.
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
Automation refactors to better support Enterprise builds #1749
Conversation
It's shorter
This will make it simpler to add bundle identifiers for a new app with different root bundle id.
Will differentiate from staging/alpha/prototype-build when we'll add it.
team_id: TEAM_ID, | ||
app_identifier: MAIN_BUNDLE_IDENTIFIERS | ||
team_id: TEAM_ID_PRODUCTION, | ||
app_identifier: PRODUCTION_BUNDLE_IDENTIFIERS | ||
) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These last calls come from register_new_device
, a lane that adds devices to the ASC account. We've long since improved this workflow via internal tooling. @Automattic/pocket-casts-ios is it okay to remove register_new_device
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This obsolete lane also has the big drawback that any device registered using it would not be claimed and associated with the Matticspace user the device belongs to (and thus would be removed like any other unclaimed device when we do our yearly cleanup too). So 💯 to remove that lane.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in #1758
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, except the constant at the global scope having side effects (potentially making the Fastfile
crash if the env vars are not defined locally… even when running a lane that don't need to interact with the ASC API and won't need that API key at all)
team_id: TEAM_ID, | ||
app_identifier: MAIN_BUNDLE_IDENTIFIERS | ||
team_id: TEAM_ID_PRODUCTION, | ||
app_identifier: PRODUCTION_BUNDLE_IDENTIFIERS | ||
) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This obsolete lane also has the big drawback that any device registered using it would not be claimed and associated with the Matticspace user the device belongs to (and thus would be removed like any other unclaimed device when we do our yearly cleanup too). So 💯 to remove that lane.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test to confirm but I'm assuming you updated the env vars in .mobile-secrets
too; if so,
Yes I did. But it doesn't hurt to test in the real world. I should have thought of that. See https://buildkite.com/automattic/pocket-casts-ios/builds/6900#018f7b31-f65d-437c-91bf-b214ed8101c3 |
Breadcrumb for future reference. I just realized that CI is fine because it has the env vars, but a developer machine might not. I'll followup tomorrow with a PR that sets up the |
I'm looking at setting up prototype builds, which we'll use for internal testing and distribute via Automattic's Enterprise account. Internal ref pdnsEh-1D4-p2.
This PR does some refactors that aims to make it easier to add support for this upcoming new build.
Highlights:
api_key
instead ofapi_key_path
.To test
If CI is green, then the refactor is correct.
Checklist
CHANGELOG.md
if necessary. — N.A.