tdviet / fedcloudclient

EGI FedCloud Client
https://fedcloudclient.fedcloud.eu
MIT License
8 stars 10 forks source link

Add a --all-sites option instead of using ALL_SITES #58

Closed enolfc closed 3 years ago

enolfc commented 3 years ago

The --site ALL_SITES is not very intuitive and not properly documented. It's easy to forget after some time without using it, so better be explicit with something like --all-sites option that avoids mispellings or confusions

tdviet commented 3 years ago

We could improve all-site options in several ways (or all):

The first two ones can be quickly added, the third may need little changes in parameter checking.

thebe14 commented 3 years ago

I added the magic value "ALL_SITES" to the help text in PR #60

thebe14 commented 3 years ago

Option 2 is indeed risky, so unless we will have confirmation for commands that change/delete stuff when all sites are targeted (needs work in all modules), I would avoid this. But maybe we can create a new issue/enhancement that proposes this together with the checks on all non-informative commands.

Option 3 is a design/usability nightmare. Two options that control the same setting, you must make them work exclusively, etc. I would avoid this too.

enolfc commented 3 years ago

Option 3 does require some extra work for the parameter checking (we are doing that anyway, no?), but it would simplify checks in the long run and possible naming issues (what if a site comes named "ALL"?).

tdviet commented 3 years ago

We could use one of the libs based on click

These libs can

thebe14 commented 3 years ago

I do think EGI has a say in how sites are named and can prevent having new sites being named ALL_SITES. About the rhetorical question of whether we are checking that the site param does not select all sites for commands where this makes no sense, I added some checks to a few commands in the last PR (hopefully to all relevant commands). But we will revise/improve that in the future, I guess.

tdviet commented 3 years ago