Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Integrate Telemetry SDK as framework dependency #60

Merged
merged 21 commits into from
Nov 25, 2019

Conversation

julianrex
Copy link
Contributor

@julianrex julianrex commented Nov 12, 2019

Test integration of Mapbox Mobile Events as a dependency.

This PR adds mapbox-events-ios as a separate project to the Xcode workspace, and links with the MapboxMobileEvents.framework:

  • CocoaPods: no change (events is specified as a dependency in the Podspec)
  • Carthage: as an interim measure, please specify github "mapbox/mapbox-events-ios" == 0.10.1-alpha in your Cartfile. It is still TBD how the Carthage dependency will be specified.
  • Manual installation: New Github release artifact (mapbox-ios-sdk-5.6.0-alpha.1-dynamic-with-events.zip) contains the MapboxMobileEvents.framework; please install this along with Mapbox.framework

This PR also bumps the release version to 5.6.0-alpha.1. As with all alphas and betas, it is not recommended for production use.


OP:

Required for alpha.1:

  • Fix CI failures
  • @rclee review changes in mapbox-events-ios
  • Update MGLMapboxEvents with @rclee's help
  • Update packaging script
  • Update CI to build/exercise new target

Optional for alpha.1:

  • Review decision to link statically with events for SDK's Github release
    • Decision to avoid linking statically with the events library for the immediate release (the current static framework currently needs work regardless)
  • Add tests - this should be a separate PR that we can land independently of this PR. Check behavior hasn't changed.
  • Document install process if necessary (e.g. if Github release requires 2 frameworks instead of 1)

@julianrex
Copy link
Contributor Author

This PR bumps the no-content timeout to 20mins (up from 5 minutes) - we need to investigate this as tail work.

@julianrex
Copy link
Contributor Author

julianrex commented Nov 22, 2019

@julianrex julianrex force-pushed the jrex-telemetry-integration branch from 20d2180 to 66daa95 Compare November 23, 2019 13:42
@julianrex julianrex marked this pull request as ready for review November 23, 2019 14:12
@julianrex julianrex requested a review from 1ec5 as a code owner November 23, 2019 14:12
@julianrex julianrex requested review from a team and removed request for 1ec5 and a team November 23, 2019 14:12
@julianrex julianrex requested review from fabian-guerra and removed request for jmkiley November 25, 2019 14:07
@julianrex julianrex added this to the Release tequila milestone Nov 25, 2019
Copy link
Contributor

@nishant-karajgikar nishant-karajgikar left a comment

Choose a reason for hiding this comment

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

looks good to me!

Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

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

LGTM. Only two minor comments. Thank you!

@@ -87,7 +87,7 @@
</MacroExpansion>
</LaunchAction>
<ProfileAction
buildConfiguration = "Release"
buildConfiguration = "RelWithDebInfo"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this changing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a hold-over from previous testing - however, it's easier to get meaningful results when profiling with symbols.

@@ -4,7 +4,8 @@
#import "NSBundle+MGLAdditions.h"

#if TARGET_OS_IPHONE || TARGET_OS_SIMULATOR
#import "MMEEventsManager.h"
#import <MapboxMobileEvents/MapboxMobileEvents.h>
//#import "MMEEventsManager.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please remove unused code

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

Successfully merging this pull request may close these issues.

4 participants