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.

Sign up for GitHub

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
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion nix/tools/withTools.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

];
positionalCompletion = "_command";
inRootDir = true;
Expand Down Expand Up @@ -66,7 +67,7 @@ let

log "Starting the database cluster..."
# Instead of listening on a local port, we will listen on a unix domain socket.
pg_ctl -l "$tmpdir/db.log" -w start -o "-F -c listen_addresses=\"\" -c hba_file=$HBA_FILE -k $PGHOST -c log_statement=\"all\"" \
pg_ctl -l "$tmpdir/db.log" -w start -o "-F -c listen_addresses=\"\" -c hba_file=$HBA_FILE -k $PGHOST -c log_statement=\"all\" -c timezone=utc" \
>> "$setuplog"

# shellcheck disable=SC2317
Expand Down