Skip to content

Enable ABT to be a soft dep of RC #6379

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
Closed
wants to merge 10 commits into from
Closed

Enable ABT to be a soft dep of RC #6379

wants to merge 10 commits into from

Conversation

@paulb777
Copy link
Member

@paulb777 paulb777 commented Aug 31, 2020 •
edited
Loading

Fix one of the issues raised in #6345.

  • Propose a new location for Interop headers in the one version world.
  • Add two Interop APIs for the RC -> ABT interface
  • Tested with ABTesting removed from the RC podspec, but added it back until 7.0
  • Added a separate unit test target for combined RC and ABT testing
  • Will wait for M79 code freeze branch before merging.

#no-changelog

@paulb777 paulb777 marked this pull request as draft August 31, 2020 22:22
@paulb777 paulb777 marked this pull request as ready for review September 1, 2020 01:07
@christibbs
Copy link
Contributor

christibbs commented Sep 1, 2020 •
edited
Loading

@paulb777 Will the end result be:

  1. Pulling in Remote Config will pull in the ABT dependency by default, but developers can remove it, or...
  2. Pulling in Remote Config will not pull in the ABT dependency, but developers can add it?

@paulb777
Copy link
Member Author

paulb777 commented Sep 1, 2020 •
edited
Loading

@christibbs It will be Number 2, but not until the Firebase 7.0.0 release to avoid an earlier breaking change.

morganchen12
morganchen12 reviewed Sep 1, 2020
View reviewed changes
@paulb777 paulb777 added this to the 6.33.0 - М80 milestone Sep 1, 2020
@maksymmalyhin maksymmalyhin mentioned this pull request Sep 3, 2020
Merged
ryanwilson
ryanwilson reviewed Sep 10, 2020
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.

Overall LGTM, a few small things

FirebaseABTesting/Sources/Interop/FIRABTInterop.h Outdated
Comment on lines 24 to 27
/// Updates the list of experiments. Experiments already
/// existing in payloads are not affected, whose state and payload is preserved. This method
/// compares whether the experiments have changed or not by their variant ID. This runs in a
/// background queue..
Copy link
Member

@ryanwilson ryanwilson Sep 10, 2020

Choose a reason for hiding this comment

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

Nit: the first line can fit more words, also double periods at the end of the paragraph.

Copy link
Member Author

@paulb777 paulb777 Sep 10, 2020

Choose a reason for hiding this comment

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

Done. Here and in the public header this was copied from.

FirebaseRemoteConfig.podspec Outdated
]
abt.requires_app_host = true
abt.dependency 'OCMock'
abt.dependency 'FirebaseABTesting', '~> 4.2'
Copy link
Member

@ryanwilson ryanwilson Sep 10, 2020

Choose a reason for hiding this comment

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

Should this be 4.3 now instead of 4.2?

Copy link
Member Author

@paulb777 paulb777 Sep 10, 2020

Choose a reason for hiding this comment

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

Yes. Good catch.

FirebaseRemoteConfig/Sources/FIRRemoteConfig.m Outdated
#import "FirebaseRemoteConfig/Sources/RCNConfigValue_Internal.h"
#import "FirebaseRemoteConfig/Sources/RCNDevice.h"

#import "FirebaseABTesting/Sources/Interop/FIRABTInterop.h"
Copy link
Member

@ryanwilson ryanwilson Sep 10, 2020

Choose a reason for hiding this comment

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

Nit: Should this not stay at the top where it was before?

Copy link
Member Author

@paulb777 paulb777 Sep 10, 2020

Choose a reason for hiding this comment

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

Done.

FirebaseABTesting/Sources/FIRExperimentController.m
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
[self updateExperimentsWithServiceOrigin:origin
events:lifecycleEvent
policy:ABTExperimentPayloadExperimentOverflowPolicyDiscardOldest
Copy link
Member

@ryanwilson ryanwilson Sep 10, 2020

Choose a reason for hiding this comment

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

Will this always be the case for interop, or do we want it to be a parameter as well?

Copy link
Member Author

@paulb777 paulb777 Sep 10, 2020

Choose a reason for hiding this comment

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

Perhaps we want it to be the default for now? If we need to support other policies, we could add an API and expose the policies via the interop as well.

Copy link
Member

@ryanwilson ryanwilson Sep 10, 2020

Choose a reason for hiding this comment

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

SGTM, thanks

ryanwilson
ryanwilson approved these changes Sep 10, 2020
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.

LGTM assuming product owners are good with this (the fact that ABT may not be there for all RC users)

@paulb777
Copy link
Member Author

paulb777 commented Sep 10, 2020

@christibbs Would you confirm this is ok from an ABT product perspective that the fact that ABT users will need to explicitly include the pod?

@christibbs
Copy link
Contributor

christibbs commented Sep 14, 2020

@christibbs Would you confirm this is ok from an ABT product perspective that the fact that ABT users will need to explicitly include the pod?

Will confirm with the team. At the very least it'll require some extensive documentation updates.

Also concerned about the case where developers pod update and miss the message that Firebase/ABTesting is removed. Or will it be removed in that case?

@maksymmalyhin maksymmalyhin removed this from the 6.33.0 - М80 milestone Sep 16, 2020
@paulb777
Copy link
Member Author

paulb777 commented Sep 17, 2020

@christibbs Why "extensive" documentation updates? Isn't it one-line to say to add pod 'Firebase/ABTesting' to the Podfille?

@paulb777
Copy link
Member Author

paulb777 commented Sep 23, 2020

Closing since it is a product requirement for ABT to be a hard dependency of RC so that RC developers have the flexibility to add in ABT in the future.

@paulb777 paulb777 closed this Sep 23, 2020
@paulb777 paulb777 deleted the pb-rc-abt-soft branch September 23, 2020 22:16
@firebase firebase locked and limited conversation to collaborators Oct 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@morganchen12 morganchen12 morganchen12 left review comments

@ryanwilson ryanwilson ryanwilson approved these changes

@christibbs christibbs Awaiting requested review from christibbs

@karenyz karenyz Awaiting requested review from karenyz

Assignees

@christibbs christibbs

Labels

api: remoteconfig cla: yes public-api-change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@paulb777 @christibbs @ryanwilson @morganchen12 @maksymmalyhin @googlebot @google-oss-bot