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

CLI: set localhost as default for database hostname in verdi setup #4908

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented May 4, 2021

Fixes #4907

The default was actually being defined on the option, however, it was
taken from the pgsu.DEFAULT_DSN dictionary, which defines the database
hostname to be None. Still, 9 out of 10 times the database is on the
localhost so not having this as a default is kind of annoying and
unnecessary.

The default was actually being defined on the option, however, it was
taken from the `pgsu.DEFAULT_DSN` dictionary, which defines the database
hostname to be `None`. Still, 9 out of 10 times the database is on the
localhost so not having this as a default is kind of annoying and
unnecessary.
@sphuber sphuber requested a review from ltalirz May 4, 2021 07:38
@ltalirz
Copy link
Member

ltalirz commented May 4, 2021

thanks @sphuber - to my knowledge, there actually is a difference between None and localhost, see
https://github.com/aiidateam/pgsu/blob/43d8d305053e9f6a1352295bf9a3f6955881d5ba/pgsu/__init__.py#L17

In particular, I believe that localhost will not work with the default postgres setup on ubuntu that goes via the local connection.

What is annoying about the default?

@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #4908 (8ea1c8c) into develop (87ab7e3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4908   +/-   ##
========================================
  Coverage    80.07%   80.07%           
========================================
  Files          518      518           
  Lines        36683    36683           
========================================
  Hits         29371    29371           
  Misses        7312     7312           
Flag Coverage Δ
django 74.56% <ø> (+0.03%) ⬆️
sqlalchemy 73.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/cmdline/params/options/__init__.py 98.27% <ø> (ø)
aiida/cmdline/params/options/commands/setup.py 56.85% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87ab7e3...8ea1c8c. Read the comment docs.

@sphuber
Copy link
Contributor Author

sphuber commented May 4, 2021

In particular, I believe that localhost will not work with the default postgres setup on ubuntu that goes via the local connection.

Maybe circumstantial evidence but I have been using localhost on Ubuntu 18.04 and 20.04 since forever without any issues.

What is annoying about the default?

I always have to specify localhost when creating a new profile. Since most cases would actually use this, it seems to me like there is not really a reason to not have that as a default.

@ltalirz
Copy link
Member

ltalirz commented May 4, 2021

In particular, I believe that localhost will not work with the default postgres setup on ubuntu that goes via the local connection.

Maybe circumstantial evidence but I have been using localhost on Ubuntu 18.04 and 20.04 since forever without any issues.

I might be wrong (I'll check). Can you please share the uncommented lines of your /etc/postgresql/10/main/pg_hba.conf ?

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

On second thought, I think you are right.

'localhost' is the correct default there, while None is a good thing to try for pgsu (because it is the only way you can access the psql shell without a password).

You may want to include this information in the commit message.

@sphuber
Copy link
Contributor Author

sphuber commented May 4, 2021

local   all             postgres                                peer

# TYPE  DATABASE        USER            ADDRESS                 METHOD

# "local" is for Unix domain socket connections only
local   all             all                                     peer
# IPv4 local connections:
host    all             all             127.0.0.1/32            md5
# IPv6 local connections:
host    all             all             ::1/128                 md5
# Allow replication connections from localhost, by a user with the
# replication privilege.
local   replication     all                                     peer
host    replication     all             127.0.0.1/32            md5
host    replication     all             ::1/128                 md5

This is the default config of postgres-12 on Ubuntu 20.04

@sphuber
Copy link
Contributor Author

sphuber commented May 4, 2021

You may want to include this information in the commit message.

Cheers, will do.

@sphuber sphuber merged commit 6c4ced3 into aiidateam:develop May 4, 2021
@sphuber sphuber deleted the fix/4907/verdi-setup-database-hostname-default branch May 4, 2021 08:11
sphuber added a commit that referenced this pull request Aug 8, 2021
#4908)

The default was actually being defined on the option, however, it was
taken from the `pgsu.DEFAULT_DSN` dictionary, which defines the database
hostname to be `None`. Still, 9 out of 10 times the database is on the
localhost so not having this as a default is kind of annoying and
unnecessary.

Note that `pgsu` specifies `None` as the default because this is at times
the only way the psql shell can be accessed without a password.

Cherry-pick: 6c4ced3
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.

Use localhost as default for --database-hostname in verdi setup
2 participants