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

[WEB-740] Migrates Segment and AppboySegment to SPM #1732

Closed
wants to merge 2 commits into from
Closed

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Sep 19, 2022

πŸ“² What

As part of our Carthage -> SPM migration we need to migrate Segment & AppboySegment

πŸ€” Why

Checkout this Guru Card for more details

πŸ›  How

  • Remove the dependencies from Cartfile and Cartfile.resolved and add them to Package.resolved
  • Update Segment Configuration in AppDelegate based on current AppboySegment version.
        self.viewModel.outputs.configureSegment
          .observeValues { writeKey in
             let config = AnalyticsConfiguration(writeKey: writeKey)
             config.use(SEGAppboyIntegrationFactory.instance())

             Analytics.setup(with: config)

             let client = Analytics.configuredClient(withWriteKey: writeKey)
             AppEnvironment.current.ksrAnalytics.configureSegmentClient(client)
        }

Fore more details on this code checkout their repo

  • Segment version to 4.1.2 AppboySegment version to 4.5.0
  • Resolve any merge conflicts

βœ… Acceptance criteria

  • Breakpoint most instances of code used in files that import Segment & import AppboySegment and run the app on device to check the breakpoints are hit and the app continues to run as expected. This package is basically just used in test cases FYI.
  • Run/build app, smoke test, and verify that all tests pass.

@scottkicks scottkicks added this to the release-5.5.0 milestone Sep 19, 2022
@scottkicks scottkicks self-assigned this Sep 19, 2022
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #1732 (015b939) into main (6c367db) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1732   +/-   ##
=======================================
  Coverage   85.35%   85.36%           
=======================================
  Files        1270     1270           
  Lines      114842   114842           
  Branches    30335    30335           
=======================================
+ Hits        98028    98030    +2     
+ Misses      15762    15760    -2     
  Partials     1052     1052           
Impacted Files Coverage Ξ”
Library/Tracking/Segment.swift 0.00% <ΓΈ> (ΓΈ)
Library/Navigation.swift 85.85% <0.00%> (+0.64%) ⬆️

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kickstarter kickstarter deleted a comment from nativeksr Sep 20, 2022
@scottkicks scottkicks marked this pull request as ready for review September 20, 2022 15:07
@msadoon
Copy link
Contributor

msadoon commented Sep 20, 2022

Hey @scottkicks, redid this one, just because there was code changes we didn't really need to do if we stuck to version 4.0.0 (recommended), and some post-build actions that were missed in the git SPM instructions. Also we didn't need to import Segment in Kickstarter-iOS app target and Library-iOS, just AppboySegment in those frameworks.

It was hard to verify these changes because I have to go through the steps myself in order to understand the required changes unless they are straight forward.

But I was able to get this working with minimal code changes (just the imports), so if its cool with you can we merge it instead and close this one?

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Yea so close this one as there is duplicate code and a threading issue addressed in the new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants