Skip to content

feat: drop all connections before dropping db for postgres#458

Merged
nikophil merged 1 commit intozenstruck:1.xfrom
nikophil:feat/drop-connections
May 23, 2023
Merged

feat: drop all connections before dropping db for postgres#458
nikophil merged 1 commit intozenstruck:1.xfrom
nikophil:feat/drop-connections

Conversation

@nikophil
Copy link
Member

@nikophil nikophil commented Apr 28, 2023

fixes #455

we cannot test this, since we use mysql in docker images, I'll test it in a project of mine
EDIT: it does not work on my project, but I don't know why... SELECT current_database() gives me the good database name 🤷‍♂️
@benblub would you mind to test it on a project of yours? 🙏 maybe it's coming from my setup...

@nikophil nikophil force-pushed the feat/drop-connections branch 3 times, most recently from d2fdff2 to 7a3a730 Compare April 28, 2023 14:26
@benblub
Copy link
Contributor

benblub commented May 9, 2023

fixes #455

we cannot test this, since we use mysql in docker images, I'll test it in a project of mine EDIT: it does not work on my project, but I don't know why... SELECT current_database() gives me the good database name 🤷‍♂️ @benblub would you mind to test it on a project of yours? 🙏 maybe it's coming from my setup...

Yes I w'll test

private function dropAndResetDatabase(): void
{
foreach ($this->connectionsToReset() as $connection) {
if ($this->registry->getConnection($connection)->getDatabasePlatform() instanceof PostgreSQLPlatform) {
Copy link
Contributor

@benblub benblub May 10, 2023

Choose a reason for hiding this comment

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

working for me with the class PostgreSQL100Platform.

the class is deprecated but the newer class isn't working yet.

Deprecated:
This class will be merged with PostgreSQLPlatform in 4.0 because support for Postgres releases prior to 10.0 will be dropped.

Suggest: inplement with an OR clause ..

@nikophil

Copy link
Member Author

Choose a reason for hiding this comment

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

hi @benblub

thanks for having tested that!

Suggest: inplement with an OR clause ..

not sure to understand what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

if ($this->registry->getConnection($connection)->getDatabasePlatform() instanceof PostgreSQLPlatform || $this->registry->getConnection($connection)->getDatabasePlatform() instanceof PostgreSQL100Platform)

@nikophil

Copy link
Member Author

Choose a reason for hiding this comment

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

I've not done this because PostgreSQL100Platform extends PostgreSQLPlatform

Copy link
Contributor

Choose a reason for hiding this comment

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

ok then just replace PostgreSQLPlatform with PostgreSQL100Platform. In my test PostgreSQL100Platform is working and PostgreSQLPlatform not.

Copy link
Member Author

Choose a reason for hiding this comment

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

I really don't understand what's going on: https://github.com/zenstruck/foundry/actions/runs/5000161439/jobs/8957237306

I changed the CI to have only ONE job with postgres, and the db is named zenstruck_foundry_${{ matrix.use-dama }}_${{ matrix.orm-db }}, ie: zenstruck_foundry_1_postgres so it is unique in the matrix...

what I've noticed is also that the CI with postgres works without dama... 🤔

Copy link
Member

Choose a reason for hiding this comment

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

what I've noticed is also that the CI with postgres works without dama...

Ok, that was exactly the issue in #73. Worked without dama but not with.

Copy link
Member Author

Choose a reason for hiding this comment

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

I managed to make dama work with postgres in GH CI: we must add ?serverVersion=15 in the dsn

Copy link
Member

Choose a reason for hiding this comment

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

Sheesh, ok that matches the recipe

Copy link
Member Author

Choose a reason for hiding this comment

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

ok then, the CI is green with postgres as the main db engine!

this was... painful 😅

Copy link
Contributor

@benblub benblub left a comment

Choose a reason for hiding this comment

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

working for me with PostgreSQL100Platform.

the class is deprecated but the newer class isn't working yet.

Deprecated:
This class will be merged with PostgreSQLPlatform in 4.0 because support for Postgres releases prior to 10.0 will be dropped.
Suggest: inplement with an OR clause ..

@nikophil

@nikophil nikophil force-pushed the feat/drop-connections branch 23 times, most recently from 797ec7e to ff7c95b Compare May 13, 2023 17:25
@nikophil nikophil force-pushed the feat/drop-connections branch 7 times, most recently from c12fd51 to 85fa025 Compare May 17, 2023 06:37
@nikophil nikophil force-pushed the feat/drop-connections branch 9 times, most recently from 2444ab0 to 56cb31a Compare May 23, 2023 06:25
* @test
*/
public function it_generates_a_valid_schema(): void
public function it_can_use_schema_reset_with_migration(): void
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why, but in some case, when this test class used to have 2 tests, the second one was failing. Hence why I merged them

@nikophil nikophil requested a review from kbond May 23, 2023 11:21
@nikophil nikophil force-pushed the feat/drop-connections branch from 56cb31a to a56c88f Compare May 23, 2023 15:05
@nikophil nikophil force-pushed the feat/drop-connections branch from a56c88f to 1511940 Compare May 23, 2023 15:18
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

good to go after the mysql jobs are added as discussed!

@nikophil nikophil merged commit b48399d into zenstruck:1.x May 23, 2023
@nikophil nikophil deleted the feat/drop-connections branch May 23, 2023 15:26
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.

PostgreSQL and ResetDatabase trait

3 participants