-
Notifications
You must be signed in to change notification settings - Fork 338
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
Use RC API key for local development from local.xcconfig #4795
Conversation
…cal.xcconfig file
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.
Yessss omg thank you! This was one of my main motivations for adding the Local.xcconfig
file, but I hadn't gotten around to it yet. ❤️
I have some suggestions to take it even further haha.
@@ -38,7 +38,6 @@ verify_no_included_apikeys() { | |||
"${SCRIPT_DIR}/../Examples/MagicWeather/MagicWeather/Constants.swift" | |||
"${SCRIPT_DIR}/../Examples/MagicWeatherSwiftUI/Shared/Constants.swift" | |||
"${SCRIPT_DIR}/../Tests/TestingApps/PurchaseTesterSwiftUI/Core/Constants.swift" | |||
"${SCRIPT_DIR}/../Tests/TestingApps/PaywallsTester/PaywallsTester/Config/LocalConfigItems.swift" |
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.
Can we not get rid of all of these files, so we can get rid of the entire verify_no_included_apikeys()
check?
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.
Well I see the utility of having then in the sense that someone might forget about the local.xcconfig
file thing and perhaps wants to do a quick change of the API key and they do it in the code directly. So it does not harm. That said, by that argument, I'd need to add the ConfigItem.swift
file to it 😄
// DO NOT MODIFY THIS FILE. | ||
// CI system adds the API key here. |
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.
What do you think about removing this file and making the CI script create a Local.xcconfig
file instead?
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.
Well, this file is still needed for the PaywallsTester app. I mean, not needed per se, but it's good to have a centralized place where we get the necessary variables from the info.plist.
Regarding having the CI script create a Local.xcconfig, I think that could totally work, yes. I'll open a separate PR for the CI changes related to this one 👍
@@ -1,31 +0,0 @@ | |||
// |
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 comment has nothing to do with the line it's attached to 😅)
We also have a :replace_api_key_integration_tests
Fastlane lane. We could refactor that to write values to Local.xcconfig
as well, instead of to the various files listed in that lane. Just a suggestion and definitely not a blocker for this PR, as that is not really a pain point for local development.
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 see. But it would definitely be much cleaner. I'll do this in a separate PR for CI-related stuff. Thank you for the suggestions!!!
YESSS |
Related PR, but for Examples apps --> #4796 |
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 is great! 🫶
struct ConfigItem: AvailableConfigItems { | ||
// DO NOT MODIFY THIS FILE. | ||
// CI system adds the API key here. | ||
struct ConfigItem { |
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.
nit
struct ConfigItem { | |
enum ConfigItem { |
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.
lovely
Motivation
When using the testing apps for developing the SDK, we usually need to add our own RevenueCat API key and/or Proxy URL. However, before this PR:
Description
This PR addresses the two pain points mentioned above when setting an API key for local development (or Proxy URL):
local.xcconfig
filelocal.xcconfig
file is ignored by git in this repo, we can finally stop worrying about committing the API key by mistakeNotes
apiKey
String
inConfigItem.swift