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.

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

Jump to bottom

Enable watchOS for Remote Config and ABTesting #7481

Merged
merged 11 commits into from
Feb 12, 2021
Merged

Enable watchOS for Remote Config and ABTesting #7481

merged 11 commits into from
Feb 12, 2021

Conversation

charlotteliang
Copy link
Contributor

@charlotteliang charlotteliang commented Feb 8, 2021 •
edited
Loading

ABTesting is a hard dependency so enable it as well.
Testing: I'm able to get config value without any changes using the watchOS sample app.
@google-cla google-cla bot added the cla: yes label Feb 8, 2021
paulb777
paulb777 reviewed Feb 8, 2021
View reviewed changes
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Thanks!

Please update CI with a build only pod lib lint at https://github.com/firebase/firebase-ios-sdk/blob/master/.github/workflows/remoteconfig.yml#L52
@google-oss-bot
Copy link

google-oss-bot commented Feb 8, 2021 •
edited
Loading

Coverage Report

Affected SDKs

  • FirebaseABTesting-iOS-FirebaseABTesting.framework

    SDK overall coverage changed from ? (9fadbaf) to 91.75% (85e43f9) by ?.

    Filename Base (9fadbaf) Head (85e43f9) Diff
    ABTConditionalUserPropertyController.m ? 92.73% ?
    ABTExperimentPayload.m ? 100.00% ?
    FIRExperimentController.m ? 85.83% ?
    FIRLifecycleEvents.m ? 100.00% ?

  • FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework

    SDK overall coverage changed from ? (9fadbaf) to 80.97% (85e43f9) by ?.

    Click to show coverage changes in 11 files.
    Filename Base (9fadbaf) Head (85e43f9) Diff
    FIRConfigValue.m ? 58.70% ?
    FIRRemoteConfig.m ? 84.07% ?
    FIRRemoteConfigComponent.m ? 96.05% ?
    RCNConfigContent.m ? 80.34% ?
    RCNConfigDBManager.m ? 77.94% ?
    RCNConfigExperiment.m ? 92.91% ?
    RCNConfigFetch.m ? 80.85% ?
    RCNConfigSettings.m ? 70.24% ?
    RCNDevice.m ? 86.15% ?
    RCNPersonalization.m ? 91.30% ?
    RCNUserDefaultsManager.m ? 97.99% ?

Test Logs
paulb777
paulb777 reviewed Feb 8, 2021
View reviewed changes
README.md Show resolved Hide resolved
ryanwilson
ryanwilson reviewed Feb 9, 2021
View reviewed changes
Copy link
Member

@ryanwilson ryanwilson left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM, please wait for @karenyz approval.
Example/watchOSSample/SampleWatchAppWatchKitExtension/ExtensionDelegate.swift Outdated Show resolved Hide resolved
paulb777
paulb777 approved these changes Feb 9, 2021
View reviewed changes
FirebaseABTesting.podspec Outdated Show resolved Hide resolved
@karenyz
Copy link
Contributor

karenyz commented Feb 9, 2021

Thanks for testing this! 🎉
@charlotteliang
Copy link
Contributor Author

my clang-format somehow is not in sync even tho it's the same version.
@paulb777
Copy link
Member

The log might be confusing, since it's actually swiftformat that checks the format of Swift files - not clang-format
@charlotteliang
Copy link
Contributor Author

Right, I also upgraded swiftformat and I ran both swiftformat and ./script/style.sh. None of them is accepted by CI.
@paulb777
Copy link
Member

swiftformat should be controlled by the Mintfile and running 0.45.5 both locally and in CI
@charlotteliang charlotteliang merged commit 80a4512 into master Feb 12, 2021
@charlotteliang charlotteliang deleted the chen-rc-watch branch February 12, 2021 19:41
@chwo chwo mentioned this pull request Feb 24, 2021
Closed
@firebase firebase locked and limited conversation to collaborators Mar 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: remoteconfig cla: yes
5 participants
@charlotteliang @google-oss-bot @karenyz @paulb777 @ryanwilson