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

fix: adjust mapbox access token usage #1150

Merged
merged 13 commits into from
Mar 13, 2024
Merged

fix: adjust mapbox access token usage #1150

merged 13 commits into from
Mar 13, 2024

Conversation

achou11
Copy link
Member

@achou11 achou11 commented Mar 11, 2024

  • Uses react-native-config to read a .env file containing the token of interest
  • Updated instruction in contributing.md to reflect use of env file
  • Updated bitrise yml to create env file using the mapbox access token which is saved as a secrete key
  • Updated Cirrus yml with encrypted mapbox token, and a script to create an env file with the encrypted token

@achou11 achou11 requested a review from ErikSin March 11, 2024 19:13
Comment on lines +234 to +235
// React Native Config
resValue "string", "build_config_package", "com.mapeo"
Copy link
Member Author

Choose a reason for hiding this comment

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

following the step described here: https://github.com/lugg/react-native-config?tab=readme-ov-file#advanced-android-setup

Hoping I read it correctly and did the right thing, but not entirely sure tbh. Figured it was a relevant step because we do specify different suffixes depending on the build and command

Copy link
Contributor

Choose a reason for hiding this comment

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

this step i skipped when I tried this library earlier. So this must be necesssary...

@ErikSin ErikSin marked this pull request as ready for review March 11, 2024 19:51
@achou11 achou11 changed the title WIP: use react-native-config for handling tokens WIP: fix: adjust mapbox access token usage Mar 11, 2024
@achou11 achou11 changed the title WIP: fix: adjust mapbox access token usage fix: adjust mapbox access token usage Mar 11, 2024
@ErikSin
Copy link
Contributor

ErikSin commented Mar 12, 2024

I did a manual build in bitrise to test the use of the secret key and everything seems to be working: https://app.bitrise.io/build/5ee6fbee-1cb2-4924-9eaa-8b06619035c4?tab=artifacts

@ErikSin
Copy link
Contributor

ErikSin commented Mar 12, 2024

@gmaclennan Im currently using a token from my personal account. Were you thinking that we just use a free token from the Digidem account, or did you have something else in mind?

@ErikSin
Copy link
Contributor

ErikSin commented Mar 13, 2024

Updated the token (to the digidem account token) in bitrise, and the reference to the encrypted token in the cirrus.yml

@gmaclennan
Copy link
Member

FYI process.env doesn't really exist in the react native runtime

@achou11
Copy link
Member Author

achou11 commented Mar 13, 2024

FYI process.env doesn't really exist in the react native runtime

this was done at my suggestion to make it slightly easier for local development by taking advantage of our usage of https://babeljs.io/docs/babel-plugin-transform-inline-environment-variables (being used for feature flagging). can revert it and instead update the docs to say that an env file is now required if you think that's a better way to go

@gmaclennan
Copy link
Member

Ah great, didn't realise we used that in comapeo. We could have skipped adding react-native-config 😀

@ErikSin
Copy link
Contributor

ErikSin commented Mar 13, 2024

Ah great, didn't realise we used that in comapeo. We could have skipped adding react-native-config 😀

I don't think its working for .env files. It seems like it is only picking up shell env variables. So react-native-config is still needed. There was some work we could have done to update babel and make it work, but that lead to a whole slew of other updating issues

@achou11
Copy link
Member Author

achou11 commented Mar 13, 2024

Ah great, didn't realise we used that in comapeo. We could have skipped adding react-native-config 😀

yeah unfortunately it would be a little inconvenient to only rely on this for local development as you have to specify/export the environment variable in the shell for the command that eventually runs react-native bundle, so we'd still probably need some kind of env file to work with anyways, unless we hardcoded the access token in a few places 😄

@achou11
Copy link
Member Author

achou11 commented Mar 13, 2024

decided to revert the change introducing process.env usage and just make it required to set up a .env file for local development. i updated the contributing docs to be a little clearer about what's needed for that part

@achou11 achou11 merged commit eb7c531 into develop Mar 13, 2024
4 checks passed
@achou11 achou11 deleted the ac/react-native-config branch March 13, 2024 20:22
@achou11 achou11 restored the ac/react-native-config branch March 13, 2024 20:22
@achou11 achou11 deleted the ac/react-native-config branch March 13, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants