feat: reduce user interaction in quicksetup

[ltalirz]

When verdi quicksetup detected that a database user or database already exists, it would prompt the user whether to use those and, if not, to pick a new name.

Novice users likely do not know what exactly this choice entails; furthermore, choosing to reuse the database user could actually fail, namely when the correct password for this user was not known in the config.json.

Furthermore, if the user decides not to reuse the db user/db, there is no point in letting them choose the new name. It is much easier to autogenerate it (if the user really wants to choose the name, they can pass it in via the --db-username/--db-name options to verdi quicksetup).

Here, we remove this user interaction from verdi quicksetup and simply generate new names for the dbuser and the dbname as needed.

[sphuber]

Thanks @ltalirz . I agree with the change in behavior, just some questions about the implementation

[sphuber]

Could we add an additional case here where you pass an invalid password to see it creates a new user automatically? Or do we hit the same problem potentially where certain postgres servers trust local connections? Is this the case for our CI setup? If not, I would still add the test (as well as the one you commented out) just so we have more coverage. If it then fails when people run the tests locally and they have their postgres configured that way, that is ok I think.

[ltalirz]

Hm... I've thought about it but is it really worth adding a test here that may fail depending on the details of the test setup?

The only thing this test would add is to check that psycopg2 throws the same exception when the password is wrong as when the user name is wrong (wrong user name is already tested).
I've checked that manually, and it does.

[sphuber]

What I mean though is that lines 221-224 of create_dbuser_db_safe are never hit in this test. In the first test, the user doesn't exist, and so the first conditional is hit and it is created. In the second test, the user exists and it can authenticate, so it goes straight to the database creation, skipping the block with self.find_new_user. So is that actually getting covered now by the tests? And my point here is that it is better to have that test covered in our CI and have it potentially fail if some developer runs the test locally and has a weird Postgres configuration. Anyway, aren't you testing this against a complete mock cluster provided by PGTest? So this should never fail, should it?

[ltalirz]

PGTest currently hardcodes the trust authentication method, i.e. when using the temporary postgres cluster the test will always fail
https://github.com/jamesnunn/pgtest/blob/417f0d89e66a9395db694a96122e0d5ac0097d59/pgtest/pgtest.py#L506-L507

I will open an issue for this to remind ourselves, also related to #3762

[sphuber]

Ok, so that means adding the test I suggested will fail on GHA? If so, then I guess we can leave it for now, but it would be good to fix in the future so we can increase coverage.

[ltalirz]

Correct, see

aiida-core/.github/workflows/ci-code.yml

Line 60 in 5b6b2b2

POSTGRES_HOST_AUTH_METHOD: trust

[ltalirz]

also opened jamesnunn/pgtest#24

[ltalirz]

We can have a look here at what happens today if we require passwords for postgres
#5776

[sphuber]

@ltalirz what is the status on this PR? Are there still open questions or should it simply be updated to contain your other recently merged PRs that worked on the quicksetup?

[ltalirz]

I think this can be updated and merged.

If you like, you can open an issue to remind ourselves that one may want to add a test that checks:
a new user is automatically created when one provides an existing username with an incorrect password.

This is currently problematic to add to the test suite, since for local tests we use pgtest which will accept any password. I will continue with #3762 to switch our CI setup to require a password for postgres, which should be closer to the real-world use cases.
If we wanted to, we could then add the test, but run it only on CI.

[sphuber]

Thanks @ltalirz