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] [remove bundleless usge] use config instead of environment variables #320

Merged
merged 1 commit into from Nov 8, 2022

Conversation

nikophil
Copy link
Member

@nikophil nikophil commented Nov 7, 2022

fixes #311

also related to #319

I don't really know if/how we should test this:

  • unit test of DatabaseResetter (and ODM/ORM schema resetters) would be mocking hell
  • functional test would require several connections/objet managers, which really feels over-engineered...

Even testing the configuration would be tricky. I usually use a KernelBuilder for this.

WDYT?

@nikophil nikophil force-pushed the reset-configuration branch 3 times, most recently from 38a94a6 to 29b6934 Compare November 7, 2022 16:33
@nikophil nikophil mentioned this pull request Nov 7, 2022
13 tasks
@nikophil nikophil force-pushed the reset-configuration branch 3 times, most recently from 05b8a3a to b377fb3 Compare November 7, 2022 17:26
@nikophil nikophil changed the title [feat] use config to replace environment variables [feat] remove bundleless 1/2 - use config instead of environment variables Nov 7, 2022
@nikophil nikophil changed the title [feat] remove bundleless 1/2 - use config instead of environment variables [feat] [bundleless 1/2] use config instead of environment variables Nov 7, 2022
@nikophil nikophil changed the title [feat] [bundleless 1/2] use config instead of environment variables [feat] [bundleless 1/X] use config instead of environment variables Nov 7, 2022
@nikophil nikophil changed the title [feat] [bundleless 1/X] use config instead of environment variables [feat] [bundleless 1] use config instead of environment variables Nov 7, 2022
@nikophil nikophil changed the title [feat] [bundleless 1] use config instead of environment variables [feat] [remove bundleless usge] config instead of environment variables Nov 7, 2022
@nikophil nikophil force-pushed the reset-configuration branch 4 times, most recently from 664cd05 to e0af06e Compare November 7, 2022 18:29
@nikophil
Copy link
Member Author

nikophil commented Nov 7, 2022

about the test stuf, maybe a quick win would be to name our connections / managers with another name than "default" use this name in the config

@kbond
Copy link
Member

kbond commented Nov 7, 2022

If not explicitly setting the connections/managers, does it use default or run on all available connections/managers?

@nikophil
Copy link
Member Author

nikophil commented Nov 7, 2022

we're using default names: $this->registry->getDefaultConnectionName(), $this->registry->getDefaultManagerName()

@kbond
Copy link
Member

kbond commented Nov 7, 2022

Ah, ok, hah - if we add a non-default connection/manager to the config for the tests, we lose the test for choosing the defaults... Let's leave untested for now - I don't want yet another test permutation. Of course, if there's a reported issue, we can re-evaluate.

@nikophil
Copy link
Member Author

nikophil commented Nov 7, 2022

ok, hopefully we'll remove 5 or more test permutation after USE_FOUNDRY_BUNDLE usage will be removed

@nikophil nikophil force-pushed the reset-configuration branch 16 times, most recently from 42e13ea to 683eebe Compare November 8, 2022 08:06
@nikophil nikophil requested a review from kbond November 8, 2022 08:44
@nikophil nikophil marked this pull request as ready for review November 8, 2022 08:44
@nikophil nikophil changed the title [feat] [remove bundleless usge] config instead of environment variables [feat] [remove bundleless usge] use config instead of environment variables Nov 8, 2022
@nikophil nikophil merged commit e417945 into zenstruck:1.x Nov 8, 2022
@nikophil nikophil deleted the reset-configuration branch November 8, 2022 13:43
@nikophil nikophil mentioned this pull request Nov 9, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[feature] add a way to exclude database connection for mongo support on reset database
2 participants