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 --ping as a CLI option #1471

Merged
merged 6 commits into from
Jun 20, 2024
Merged

Add --ping as a CLI option #1471

merged 6 commits into from
Jun 20, 2024

Conversation

offbyone
Copy link
Contributor

@offbyone offbyone commented Jun 16, 2024

Description

This builds in the functionality asked for in #1470; it allows pgcli to replace pg_isready from the postgresql command line toolchain

Fixes: #1470

Checklist

  • I've added this contribution to the changelog.rst.
  • I've added my name to the AUTHORS file (or it's already there).
  • I installed pre-commit hooks (pip install pre-commit && pre-commit install), and ran black on my code.
  • Please squash merge this pull request (uncheck if you'd like us to merge as multiple commits)

offbyone added 2 commits June 16, 2024 11:07
This builds in the functionality asked for in dbcli#1470; it allows pgcli to
replace `pg_isready` from the postgresql command line toolchain
@offbyone offbyone marked this pull request as ready for review June 16, 2024 18:11
pgcli/main.py Fixed Show fixed Hide fixed
@j-bennet
Copy link
Contributor

j-bennet commented Jun 18, 2024

@offbyone This looks nice! I'd add an integration test scenario for this as well, see --list for an example:

Scenario: list databases
When we list databases
then we see list of databases

pgcli.echo doesn't seem to actually _echo_ anything
@offbyone
Copy link
Contributor Author

I added two feature tests, but one of them is failing and I don't quite know why; the exit code 0 is coming back with exit code 120, which ... I do not understand. If you know why, I'd be thrilled to update the spec.

pgcli/main.py Outdated Show resolved Hide resolved
results = None
try:
results = list(pgcli.pgexecute.run("SELECT 1"))
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what exception we could get, here. If pgcli cannot connect to the server (or use the database), we'll fail when calling PGCLI.connect() a few blocks before, not here.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, @dbaty, it's extremely unlikely, but not impossible. I've seen this once in my life, and not with postgresql; database server was up, but the transaction log was full. I'm ok with keeping the try/catch.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, @dbaty, it's extremely unlikely, but not impossible. I've seen this once in my life, and not with postgresql; database server was up, but the transaction log was full.

@j-bennet : then it would be useful to log the exact error we got, instead of the misleading "could not connect to the database".

@offbyone
Copy link
Contributor Author

I am 100% fine with any of those changes; I'm not super familiar with the codebase, and it took me until earlier today to even figure out how to test it effectively. Feel free! I left "Allow edits by maintainers" enabled so you are more than welcome to push any changes that make sense to you!

pgcli/main.py Fixed Show fixed Hide fixed
@dbaty
Copy link
Member

dbaty commented Jun 19, 2024

I added two feature tests, but one of them is failing and I don't quite know why; the exit code 0 is coming back with exit code 120, which ... I do not understand. If you know why, I'd be thrilled to update the spec.

I pulled your branch, ran behave and did not get any error. Could you clarify which command you run and the error you get?

@offbyone
Copy link
Contributor Author

I don't, any more; I found the cause of my failure to write the tests correctly and pushed an update to it.

Co-authored-by: Damien Baty <[email protected]>
pgcli/main.py Fixed Show fixed Hide fixed
pgcli/main.py Outdated Show resolved Hide resolved
@j-bennet
Copy link
Contributor

Looks good @offbyone; merging. 🍒 Thanks for your help, @dbaty!

@j-bennet j-bennet merged commit b979180 into dbcli:main Jun 20, 2024
7 checks passed
@offbyone
Copy link
Contributor Author

Thanks to all of you for the cleanups and finalizing it!

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.

Document how to replace pg_isready with pgcli
3 participants