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 postgres start options #351

Merged

Conversation

rafmagns-skepa-dreag
Copy link
Contributor

@rafmagns-skepa-dreag rafmagns-skepa-dreag commented Dec 10, 2020

Fixes #346.

Changes proposed: add ability to pass options to pg_ctl's -o flag to send arguments to the underlying postgres executable

Additional changes:

  • Add tests for postgres 12, 13
  • Update travis image to bionic
  • Update 3.9 testing to released version
  • Update minimum postgres version to 9.5 everywhere
  • Small test fixes

@coveralls
Copy link

coveralls commented Dec 14, 2020

Coverage Status

Coverage increased (+6.2%) to 65.971% when pulling 5a866c9 on rafmagns-skepa-dreag:f/add-postgres-start-options into 0c8d8eb on ClearcodeHQ:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 59.513% when pulling 3ac409b on rafmagns-skepa-dreag:f/add-postgres-start-options into 5bbcca3 on ClearcodeHQ:master.

Copy link
Member

@fizyk fizyk left a comment

Choose a reason for hiding this comment

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

I love the thorough approach you took here :)

Just a small requirement, to change the args to options. And rename postgres_extra_params to postgres_options maybe.

@@ -33,6 +33,7 @@
_help_unixsocketdir = "Location of the socket directory"
_help_dbname = "Default database name"
_help_load = "Load this SQL file by default, may be set multiple times"
_help_postgres_start = 'Postgres executable extra parameters'
Copy link
Member

Choose a reason for hiding this comment

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

let's mention and indicate, that this is for the -o parameter, options. Here and in readme's table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be addressed now

requirements-test.txt Outdated Show resolved Hide resolved
src/pytest_postgresql/executor.py Show resolved Hide resolved
@@ -71,13 +72,13 @@ def test_rand_postgres_port(postgresql_rand):

@pytest.mark.parametrize('_', range(2))
def test_postgres_terminate_connection(
postgresql, _):
postgres10, _):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to have been using postgres 9.x before. The backend_type column was introduced in version 10, so here we make sure that it'll always use at least 10

@rafmagns-skepa-dreag rafmagns-skepa-dreag marked this pull request as ready for review December 16, 2020 05:46
src/pytest_postgresql/executor.py Show resolved Hide resolved
@@ -60,10 +60,14 @@ def version(self):
options=self.options
) as connection:
version = str(connection.server_version)
# Pad the version for releases before 10
# if not we get 90524 instead of 090524
Copy link
Member

Choose a reason for hiding this comment

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

Why is the padding needed? the parse_version output can be safely compared I think? OR was there a different issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I was running it locally, it would parse the version for the 9.x releases incorrectly. 90524 became 90.52 instead of 9.5

@rafmagns-skepa-dreag
Copy link
Contributor Author

I solved the macos build issues. It now runs tests against all supported versions of postgres on macos 10.15 Catalina (build was previously running against Travis default 10.13 High Sierra). However, the build takes a very long time because it updates homebrew. We can turn this off and just have it test against postgres 13 which would remove the brew update. Not sure what the Travis plan is like for this org/repo, but those macos builds are likely to get expensive quickly

Copy link
Member

@fizyk fizyk left a comment

Choose a reason for hiding this comment

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

@rafmagns-skepa-dreag it open source, so we use an open source plan for this repo (I suppose that's the reason some builds finish two days after being triggered).
However, I plan to migrate it to the github actions, so if you'd have a github actions template you could use here instead travis at the very moment, feel free to migrate osx tests there, however the only fix I require would be for to fix the coverage issue in pytest configuration

pytest.ini Outdated
@@ -0,0 +1,3 @@
[pytest]
Copy link
Member

Choose a reason for hiding this comment

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

configure these here: https://github.com/ClearcodeHQ/pytest-postgresql/blob/master/setup.cfg#L9

Just looked around as to why the coverage isn't being gathered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah! I was wondering why the coverage failed. Thanks!

@fizyk fizyk merged commit 6b0e9de into ClearcodeHQ:master Feb 11, 2021
@fizyk
Copy link
Member

fizyk commented Feb 11, 2021

will be v2.6.0 as soon as it finishes building.

@rafmagns-skepa-dreag rafmagns-skepa-dreag deleted the f/add-postgres-start-options branch February 11, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to pass arguments directly to the postgres start command
3 participants