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

feat(crashlytics ios): put input files when pod install. #4520

Merged
merged 2 commits into from
Nov 10, 2020

Conversation

tihimsm
Copy link
Contributor

@tihimsm tihimsm commented Nov 9, 2020

Description

I commit to this repository for the first time.
I'm sorry if there is a wrong way.

Put the Build Phase setting of iOS when pod install.
https://firebase.google.com/docs/crashlytics/get-started?hl=ja#initialize-crashlytics

I want to add Input Files.
${DWARF_DSYM_FOLDER_PATH}/${DWARF_DSYM_FILE_NAME}/Contents/Resources/DWARF/${TARGET_NAME}
$(SRCROOT)/$(BUILT_PRODUCTS_DIR)/$(INFOPLIST_PATH)

Related issues

Discussion #4519

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@vercel
Copy link

vercel bot commented Nov 9, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/7iz5xrdjk
✅ Preview: https://react-native-firebase-git-feat-crashlytics-add-input-files.invertase.vercel.app

@CLAassistant
Copy link

CLAassistant commented Nov 9, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Nov 9, 2020

Codecov Report

Merging #4520 (7ca3861) into master (7792f2a) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4520   +/-   ##
=======================================
  Coverage   41.92%   41.92%           
=======================================
  Files          35       35           
  Lines        1119     1119           
  Branches      278      278           
=======================================
  Hits          469      469           
  Misses        489      489           
  Partials      161      161           

@mikehardy
Copy link
Collaborator

The fork workflow is just fine - I'll look this over, thanks for taking the time to make a PR! 👏

@mikehardy mikehardy added the Workflow: Needs Review Pending feedback or review from a maintainer. label Nov 9, 2020
@mikehardy
Copy link
Collaborator

Hey there - thanks again for the PR

A quick question: is this backwards-compatible? If we merge it will it change the behavior for existing users in any way?

Also, the CLA must be signed before we can merge (just click the details link next to the CLA check in the list of checks that run and it will take you through it)

@tihimsm
Copy link
Contributor Author

tihimsm commented Nov 10, 2020

@mikehardy

Hi, thanks for your comments!

Also, the CLA must be signed before we can merge (just click the details link next to the CLA check in the list of checks that run and it will take you through it)

I agreed with CLA!

A quick question: is this backwards-compatible? If we merge it will it change the behavior for existing users in any way?

Before: When Pod install, Does not include Input Files.
スクリーンショット 2020-11-10 14 45 39

After: When Pod install, Automatically include Input Files.
スクリーンショット 2020-11-10 14 45 22

I made it exactly as described in the Firebase documentation.
https://firebase.google.com/docs/crashlytics/get-started?hl=ja#initialize-crashlytics

I can add it manually on Xcode, but under the current situation, xcodeproj is rewritten by pod install when building with CI etc.
So I submitted this PR.

Thanks.

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM - thank you!

@mikehardy mikehardy merged commit f2161fd into invertase:master Nov 10, 2020
@mikehardy mikehardy removed the Workflow: Needs Review Pending feedback or review from a maintainer. label Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants