-
Notifications
You must be signed in to change notification settings - Fork 187
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
✨ NEW: Add verdi group move-nodes
command
#4428
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It all looks pretty good! I just added two ideas that might be worth considering, let me know what you think.
Thanks for the review, @ramirezfranciscof! I've replied to your comments, will make changes once we have resolved the issues. |
7544278
to
fcc7ba3
Compare
Usage: $ verdi group move-nodes -h
Usage: verdi group move-nodes [OPTIONS] [NODES]...
Move nodes from one group to another.
Options:
-s, --source-group GROUP A single group identified by its ID, UUID or
label. [required]
-t, --target-group GROUP A single group identified by its ID, UUID or
label. [required]
-f, --force Do not ask for confirmation and skip all checks.
-h, --help Show this message and exit. I've created two test groups with the following content: $ verdi group show test1
----------------- ----------------
Group label test1
Group type_string core
Group description <no description>
----------------- ----------------
# Nodes:
PK Type Created
---- ------ --------------
161 Int 7D:10h:42m ago
157 Int 7D:10h:44m ago
159 Int 7D:10h:43m ago $ verdi group show test2
----------------- ----------------
Group label test2
Group type_string core
Group description <no description>
----------------- ----------------
# Nodes:
PK Type Created
---- ------ --------------
159 Int 7D:10h:43m ago When I try to move a node that is not in $ verdi group move-nodes -s test1 -t test2 151
Critical: None of the specified nodes are in Group<test1>. When I try to move two nodes, one which is in $ verdi group move-nodes -s test1 -t test2 151 161
Warning: 1 nodes with PK {151} are not in Group<test1>.
Are you sure you want to move 1 nodes from Group<test1> to Group<test2>? [y/N]: When I try to move three nodes, two which is in $ verdi group move-nodes -s test1 -t test2 161 151 159
Warning: 1 nodes with PK {151} are not in Group<test1>.
Warning: 1 nodes with PK {159} are already in Group<test2>. These will still be removed from Group<test1>.
Are you sure you want to move 2 nodes from Group<test1> to Group<test2>? [y/N]: When I try to move no nodes at all: $ verdi group move-nodes -s test1 -t test2
Critical: No nodes identifiers were provided. When I try to move the nodes to the same target group as the source group: $ verdi group move-nodes -s test1 -t test1 161
Critical: Source and target group are the same: Group<test1> When I come to my senses, and try to move two nodes which are in $ verdi group move-nodes -s test1 -t test2 161 157
Are you sure you want to move 2 nodes from Group<test1> to Group<test2>? [y/N]: |
fcc7ba3
to
7bef6a3
Compare
@ramirezfranciscof I've updated the PR in line with with the changes that were made for the To come back to your original worry about the I've also added @yakutovicha as a reviewer, since he actually requested to be a reviewer for the |
Codecov Report
@@ Coverage Diff @@
## develop #4428 +/- ##
===========================================
+ Coverage 82.11% 82.13% +0.02%
===========================================
Files 533 533
Lines 38432 38470 +38
===========================================
+ Hits 31554 31592 +38
Misses 6878 6878
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @mbercx, I think you did a great job. I also agree with the logic you've come up with to make the moving process as safe as possible: first, decide what to do and only then do it.
I have to admit though, that unrelated to this PR we have to add some functionality to the groups to simplify the code and to make things faster:
- In principle, we don't need to load nodes when adding them to the group, but we can't use the
pk
orUUID
to reference the node. I made an issue some years ago about that (Group.add_nodes() function should be able to get a list of 'pk' or 'uuid' as the argument. #1320). - We should be able to directly get the pks of the nodes that belong to a group. I made an issue some seconds ago about that (Implement
group.node_pks
#4755)
Thanks for the review @yakutovicha! I agree with both your points, and would still add that perhaps there should be a click option that just passes the identifiers without loading the nodes first, removing the need for e.g. this line: node_pks = [node.pk for node in nodes] Then we can execute these group commands and their checks without ever having to load the nodes. However, somewhere there should be a check that the identifiers do indeed correspond to nodes. If it's ok for you, I'll keep the slow version for now, and open an issue regarding improving the speed of these commands that refers to your issues, which we will solve for v2.0.0. |
Definitely. From my side, the PR is good to be merged. Should I approve or do we want @ramirezfranciscof to have a look as well? |
provided that the pre-commit issue is fixed, of course :) |
For the record, that pre-commit issue is not from files changed in this PR. :) But I still have to add more tests anyways, so I'll look into adding some pylint disables for those |
just investigating |
see #4756 |
b128aaf
to
f278aac
Compare
verdi group move-nodes
commandverdi group move-nodes
command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mbercx , concept is looking good, just have some comments about the implementation. Also, it would be good to have some better test coverage once we agree on the implementation and features.
Thanks @sphuber! I've added the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mbercx some final requests to the behavior.
I'm ok with this now (although I still find the "failing in the middle of the remove_nodes
/add_nodes
problematic, I don't really have a good solution either xD), so I'll leave the final aproval to @sphuber.
Add an extra subcommand `move-nodes` to `verdi group` that moves nodes from a source group to a target group, instead of having to use the `remove-nodes` and `add-nodes` subcommands in order to achieve this. Similar to the `remove-nodes` and `add-nodes` commands, the move can be made without prompts by adding the `--force` flag, and the nodes are presented as a list of arguments. Add corresponding test method to the `TestVerdiGroup` class of the test_group.py suite.
Here we add several warnings and failures depending on the provided input: Warnings: * If some of the specified nodes aren't in the source group. * If some of the specified nodes are in the target group. Critical Failures: * If the source and target group are the same. * If no node identifiers are provided to move. * If none of the specified nodes are in the source group. All of these can be skipped by using the `--force` flag.
for more information, see https://pre-commit.ci
aa73e5e
to
d1d98c9
Compare
Thanks for the comments @sphuber! I agree with the revised logic, I think originally I was for some reason very focused on Here is an updated example of the usage: For two groups: $ verdi group list -C
Report: To show groups of all types, use the `-a/--all` option.
PK Label Type string User Node count
---- ----------- ------------- ---------------- ------------
2 dummygroup1 core mbercx@gmail.com 2
3 dummygroup2 core mbercx@gmail.com 2 Which each contain two nodes: $ verdi group show 2
----------------- ----------------
Group label dummygroup1
Group type_string core
Group description <no description>
----------------- ----------------
# Nodes:
PK Type Created
---- --------------- --------------
279 CalculationNode 3h:30m:37s ago
280 Int 3h:30m:37s ago
$ verdi group show 3
----------------- ----------------
Group label dummygroup2
Group type_string core
Group description <no description>
----------------- ----------------
# Nodes:
PK Type Created
---- ------ --------------
280 Int 3h:30m:40s ago
281 Bool 3h:30m:40s ago Trying to move nodes to the same group: $ verdi group move-nodes -s dummygroup1 -t dummygroup1
Critical: Source and target group are the same: Group<dummygroup1>. Not specifying any nodes or $ verdi group move-nodes -s dummygroup1 -t dummygroup2
Critical: Neither NODES or the `-a, --all` option was specified. No nodes you want to move aren't in the source group: $ verdi group move-nodes -s dummygroup1 -t dummygroup2 281
Critical: None of the specified nodes are in Group<dummygroup1>. Some nodes you want to move aren't in the source group: $ verdi group move-nodes -s dummygroup1 -t dummygroup2 1 2 3 279
Warning: 3 nodes with PK {1, 2, 3} are not in Group<dummygroup1>.
Are you sure you want to move 1 nodes from Group<dummygroup1> to Group<dummygroup2>? [y/N]: The node you want to move is in the source group, but also already in the target group: $ verdi group move-nodes -s dummygroup1 -t dummygroup2 280
Warning: 1 nodes with PK {280} are already in Group<dummygroup2>. These will still be removed from Group<dummygroup1>.
Are you sure you want to move 1 nodes from Group<dummygroup1> to Group<dummygroup2>? [y/N]: Mixing it up: $ verdi group move-nodes -s dummygroup1 -t dummygroup2 1 2 3 280 279
Warning: 3 nodes with PK {1, 2, 3} are not in Group<dummygroup1>.
Warning: 1 nodes with PK {280} are already in Group<dummygroup2>. These will still be removed from Group<dummygroup1>.
Are you sure you want to move 2 nodes from Group<dummygroup1> to Group<dummygroup2>? [y/N]: |
Fixes #4426.
Add an extra subcommand
move-nodes
toverdi group
that moves nodesfrom a source group to a target group, instead of having to use the
remove-nodes
andadd-nodes
subcommands in order to achieve this.Similar to the
remove-nodes
andadd-nodes
commands, the move can bemade without prompts by adding the
--force
flag, and the nodes arepresented as a list of arguments.
Add corresponding test method to the
TestVerdiGroup
class of thetest_group.py suite.