agroal / pgagroal

High-performance connection pool for PostgreSQL
https://agroal.github.io/pgagroal/
BSD 3-Clause "New" or "Revised" License
667 stars 59 forks source link

Enable PostgreSQL and run pgagroal-cli status in CI #427

Closed decarv closed 3 months ago

decarv commented 3 months ago

This commit modifies the build-linux workflow by incorporating PostgreSQL and by testing pgagroal for both GCC and CLANG build. The changes ensure that the workflow:

(a) Installs and runs the latest version of PostgreSQL; (b) Runs pgagroal; and (c) Runs pgagroal-cli status command and to confirm that pgagroal's status is "Running".

I added a sleep to guarantee that pgagroal can start before pgagroal-cli is run.

decarv commented 3 months ago

Unfortunately, I was not able to add macOS support. I could try, but it would be a bit harder as I do not have the environment.

decarv commented 3 months ago

Implemented @fluca1978 suggestions on this commit.

The latest commit fixed a typo (introduced by mistake on the previous commit) in the name of one of the runs.

The build completes without errors.

fluca1978 commented 3 months ago

Looks fine for me. @jesperpedersen PTAL

jesperpedersen commented 3 months ago

@decarv Is it possible for you to use the GitHub CI with its Mac support to test it ?

Otherwise we can just move forward and leave it for later...

decarv commented 3 months ago

@jesperpedersen , absolutely!

Initially, I hesitated to include macOS because I believed that it would be different to set up PostgreSQL, and I would have no way to test the setup locally -- and would end up polluting the actions with failed runs.

I plan to address this later tonight.

decarv commented 3 months ago

@jesperpedersen

Added the macOS support and the build completes without errors.

The installation step for PostgreSQL may look kind of hacky (I pointed this out in the commit message), but it is the workaround I managed to find to avoid Homebrew errors in creating symlinks during installation. I could not find a better solution (for reference, there is a previous discussion on this subject: https://github.com/Homebrew/brew/issues/1742).

fluca1978 commented 3 months ago

Good job, I admit the || true part is really tricky. Can we document it also in place for a quick future reference? Besides, this is fine for me. @jesperpedersen PTAL

decarv commented 3 months ago

@fluca1978

something like this?

brew install ${latest_pg} || true  # `|| true` prevents install errors from breaking the run
jesperpedersen commented 3 months ago

We should have the stop commands as well for PostgreSQL and pgagroal

decarv commented 3 months ago

We should have the stop commands as well for PostgreSQL and pgagroal

I don't understand.

Do you mean to shut down a pgagroal instance with pgagroal-cli shutdown and test whether this effectively shuts down the server?

What should I be looking for when stopping PostgreSQL?

jesperpedersen commented 3 months ago

Yeah, just issue pgagroal-cli stop and pg_ctl stop after

decarv commented 3 months ago

The previous design was to kill the pgagroal process with kill $pid. Now I shutdown pgagroal with pgagroal-cli shutdown and stop postgres for each GCC and CLANG builds.

Also, I learned that the check if [ $? != 0]; then ... is unnecessary because the run will just stop when a program exits with status 1.

fluca1978 commented 3 months ago

Since we are no more testing that pgagroal-cli status outputs "running" (we were testing $?, but as @decarv stated, it is not necessary), why don't we just test for pgagroal-cli ping to see if the pooler is running?

jesperpedersen commented 3 months ago

Yeah, ping should be enough for now

decarv commented 3 months ago

@fluca1978 I replaced pgagroal-cli status with pgagroal-cli ping. Please correct me if that is not what you meant for me to do.

I like it because it conveys something semantically (although we know that any command would exit with status 1 if the server is not running).

fluca1978 commented 3 months ago

Looks good to me. @jesperpedersen PTAL

jesperpedersen commented 3 months ago

Merged.

Thanks for your contribution !