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

Better error message when psql is not there for database_exists #992

Open
wants to merge 1 commit into
base: stretch
Choose a base branch
from

Conversation

autra
Copy link
Contributor

@autra autra commented May 16, 2020

The problem

If app packager checks too soon for db existence (before postgres is installed) the error message is not very clear.

Solution

Just add a litte error message if psql cannot be found. The install crashes the same, but the packager now knows why, hopefully.

PR Status

...

How to test

...

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@@ -187,7 +187,12 @@ ynh_psql_database_exists() {
# Manage arguments with getopts
ynh_handle_getopts_args "$@"

if ! sudo --login --user=postgres PGUSER="postgres" PGPASSWORD="$(cat $PSQL_ROOT_PWD_FILE)" psql -tAc "SELECT datname FROM pg_database WHERE datname='$database';" | grep --quiet "$database"
# if postgres user doesn't exist, we consider the db does not as well
Copy link
Contributor Author

@autra autra May 16, 2020

Choose a reason for hiding this comment

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

aaaand forgot to edit the comment.

@autra autra force-pushed the psql_exist_error_message branch from bd9feb7 to 69dc711 Compare May 16, 2020
# though it could exists.
if ! command -v psql
then
ynh_die --message="PostgreSQL is not installed, impossible to check for db existence"
Copy link
Member

@alexAubin alexAubin May 16, 2020

Choose a reason for hiding this comment

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

Uuuuh I'm wondering if this helper ynh_psql_database_exists is called directly or indirectly during remove script, that could exit the remove script prematurely..?

Copy link
Contributor Author

@autra autra May 16, 2020

Choose a reason for hiding this comment

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

yes you're right ("If it can happen..."). OMG I don't know if there is a way out of this then.

Copy link
Member

@alexAubin alexAubin May 16, 2020

Choose a reason for hiding this comment

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

I'm guessing you had a real-life context that led you to implement this, soooo uuuh what was it ? :P

Naively just adding a warning could be good enough ?

Copy link
Contributor Author

@autra autra May 16, 2020

Choose a reason for hiding this comment

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

Well it's exactly what I wrote actually : I've decided to put all the checks (does the user/home dir/db already exists?) at the beginning of my script. Then of course it failed because I didn't have psql yet. I just thought that it should log correctly in this case, because I felt like it took me too much time to check what was happening.

But you're right : I should keep the old behaviour but just add a warning. I'm not yet sure how bash behaves in this case though... (what does it do? Returns 0? Returns nothing?)

Copy link
Member

@alexAubin alexAubin May 16, 2020

Choose a reason for hiding this comment

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

Hmmmm logging a warning is just a message and is idependant of what the function returns. If you want it to return 1, then just write return 1

Copy link
Contributor

@yalh76 yalh76 May 16, 2020

Choose a reason for hiding this comment

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

In addition ynh_psql_database_exists seems to only be use in ynh_psql_remove_db that is only used in the removal script... My opinion is: no need to check if postgresl is installed or not

Copy link
Contributor Author

@autra autra May 17, 2020

Choose a reason for hiding this comment

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

In addition ynh_psql_database_exists seems to only be use in ynh_psql_remove_db that is only used in the removal script... My opinion is: no need to check if postgresl is installed or not

If I read correctly the code of ynh_psql_create_db, I think we should alway call this function before attempting to create a db, only for the fact that ynh_psql_create_db modifies owner and rights quite violently. I actually do this check in diaspora_ynh. (actually maybe another fix should be to make ynh_psql_create_db call this function directly? Idk)

@alexAubin
Copy link
Member

@alexAubin alexAubin commented May 17, 2020

Related : #993

@autra autra force-pushed the psql_exist_error_message branch from 69dc711 to 481d4d0 Compare May 17, 2020
@autra
Copy link
Contributor Author

@autra autra commented May 17, 2020

I've updated with a (hopefully) correct return. Should match the old behaviour.

@autra autra force-pushed the psql_exist_error_message branch from 481d4d0 to efc3fc1 Compare May 17, 2020
Copy link
Member

@zamentur zamentur left a comment

LGTM (untested, but i read debate and read the final fix)

@zamentur zamentur added micro tests needed labels Jan 3, 2021
@zamentur zamentur added this to the 4.2.x milestone Jan 3, 2021
@zamentur zamentur added this to In progress in 4.2.x via automation Jan 4, 2021
@zamentur zamentur removed this from the 4.2.x milestone Jan 4, 2021
@zamentur zamentur moved this from In progress to Review in progress in 4.2.x Jan 4, 2021
@zamentur zamentur added -enhancement and removed -enh labels Jan 4, 2021
@alexAubin alexAubin removed this from Review in progress in 4.2.x Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
micro tests needed
Projects
Pending
Awaiting triage
4 participants