Skip to content
This repository has been archived by the owner on Dec 3, 2024. It is now read-only.

Feature | Postgres Support #69

Merged
merged 41 commits into from
Mar 3, 2023
Merged

Conversation

ksimenic
Copy link
Contributor

@ksimenic ksimenic commented Feb 21, 2023

This PR merges 2 existing PRs: #60 and #63 and potentially solves #61

Additionally, both PRs are standardised (pint) and upgraded with following changes:

  • 60 - improved description in config file
  • 60 - improved env vars fallbacks and added new env var as override
  • 60 - added missing return types on models
  • 60 - improve TestCase logic, add return types and fix failing test
  • 63 - extracted DB::connection instanceof check to separate function and take db_connection configuration var (from 60) into account

@ksimenic
Copy link
Contributor Author

Additional comment for testing. Since sqlite is used for testing, it is not that straight forward to implement tests for mysql/pgsql. That would also require additional work for both local development and github actions. However, if adding tests is necessary I can work on them in the scope of this PR or in some other PR.

@Sammyjo20
Copy link
Owner

Many thanks for this PR I will have a review now. If you know how to build a testing matrix with SQLite, MySQL and Postgres that would be fantastic. Might that be something you could add to this PR please?

Copy link
Owner

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

Thank you for this

src/Helpers/SerializationHelper.php Outdated Show resolved Hide resolved
src/Helpers/SerializationHelper.php Outdated Show resolved Hide resolved
src/Helpers/SerializationHelper.php Outdated Show resolved Hide resolved
@ksimenic
Copy link
Contributor Author

@Sammyjo20

Added tests, as requested. Additionally, I ran pint on whole project (not just on updated files) and fixed code style per your suggestion.

There are 2 things you should be aware of.

  1. Running tests locally
    Since now we have tests on MySQL and PgSQL, to run the tests locally we need both MySQL and PgSQL server.
    I am using DBnging locally. Both MySQL and PgSQL are created with default values except the port. For MySQL port is 33066 and for PgSQL port is 54321. Additionally, once server is created and started you must create database named "testing" on both servers.

image

  1. Running tests in matrix
    Needed to drop support for "windows-latest" since you can't create "services" (postgres, mysql) there.

image

@ksimenic ksimenic requested a review from Sammyjo20 February 22, 2023 11:48
@ksimenic
Copy link
Contributor Author

Hey @Sammyjo20. I am just wondering if there is a plan to release this any time soon? If not, I will need to unblock my projects by using my own fork. No problem at all if there is no plan to release it. Thank you!

@Sammyjo20
Copy link
Owner

Hey @ksimenic I am hoping to release this at the weekend if this works for you?

@ksimenic
Copy link
Contributor Author

@Sammyjo20 works for me. Thank you!

Copy link
Owner

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

Happy with this 💪

@Sammyjo20 Sammyjo20 merged commit 84a2332 into Sammyjo20:main Mar 3, 2023
@Sammyjo20
Copy link
Owner

Thank you @ksimenic for your help on this!

@ksimenic
Copy link
Contributor Author

ksimenic commented Mar 3, 2023

Hey @Sammyjo20. Thank you for releasing this! Just wondering why you dropped support for Laravel 8.x?

@Sammyjo20
Copy link
Owner

Hey @ksimenic I've dropped support for Laravel 8 because it's no longer getting any security updates or officially supported.

image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants