Forward "verdi code delete" to "verdi node delete" #3546
Conversation
verdi code delete was preventing users from deleting codes with associated calculations. While it's good to warn users, preventing them from doing it entirely seems unnecessary, also given that they could simply use verdi node delete (which most users won't realize). Thus adding a --force option that pipes `verdi code delete` through to `verdi node delete`.
|
I'm not sure I understand. Why was it preventing the deletion of those codes? were they trying to delete the codes (which are data nodes) without deleting the calculations associated? If this is so, passing the force keyword from code delete to node delete wouldn't help because it is only used to avoid asking for confirmation, it does not allow you to override fixed traversal rules which would be the issue in that case. EDIT: Nevermind, I now understand the problem is actually the confirmation prompt. |
|
Now, although adding the force keyword could be useful, I don't see why for verdi code delete you get an error instead of the information and confirmation prompt you get when you call verdi node delete. Also, mildly unrelated: is |
Well, one could pipe through directly to @sphuber @giovannipizzi What's your take on this?
Yes. |
But I mean, isn't that what you wanted with this PR? (or at least one of the options proposed) To be able to have 'verdi code delete' work when deleting codes that have calculations attached? |
* pipe "verdi code delete" through to the same function used in "verdi node delete" * fix migration warning for newer DB version
|
@ramirezfranciscof I just updated the PR to indeed simply pipe through to the function used by |
| @@ -162,16 +162,23 @@ def check_schema_version(profile_name): | |||
| set_db_schema_version(code_schema_version) | |||
| db_schema_version = get_db_schema_version() | |||
|
|
|||
| if code_schema_version != db_schema_version: | |||
| if code_schema_version > db_schema_version: | |||
There was a problem hiding this comment.
This code also exists for SqlAlchemy, maybe you want to update it there as well. Although this is not as easy as there you cannot simply compare revisions by their value. Anyway this code is touched heavily by PR #3582
There was a problem hiding this comment.
Ok, so how would one do it?
If there's no good way to do it, I guess we can leave the sqlalchemy version untouched.
There was a problem hiding this comment.
It's fine for now, but these changes will probably become irrelevant with PR #3582 . But we can merge this now.
| verbosity = 2 | ||
|
|
||
| node_pks_to_delete = [code.pk for code in codes] | ||
| delete_nodes(node_pks_to_delete, dry_run=dry_run, verbosity=verbosity, force=force) |
There was a problem hiding this comment.
What will the message now be here for a code with calculations and force=False? I guess since it pipes through to delete_nodes it will not explicitly mention code or its calculations but just nodes in general. Would it maybe be better to do the check in this command and raise a critical error unless force=True. Then the message can be made more specific.
There was a problem hiding this comment.
An earlier version of the PR did exactly this - but then I also agree with @ramirezfranciscof that I don't really see a reason to be more strict with codes than with other nodes.
When one calls verdi code delete ... without --force, it will simply prompt yes/no if there are other nodes attached.
To me this seems fine.
|
Thanks @ltalirz |
ltalirz
changed the title
fix #3545
verdi code delete was preventing users from deleting codes with
associated calculations.
While it's good to warn users, preventing them from doing it entirely
seems unnecessary, also given that they could simply use verdi node
delete (which most users won't realize).
Thus adding a --force option that pipes
verdi code deletethrough toverdi node delete.