Skip to content
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

Fix installable build #1592

Closed
wants to merge 3 commits into from
Closed

Fix installable build #1592

wants to merge 3 commits into from

Conversation

crazytonyli
Copy link
Contributor

@crazytonyli crazytonyli commented May 14, 2024

Set MATCH_CERTIFICATE_ID env var so that fastlane match can install the right certificate for us. See the code comments for details.

@dangermattic
Copy link
Collaborator

dangermattic commented May 14, 2024

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented May 14, 2024

You can test the changes in simplenote-ios from this Pull Request by:

  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr1592-625a3dd-018f83c4-6049-4257-a66c-73d15f69833e on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@crazytonyli crazytonyli requested a review from a team May 15, 2024 22:11
@crazytonyli crazytonyli marked this pull request as ready for review May 15, 2024 22:11
@crazytonyli crazytonyli added the tooling Related to anything that supports the building & maintaining of the project. label May 15, 2024
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

🎗️ Not related to this PR so could be addressed separately (just thought of it since this PR is about Prototype Builds) but would be nice to use our release-toolkit's prototype_build_details_comment action to generate the PR comment (which is formatted nicer and solves the issue with QR code image)

Comment on lines +1012 to +1013
# At the moment, we have two certificates for in-house distribution in the fastlane
# match repo. `fastlane match` will only install one of them (See [1]), which may not be
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for us having two certificates in the match repo currently? Maybe as we go through transitioning to a soon-to-be-expired one to a new further-expiration-date one?

I think I remember an issue or even a PR in fastlane exactly about that though (making fastlane always use the latest cert instead of just the first one) 🤔

Which, if that's the root cause for this issue in this repo and what this workaround aims to solve here, would be a better solution long term, and if so, worth taking note of them in that comment so that we can adopt the better solution once fixed in fastlane itself in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I remember an issue or even a PR in fastlane exactly about that though

This is the one I was thinking about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason for us having two certificates in the match repo currently? Maybe as we go through transitioning to a soon-to-be-expired one to a new further-expiration-date one?

I haven't been following certificates renewal closing, so I'm not sure why we have two.

making fastlane always use the latest cert instead of just the first one

I don't think that's a proper fix. That's no difference than picking first or last one. The correct fix should be installing the certificate that's used by the provisioning profiles. Or, just install all certificates and let Xcode figure out the rest.

Copy link
Contributor

Choose a reason for hiding this comment

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

The correct fix should be installing the certificate that's used by the provisioning profiles. Or, just install all certificates and let Xcode figure out the rest.

Good point. You might want to leave a comment in the fastlane PR I mention above about this btw, as if it gets merged and shipped then the next time we update fastlane that might break our setups if they start picking the latest? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

And also in the short term if there's no good reason for us to have two distinct certificates in the Enterprise account, I think we should remove the one that is only used my Simplenote profiles to instead only keep the cert used by all other apps and profiles, even if that means you'll need to refresh the Simplenote profiles to use the other certificate.

That way that will avoid confusion in the future (about "why do we have 2 certificates? Which one is used by what? Which one should we renew?") and ease maintenance of our Enterprise account (including freeing up a slot for helping doing certificate rotation when the main cert will be about to expire, as iirc we can only have a maximum of two distribution certificates per account at a time?)

And finally doing such a cleanup in our account will also avoid you needing that fix (and us having to update the fix when certificates are renewed)

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 thought about that, but didn't do it because S8CMX4U6QW is newer than the other certificate (UW99FF5HN9): S8CMX4U6QW expires at 2027-01-07 17:22:57 UTC and UW99FF5HN9 expires at 2025-02-22 08:40:35 UTC.

It feels strange to revoke a newer certificate and keep an older one. But I guess that doesn't really
matter in how we use them in practice. I can look into renewing profiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The in-house distribution provisioning profiles are renewed and installable build step now passes. I'll close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Related to anything that supports the building & maintaining of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants