-
-
Notifications
You must be signed in to change notification settings - Fork 29
Upgrade Google SignIn SDK from v8 to v9 #106
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
base: main
Are you sure you want to change the base?
Upgrade Google SignIn SDK from v8 to v9 #106
Conversation
…0.0, and enhance documentation
…tation for Google SignIn sample + remove Firebase dependencies from Sample project + fix warnings in Sample project
…OS simulator settings in project files
… CI and publish workflows
Add CI workflows and update Google.SignIn version
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 620bc93577
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <key>CLIENT_ID</key> | ||
| <string>YOUR_CLIENT_ID.apps.googleusercontent.com</string> | ||
| <key>REVERSED_CLIENT_ID</key> | ||
| <string>com.googleusercontent.apps.YOUR_REVERSED_CLIENT_ID</string> |
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.
Ensure URL scheme matches the templated client ID
The new template encourages users to supply their own CLIENT_ID/REVERSED_CLIENT_ID, but the sample’s Info.plist still hardcodes the old reversed client ID URL scheme. If someone follows the template and only drops in their own plist, the OAuth callback scheme won’t match and the sign-in flow can’t return to the app, so sign‑in will fail. Consider templating/placeholdering the Info.plist scheme too or explicitly documenting that it must be updated alongside the plist.
Useful? React with 👍 / 👎.
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.
Indeed, I didn't realize the sample Info.plist still hardcodes one:
https://github.com/Kapusch/GoogleApisForiOSComponents/blob/620bc9357713121b8e52679bdb67beb6725d6714/samples/Google/SignIn/SignInExample/Info.plist#L36-L39
It should be cleared.
|
Well done. Aside from codex's minor catch, my only comment is that we shouldn't be using the squiggly arrow in
Unless you have a good reason otherwise. Lucky that this was a relatively straightforward upgrade without shifting resources or compiler flags. And it looks like the header diffs were for types that aren't currently bound anyway. I'd be interested to hear how your experience was diving into this; if you got stuck on anything that could be better documented for future new contributors. And to what degree you were able to leverage any LLM(s). |
All this was relatively new to me, and I was like "god I'm going to break everything" 😄 Also, I was bit afraid to maintain this fork and deviate too much from the upstream. So what I did is preparing my plan with ChatGPT, with extending research into the existing repos, and refining it to my expectations. Then, I had one session with Codex VS Code extension to get this upgrade from v8 to v9 and that helped me to learn better what was going on under the hood and to be more confident to not break everything. I generated some docs to help new contributors and also to make it "ai-ready" for development assisted with AI. But maybe we should add a document
The squiggly arrow was proposed by Codex. I wasn't much aware about how dependencies work with cake and I learnt that this was useful to get automatic patches resolved under that major and minor version. So I thought it was good enhancement for developers and to prevent maintainers to bump the versions ourselves in case of required security patch. Now I have some questions which I need to clarify with myself: since my initial feature branch got integrated to my main branch in the fork repo, should I proceed this way:
Thanks for your review. |
This pull request introduces major improvements to the Google Sign-In binding for iOS, including an upgrade to the latest native SDKs, modernization of the sample project, and the addition of CI/CD workflows and documentation for building and publishing. The changes ensure compatibility with recent .NET and native iOS SDKs, remove hardcoded secrets from samples, and streamline local development and package publishing.
SDK and Dependency Upgrades:
Google.SignInbinding to version 9.0.0.0 and updated its native Pod dependencies toGoogleSignIn~> 9.0.0,AppAuth2.0.0, andGTMAppAuth5.0.0for compatibility with the latest iOS SDKs. [1] [2]Sample Project Modernization & Security:
SignInExamplesample to use the new Google Sign-In API (v9), including changes to initialization, sign-in, sign-out, and disconnect flows, and improved error handling and nullability. [1] [2] [3] [4]GoogleService-Info.plistcontaining secrets and replaced it with aGoogleService-Info.plist.templatefor safer configuration. Updated the project to only include the plist if it exists locally. [1] [2] [3]Build System and CI/CD:
.config/dotnet-tools.jsonto manage local .NET tools, including Cake for builds.Documentation:
These changes collectively modernize the codebase, improve security and maintainability, and make it easier for contributors to build, test, and publish updated bindings.