Skip to content

verdi: Deprecate --db-engine option #6906

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 6, 2025
Merged

verdi: Deprecate --db-engine option #6906

merged 2 commits into from
Jun 6, 2025

Conversation

danielhollas
Copy link
Collaborator

Partly addresses #6905, see the issue for more details and motivation behind this.

danielhollas
danielhollas commented Jun 5, 2025
View reviewed changes
src/aiida/cmdline/params/options/main.py
default='postgresql_psycopg',
type=click.Choice(['postgresql_psycopg']),
type=click.Choice(['postgresql_psycopg', 'postgresql_psycopg2']),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We add back postgresql_psycopg2 since this option is ignored anyway. It might save somebody some trouble when upgrading to aiida 2.7.0

@codecov Codecov
Copy link

codecov bot commented Jun 5, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.58%. Comparing base (88df72a) to head (d981f0d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/aiida/cmdline/commands/cmd_setup.py 50.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6906      +/-   ##
==========================================
- Coverage   78.58%   78.58%   -0.00%     
==========================================
  Files         564      564              
  Lines       43126    43130       +4     
==========================================
+ Hits        33888    33890       +2     
- Misses       9238     9240       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

agoscinski
agoscinski requested changes Jun 5, 2025
View reviewed changes
Copy link
Contributor

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

thanks for taking care it. minor changes requested

src/aiida/cmdline/commands/cmd_setup.py
@@ -205,6 +212,12 @@ def quicksetup(
# store default user settings so user does not have to re-enter them
_store_default_user_settings(ctx.obj.config, email, first_name, last_name, institution)

if non_interactive and db_engine != 'postgresql_psycopg':
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is just to avoid the error message to appear when one enters it in interactive mode? I think we could also fire it in interactive mode but no strong opinios.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I found it quite distracting when I was testing it.

danielhollas
danielhollas commented Jun 5, 2025
View reviewed changes
src/aiida/cmdline/commands/cmd_setup.py
dbinfo_su = {
'host': db_host,
'port': db_port,
'user': su_db_username,
'password': su_db_password,
'database': su_db_name,
'dbname': su_db_name,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@agoscinski FYI: This removes the following deprecation warning coming from the pgsu package that you can trivially see when running verdi quicksetup and that I saw many times in test suites and never knew where it is coming from :D

image

This was missed in #6362

Copy link
Contributor

Choose a reason for hiding this comment

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

I am happy that you found closure

@danielhollas danielhollas force-pushed the deprecate-db-engine branch from 4eba484 to 1e4a3b7 Compare June 5, 2025 15:17
@danielhollas danielhollas requested a review from agoscinski June 5, 2025 15:19
@danielhollas danielhollas moved this to In review in aiida-core v2.7.0 Jun 5, 2025
@danielhollas danielhollas self-assigned this Jun 5, 2025
@agoscinski agoscinski force-pushed the deprecate-db-engine branch from 1e4a3b7 to d981f0d Compare June 6, 2025 08:12
agoscinski
agoscinski approved these changes Jun 6, 2025
View reviewed changes
Copy link
Contributor

@agoscinski agoscinski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the work! I ignore patch coverage because changes only effect a deprecated cli command

src/aiida/cmdline/commands/cmd_setup.py
dbinfo_su = {
'host': db_host,
'port': db_port,
'user': su_db_username,
'password': su_db_password,
'database': su_db_name,
'dbname': su_db_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am happy that you found closure

@agoscinski
Copy link
Contributor

@danielhollas ok?

Deprecate `verdi quicksetup` the `--db-engine` option (#6906)

The `--db-engine` option only supports `postgresql_psycopg`. To avoid failure
due the deprecation of `postgresql_psycopg2` (note that intuitively
`postgresql_psycopg` is the newer version) we add the support in the click
options and echo a deprecation warning that the option does not have any
effect.

Fixes renaming of `database` -> `dbname` missed in #6362 that raised
deprecation warning.

@danielhollas
Copy link
Collaborator Author

I would mention in the commit message that the option doesn't actually influence anything, since the actual DB engine is hardcoded. So it's safe to accept the old value. Otherwise looks good!

@agoscinski agoscinski merged commit 8dd0948 into aiidateam:main Jun 6, 2025
11 of 12 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in aiida-core v2.7.0 Jun 6, 2025
@danielhollas danielhollas deleted the deprecate-db-engine branch June 6, 2025 10:26
@danielhollas danielhollas mentioned this pull request Jun 9, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agoscinski agoscinski agoscinski approved these changes

@unkcpz unkcpz Awaiting requested review from unkcpz

@mbercx mbercx Awaiting requested review from mbercx

Assignees

@danielhollas danielhollas

Labels
None yet
Projects
aiida-core v2.7.0
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danielhollas @agoscinski