-
Notifications
You must be signed in to change notification settings - Fork 328
fix(core): improper reading of .testcontainers.properties #863
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
Conversation
see if you can make the tests pass? i can take a look to see if i can finish this pr when i am available |
The environment variables were not overridden from the .testcontainers.properties file for ryuk variables. This causes the properties file to never actually be used. This commit detects the environment variable, and if unspecified falls back to the properties file, and if not specifed, defaults to false
@alexanderankin I fixed it by adding a Optional[bool] defaulted to None, which allows an override for ryuk_disabled and ryuk_privileged programmatically. Sorry for not reading the CONTRIBUTING.md. I didn't see it in the top level directory and didn't see the README.md 😅 I'm on a mac, so the test is set to skipped, but I disabled that and it seems to have passed. I can't be too sure until I see the tests here though. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #863 +/- ##
=======================================
Coverage ? 82.09%
=======================================
Files ? 14
Lines ? 916
Branches ? 148
=======================================
Hits ? 752
Misses ? 128
Partials ? 36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Removed unused method get_bool_env, as it was replaced with _render_bool Added more tests for proper test coverage for loading variables via testcontainers.properties file
@alexanderankin Can I get one more run please? I have added some extra test cases and caught some errors myself :) |
@alexanderankin It seems like the docs build failed due to a connection failure? Mind triggering again? |
can you tldr what this code does? im on a work trip right now and just dont have time this week to look at this - i would merge otherwise |
TLDR: Originally when running test containers, you can set variables inside of What was happening originally was that the Function causing this issue:
As you can see this would return either True or False. What my change does, is bring this function into the class, and when attempting to access the disabled or privileged class value, it calculates it based on if it was set previously (via code, env, or properties file), and if not then checks the environment, then checks the properties file, then defaults to False Also, I totally understand being busy! I just want to say thank you so much for providing this package from your own time and energy :) |
fix #864
The environment variables were not overridden from the .testcontainers.properties file for ryuk variables. This causes the properties file to never actually be used. This commit detects the environment variable, and if unspecified falls back to the properties file, and if not specified, defaults to false