Replace `verdi database` commands with `verdi storage` #5228

ramirezfranciscof · 2021-11-10T17:42:19Z

The following specific commands were moved:

integrity: the checks that were performed here are no longer relevant.
There will be a new verdi storage integrity that will perform more
general checks (once the features of the repository are implemented).
migrate: moved to verdi storage migrate.
summary: information now available through verdi storage info.
version: information now available through verdi status.

The respective tests were adapted as well.

codecov · 2021-11-11T10:52:45Z

Codecov Report

Merging #5228 (bf8ad01) into develop (370358d) will increase coverage by 0.04%.
The diff coverage is 96.94%.

@@             Coverage Diff             @@
##           develop    #5228      +/-   ##
===========================================
+ Coverage    81.20%   81.23%   +0.04%     
===========================================
  Files          532      533       +1     
  Lines        37371    37345      -26     
===========================================
- Hits         30344    30335       -9     
+ Misses        7027     7010      -17

Flag	Coverage Δ
django	`76.11% <96.94%> (+0.05%)`	⬆️
sqlalchemy	`75.19% <96.94%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
aiida/cmdline/commands/__init__.py	`100.00% <ø> (ø)`
aiida/cmdline/commands/cmd_database.py	`92.86% <80.00%> (+30.46%)`	⬆️
aiida/cmdline/commands/cmd_storage.py	`98.67% <98.67%> (ø)`
aiida/cmdline/commands/cmd_status.py	`84.85% <100.00%> (+0.64%)`	⬆️
aiida/manage/database/integrity/sql/links.py	`0.00% <0.00%> (-100.00%)`	⬇️
aiida/manage/database/integrity/sql/nodes.py	`0.00% <0.00%> (-100.00%)`	⬇️
aiida/orm/implementation/sql/backends.py	`75.00% <0.00%> (-21.42%)`	⬇️
aiida/orm/implementation/sqlalchemy/backend.py	`89.48% <0.00%> (-5.26%)`	⬇️
aiida/manage/manager.py	`80.45% <0.00%> (-0.55%)`	⬇️
aiida/transports/plugins/local.py	`81.66% <0.00%> (+0.26%)`	⬆️
... and 3 more

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 370358d...bf8ad01. Read the comment docs.

The following specific commands were moved: - integrity: the checks that were performed here are no longer relevant. There will be a new verdi storage integrity that will perform more general checks (once the features of the repository are implemented). - migrate: moved to `verdi storage migrate`. - summary: information now available through `verdi storage info`. - version: information now available through `verdi status`. The respective tests were adapted as well.

ramirezfranciscof · 2021-11-11T16:17:59Z

I think this is ready for review. I moved the stuff around and added a couple of tests to cover some parts of the migration that was not previously being tested (now the only part not being tested is the re-prompting but I'm not sure how to trigger that with the run_cli_command fixture). One of the tests (tests_storage_migrate_raises) has a slightly awkward mock but I explained there why I had to do it that way.

(@chrisjsewell I put @sphuber as the reviewer because I didn't want to bother you during your holidays, but obviously feel free to take a look too)

sphuber

Thanks @ramirezfranciscof . Looks good, just some comments.

aiida/cmdline/commands/cmd_database.py

sphuber · 2021-11-12T08:10:21Z

aiida/cmdline/commands/cmd_database.py

-    echo.echo(backend_manager.get_schema_version_database())
+    .. deprecated:: v2.1.0
+    """
+    echo.echo_warning(


We have a dedicated decorator for this. Probably best to use that. Its in aiida.cmdline.utils.decorators.

Ok, will do. A couple of questions though:

I see it takes care of the warning, but should I still leave the .. deprecated:: v2.1.0 in the docstring?

Do I just decorate the end commands? Just the group? Both the group and the end commands?

I didn't find anything on the docs or the wiki for deprecations of the CLI, should I maybe add this info to the wiki?

Yes, I think leaving the explicit deprecated in the docstring is ok. But I think it should mention the version when it was deprecated, not when it is going to be removed.

definitely the leaf commands as the group is not really called (just for the help) but if it works, why not also add it there.

Feel free to add it to the wiki. Since this is a developer's aspect, it should go there and not the docs.

I see. But then how do we keep easy track of when we agreed to remove something? I included this info in the deprecation warning now but on second thought I am not sure this is something we want to show to the user necessarily... 🤔

Nah, in the end it did nothing for the groups, not even showed up if I tried using the help. But I think if this is a limitation of the decorator we could maybe extend it to also work on groups no? If it is even possible. For the future anyways.

Check out if you think it is clear: wiki section

I think it is fine to mention to the user when we are planning on removing it. Why wouldn't we? This shouldn't be a secret should it and it affects them.

aiida/cmdline/commands/cmd_database.py

aiida/cmdline/commands/cmd_status.py

tests/cmdline/commands/test_storage.py

Co-authored-by: Sebastiaan Huber <[email protected]>

aiida/cmdline/commands/cmd_storage.py

ramirezfranciscof · 2021-11-16T08:17:31Z

I think I addressed all the comments now (even changing the ones about which I still have some reservations =P). Ready for a second (last? 😬) pass whenever you can @sphuber.

sphuber

Thanks @ramirezfranciscof two minor things. Already approve if it is intentional and don't want to change it, but otherwise I will quickly reapprove after you addressed it.

tests/cmdline/commands/test_status.py

sphuber · 2021-11-16T08:22:01Z

aiida/cmdline/commands/cmd_database.py

-            echo.echo_success('migration completed')
+@click.pass_context
+@decorators.deprecated_command(
+    'This command has been deprecated and will be removed soon (in v3.0). '


Is it intentional that this will be removed in 3.0 and the rest in 2.1?

Yes, this is intentional. I personally would prefer to leave the deprecations for longer (not sure if until 3.0, but probably more than 2.1 since I suspect this will be out relatively soon), but this is what was agreed in the last meeting (see notes).

BTW: I'm still not convinced about exposing this information to the user since this is mostly for internal reference (i.e. remembering when to take it out), but I didn't know where else to track this since the .. deprecated:: vA.B.C addition to the docstring is for recording when it was deprecated and not up to when. Any suggestions?

I don't agree that it is for internal remembering. Since it affects the user exactly when it is going to be removed, it seems like very relevant information for them. Only thing missing is when the version that it will be removed in is expected to be released.

sphuber

Good to go for me, thanks @ramirezfranciscof

ramirezfranciscof · 2021-11-16T12:09:26Z

Thanks a lot for the review @sphuber !

ramirezfranciscof force-pushed the repocli_base branch from 031d27b to 9c0c6a2 Compare November 11, 2021 10:30

ramirezfranciscof force-pushed the repocli_base branch from 9c0c6a2 to 368d441 Compare November 11, 2021 14:22

ramirezfranciscof force-pushed the repocli_base branch from 368d441 to 1e27f3b Compare November 11, 2021 14:29

ramirezfranciscof added the topic/verdi label Nov 11, 2021

ramirezfranciscof added this to the v2.0.0 milestone Nov 11, 2021

ramirezfranciscof requested a review from sphuber November 11, 2021 16:13

ramirezfranciscof marked this pull request as ready for review November 11, 2021 16:13

sphuber requested changes Nov 12, 2021

View reviewed changes

ramirezfranciscof and others added 2 commits November 12, 2021 11:30

Apply suggestions from code review

63ca7e4

Co-authored-by: Sebastiaan Huber <[email protected]>

Add modifications of code review

c5eb984

chrisjsewell reviewed Nov 12, 2021

View reviewed changes

aiida/cmdline/commands/cmd_storage.py Outdated Show resolved Hide resolved

ramirezfranciscof requested a review from sphuber November 12, 2021 15:49

Second batch of PR modifications.

55c8b7e

ramirezfranciscof force-pushed the repocli_base branch from 9e3017c to 55c8b7e Compare November 12, 2021 16:09

Merge branch 'develop' into repocli_base

49d1097

sphuber previously approved these changes Nov 16, 2021

View reviewed changes

Quickfix

bf8ad01

ramirezfranciscof dismissed sphuber’s stale review via bf8ad01 November 16, 2021 10:15

ramirezfranciscof requested a review from sphuber November 16, 2021 10:23

sphuber approved these changes Nov 16, 2021

View reviewed changes

ramirezfranciscof merged commit f8d1615 into aiidateam:develop Nov 16, 2021

giovannipizzi mentioned this pull request Dec 6, 2021

verdi database migrate still mentioned in a few places (instead of verdi storage migrate) #5245

Closed

This was referenced Dec 13, 2021

Docs: Add documentation on the verdi storage command #5271

Open

verdi: add backup sub-command to the storage group #5273

Closed

chrisjsewell mentioned this pull request Jan 24, 2022

Improving AiiDA terminology #5324

Open

ramirezfranciscof deleted the repocli_base branch February 25, 2022 16:57

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `verdi database` commands with `verdi storage` #5228

Replace `verdi database` commands with `verdi storage` #5228

ramirezfranciscof commented Nov 10, 2021 •

edited

Loading

codecov bot commented Nov 11, 2021 •

edited

Loading

ramirezfranciscof commented Nov 11, 2021

sphuber left a comment

sphuber Nov 12, 2021

ramirezfranciscof Nov 12, 2021

sphuber Nov 12, 2021

ramirezfranciscof Nov 12, 2021

sphuber Nov 16, 2021

ramirezfranciscof commented Nov 16, 2021

sphuber left a comment

sphuber Nov 16, 2021

ramirezfranciscof Nov 16, 2021

sphuber Nov 16, 2021

sphuber left a comment

ramirezfranciscof commented Nov 16, 2021

Replace verdi database commands with verdi storage #5228

Replace verdi database commands with verdi storage #5228

Conversation

ramirezfranciscof commented Nov 10, 2021 • edited Loading

codecov bot commented Nov 11, 2021 • edited Loading

Codecov Report

ramirezfranciscof commented Nov 11, 2021

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramirezfranciscof commented Nov 16, 2021

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

ramirezfranciscof commented Nov 16, 2021

Replace `verdi database` commands with `verdi storage` #5228

Replace `verdi database` commands with `verdi storage` #5228

ramirezfranciscof commented Nov 10, 2021 •

edited

Loading

codecov bot commented Nov 11, 2021 •

edited

Loading