Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

Adding Firebase crashlytics #1104

Merged
merged 1 commit into from
Jul 9, 2019

Conversation

langsmith
Copy link
Contributor

Resolves #625

@langsmith langsmith self-assigned this Jun 13, 2019
@langsmith langsmith force-pushed the ls-firebase-crashlytics-implementation branch from 797c025 to af86cdc Compare June 13, 2019 05:29
@langsmith langsmith force-pushed the ls-firebase-crashlytics-implementation branch 3 times, most recently from 4e48734 to d67f773 Compare June 25, 2019 20:23
@langsmith langsmith force-pushed the ls-firebase-crashlytics-implementation branch 14 times, most recently from 069a6b2 to d775ab7 Compare July 3, 2019 23:24
@langsmith langsmith marked this pull request as ready for review July 3, 2019 23:36
@langsmith langsmith requested review from tobrun and LukasPaczos July 3, 2019 23:36
@langsmith langsmith force-pushed the ls-firebase-crashlytics-implementation branch from d775ab7 to e092ec1 Compare July 3, 2019 23:38
@langsmith langsmith force-pushed the ls-firebase-crashlytics-implementation branch 2 times, most recently from 4ed5316 to 3ae2601 Compare July 4, 2019 04:56
@langsmith langsmith force-pushed the ls-firebase-crashlytics-implementation branch 2 times, most recently from ffad6cf to b608507 Compare July 4, 2019 06:31
@langsmith langsmith force-pushed the ls-firebase-crashlytics-implementation branch 12 times, most recently from 28208ea to 112ab5e Compare July 7, 2019 16:46
@langsmith
Copy link
Contributor Author

Phew, finally ready for review when ya'll get a chance.

It's no longer in the actual pr, but during testing, I set up forcing a test crash as documented at https://firebase.google.com/docs/crashlytics/force-a-crash.

ezgif com-resize

After linking the debug version from Fabric to Firebase, the crashes started to come into the Firebase console.

Screen Shot 2019-07-03 at 1 29 48 PM

Screen Shot 2019-07-03 at 1 30 40 PM

Couple of things to know about this pr:

  1. Dependency management got kinda' wild while dealing with new/upgraded Firebase libraries. As the error messages continued one after another, it became clear that migrating to AndroidX dependencies would be the best choice for dealing with these conflicts and for the app in general moving forward. Thanks in large part to Android Studio's automatic AndroidX migration tool, this pr transitions the app to AndroidX. Most of this pr's changed classes are because of importing new AndroidX packages. Use the file filter if you want to a see a more manageable overview of this pr's changes (https://github.com/mapbox/mapbox-android-demo/pull/1104/files?file-filters%5B%5D=.gradle&file-filters%5B%5D=.yml&file-filters%5B%5D=dotfile excludes .java/.kt/XML files)
  2. The Firebase setup required me to bump the minSDK to 16. I became comfortable with this new minSDK requirement after looking at the Platform versions breakdown on https://developer.android.com/about/dashboards. I'm comfortable with excluding the .6% of devices that are pre-16. If you have any objections to bumping this, please let me know.
  3. The initializeFirebaseApp() that was in the MapboxApplication class, was removed because the initialization is now done behind the scenes by Firebase's core library.
  4. Firebase requires the Google Services plugin and google-services.json file in the MapboxAndroidDemo folder to do the crash reporting. The JSON file holds special keys, so it can't be publically committed to Github. I've added the release flavor's Firebase JSON file text as a CircleCI environment variable and created a circle.yml task to create the file during CI runs.

Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Looks great! Main request for change is to optimize circle configuration for external pull request. We don't allow using env variables for them and we shouldn't execute steps that rely on them. See https://github.com/mapbox/mapbox-gl-native/blob/master/circle.yml#L573 for an example.

circle.yml Outdated Show resolved Hide resolved
circle.yml Outdated Show resolved Hide resolved
circle.yml Outdated Show resolved Hide resolved
@langsmith
Copy link
Contributor Author

Ok @tobrun , I've addressed your feedback

Main request for change is to optimize circle configuration for external pull request. We don't allow using env variables for them and we shouldn't execute steps that rely on them. See https://github.com/mapbox/mapbox-gl-native/blob/master/circle.yml#L573 for an example.

I believe 6e313e0 correctly addresses the key/secret generation in the way you were envisioning. If not, let me know!

circle.yml Outdated Show resolved Hide resolved
circle.yml Outdated Show resolved Hide resolved
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

:shipit:

@langsmith langsmith force-pushed the ls-firebase-crashlytics-implementation branch from 231b55d to 152fea4 Compare July 9, 2019 15:04
@langsmith langsmith merged commit c0f66bd into master Jul 9, 2019
@langsmith langsmith deleted the ls-firebase-crashlytics-implementation branch July 9, 2019 15:20
@langsmith langsmith mentioned this pull request Jul 25, 2019
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch from Firebase Crash Reporting to Firebase Crashlytics
2 participants