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

nix: UTC by default for postgrest-with-postgresql #2666

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

steve-chavez
Copy link
Member

Fixes unreliable output on tests, see #2523 (comment)

@steve-chavez steve-chavez merged commit c49932d into PostgREST:main Feb 15, 2023
@@ -25,6 +25,7 @@ let
"ARG_USE_ENV([PGUSER], [postgrest_test_authenticator], [Authenticator PG role])"
"ARG_USE_ENV([PGDATABASE], [postgres], [PG database name])"
"ARG_USE_ENV([PGRST_DB_SCHEMAS], [test], [Schema to expose])"
"ARG_USE_ENV([PGTZ], [utc], [Timezone to use])"
Copy link
Member

Choose a reason for hiding this comment

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

What does adding this do?

Or did you want to use $PGTZ below in the -c timezone= addition?

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 was mostly adding it for documentation purposes. PGTZ is pg env var: https://www.postgresql.org/docs/current/libpq-envars.html

So I can do:

PGTZ=EST postgrest-with-postgresql-15 psql

select now()

And get the correct time for my timezone.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does override the -c timezone, I tested it.

Copy link
Member

Choose a reason for hiding this comment

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

Hm. Doesn't that mean that at least one of the two is redundant, then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I don't see it that way. PGTZ is for overriding and -c timezone=utc is for the default.

The problem on #2523 (comment) was that running postgrest-test-spec failed locally and not on CI and that was confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Open to hear better alternatives, not sure what would be the issue with the current setup though.

Copy link
Member

Choose a reason for hiding this comment

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

PGTZ is for overriding

But it doesn't do it. If you remove the ARG_USE_ENV line - it will still override, when you explicitly set PGTZ before calling postgrest-with-postgresql-xx.

So this line is just unused in it's current form, especially the default value you give on that line, too. Try changing the default in the ARG_USE_ENV line - it doesn't have any effect. Or in other words, you currently have two default values: One in the ARG_USE_ENV, one in the -c timezone=. Obviously, only one default can actually "work", right?

There's two ways to actually use PGTZ in the nix script, too:

"ARG_USE_ENV([PGTZ], [utc], [Timezone to use])"
...
export PGTZ

Doing it this way, you don't need the -c timezone=utc anymore. PGTZ will be used from the outside - and if not set, the default utc will be used as given in the ARG_USE_ENV line. PGTZ will then be passed to any client that uses the temporary database.

I think this is the cleanest approach.


The other way would be like this:

"ARG_USE_ENV([PGTZ], [utc], [Timezone to use])"
...
-c timezone="$PGTZ"

This would make the the default for PGTZ only available within the current script - but then set it as the default via server-side configuration.

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, cool. Fixed according to approach 1 on #2674

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.

2 participants