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

Add option to ignore process.env when on CI #410

Closed
goatandsheep opened this issue Feb 8, 2023 · 10 comments
Closed

Add option to ignore process.env when on CI #410

goatandsheep opened this issue Feb 8, 2023 · 10 comments
Labels
wontfix This will not be worked on

Comments

@goatandsheep
Copy link
Owner

Originally opened by @lmcjt37

My team found during an upgrade to react native, along with many other packages, that a change occurred that caused our .env.test to be ignored in favour of the environment variables injected via the CI. This appeared to only be the case when running yarn test --coverage.

Interestingly, when changing the coverageProvider to "v8", the environment variables were correct. But that didn't solve all our issues and raised other significantly complex issues.

@github-actions
Copy link

github-actions bot commented Feb 8, 2023

Hey, thank you for opening this issue! 🙂 To boost priority on this issue and support open source please tip the team at https://issuehunt.io/r/goatandsheep/react-native-dotenv/issues/410

@goatandsheep
Copy link
Owner Author

What are you using .env.test for? Are you able to remove the environment variables from the CI settings? I don't think I'm understanding but I thought the whole thing about choosing the environment name through ENV= (or APP_ENV= if that doesn't work) is that you can choose which set of environment variables to go with, i.e. one for testing, one for CI, etc. Let me know what I'm not understanding.

Also are you running multiple tests in the same script? I wonder if this is a caching problem.

@lmcjt37
Copy link

lmcjt37 commented Feb 8, 2023

The weird thing is that before the upgrade everything was working, the upgrade introduced a lot of breaking changes.

Our .env.test file is there specifically to inject variables for unit tests, and we load the file in specifically through babel.config. We found this issue only when running yarn test --coverage, so perhaps something in the environment changes specifically for Jest (since the update) when the coverage flag is set.

In our case we saw the CI environment variables taking precedence over the loaded .env file in the above scenario which caused our tests to fail.

Additionally, I had tested setting APP_ENV which didn't make a difference, and could see logging showing me the expected steps in your script were running through.

@goatandsheep
Copy link
Owner Author

What happens if you delete the CI environment variables? Are you able to do that without messing up the builds?

@lmcjt37
Copy link

lmcjt37 commented Feb 8, 2023

We aren't able to remove the variables being overridden. These values are secrets hence the .env.test to replace just the necessary ones.

But, what you're asking about removing the value from the CI. That will allow the .env.<ENVIRONMENT> to work as expected. This is why I introduced the additional argument as it seemed a less invasive approach. If it's not used then everything carries on working as is.

@goatandsheep
Copy link
Owner Author

I think there's already a way to make sure that we don't import the local variables. Let me check

@goatandsheep
Copy link
Owner Author

Can you set the necessary variables to undefined in your babel.config.js? This will clear the values in process.env

@goatandsheep
Copy link
Owner Author

@lmcjt37 I am hesitant to add this not only because it encourages a bad practice but because your context implies you're likely using this library in a dangerous way. You should have .env.test in your .gitignore unless it is very generic settings. You should be setting your environment variables through the CI portal. If you know of a case where this happened because of unnecessary system environment variables taking priority, then we should collaborate on toggling system environment variables, but not for this context. This library is meant to emulate dotenv-flow for babel-specific projects and this is outside of that scope.

@lmcjt37
Copy link

lmcjt37 commented Feb 28, 2023

I'm happy to close this.

We have since refactored our pipeline and context variables. So it's likely this isn't an issue any longer.

I will retest without our patch to confirm.

@stale
Copy link

stale bot commented May 2, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label May 2, 2023
@stale stale bot closed this as completed May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants