cli: Improve chain rule managing commands #909

aarifullin · 2024-01-12T14:45:39Z

aarifullin commented

2024-01-12 14:45:39 +00:00

Introduce namespace option to pass namespace target.
Take a target for root namespace, if namespace and cid
are both empty.

* Introduce namespace option to pass namespace target. * Take a target for root namespace, if namespace and cid are both empty.

aarifullin force-pushed feat/cli-ape from e54a8edb47 to 8285cb3ac5

2024-01-30 09:36:43 +00:00

Compare

aarifullin force-pushed feat/cli-ape from 8285cb3ac5 to 506ecd555c

2024-01-30 09:41:58 +00:00

Compare

aarifullin changed title from ~~WIP: [#XX] cli: Make add-rule and list-rules recieve namespace param~~ to cli: Improve chain rule managing commands

2024-01-30 09:47:47 +00:00

aarifullin requested review from acid-ant 2024-01-30 10:00:52 +00:00

aarifullin requested review from storage-core-committers 2024-01-30 10:00:53 +00:00

aarifullin requested review from storage-core-developers 2024-01-30 10:01:00 +00:00

acid-ant approved these changes 2024-01-30 10:10:27 +00:00

aarifullin force-pushed feat/cli-ape from 506ecd555c to 0777e30836

2024-02-02 17:07:52 +00:00

Compare

aarifullin requested review from acid-ant 2024-02-02 17:31:14 +00:00

aarifullin changed title from ~~cli: Improve chain rule managing commands~~ to WIP: cli: Improve chain rule managing commands

2024-02-02 18:02:07 +00:00

aarifullin force-pushed feat/cli-ape from 0777e30836 to e2c49f4e6c

2024-02-02 18:08:35 +00:00

Compare

aarifullin changed title from ~~WIP: cli: Improve chain rule managing commands~~ to cli: Improve chain rule managing commands

2024-02-02 18:09:20 +00:00

fyrchik approved these changes 2024-02-05 07:18:54 +00:00

acid-ant approved these changes 2024-02-05 07:43:27 +00:00

dstepanov-yadro requested changes 2024-02-05 08:03:58 +00:00

cmd/frostfs-cli/modules/control/list_rules.go Outdated

													
				@ -24,0 +35,4 @@

							Type: control.ChainTarget_CONTAINER,

						}

					}

					nsStr, _ := cmd.Flags().GetString(commonflags.NSFlag)

dstepanov-yadro commented

2024-02-05 08:03:25 +00:00

I suggest requiring you to enter "root' explicitly. Because if I forgot to enter correct namespace, then root namespace will be affected.

dstepanov-yadro commented

2024-02-05 08:03:45 +00:00

@aarifullin @fyrchik what do you think?

fyrchik commented

2024-02-05 08:34:43 +00:00

Root is somewhat default namespace, but I like the idea.
Though, if I don't know anything about namespaces, it is somewhat irrelevant.
We could also print it in verbose mode.

Root is somewhat default namespace, but I like the idea. Though, if I don't know anything about namespaces, it is somewhat irrelevant. We could also print it in verbose mode.

fyrchik commented

2024-02-05 08:35:12 +00:00

Actually, I would argue, that rules on some specific namespace should not be a common scenario for a node administrator.

dstepanov-yadro commented

2024-02-05 12:31:30 +00:00

explanation:

frostfs-cli add-rule ... without namespace leads to using root namespace
frostfs-cli add-rule --namespace leads to error
frostfs-cli add-rule --namespace root leads to using root namespace

explanation: * `frostfs-cli add-rule ...` without namespace leads to using root namespace * `frostfs-cli add-rule --namespace ` leads to error * `frostfs-cli add-rule --namespace root` leads to using root namespace

aarifullin commented

2024-02-05 13:17:46 +00:00

Poster

@dstepanov-yadro

I have decided to make the usage consistent with frostfs-adm and introduced flags --target-type --target-name. That is correct because flags make the command usage more intuitive (and, again, consistent with admin util).
Also, I liked the idea suggested by @fyrchik to make an interaction confirm if you wanna use root namespace if name flag is not set.
If no one likes how it looks then I will just make --target-name flag as required and "" won't be acceptable for input

@dstepanov-yadro I have decided to make the usage consistent with `frostfs-adm` and introduced flags `--target-type --target-name`. That is correct because flags make the command usage more intuitive (and, again, consistent with admin util). Also, I liked the idea suggested by @fyrchik to make an interaction `confirm if you wanna use root namespace` if name flag is not set. If no one likes how it looks then I will just make `--target-name` flag as required and `""` won't be acceptable for input

dstepanov-yadro commented

2024-02-05 15:35:50 +00:00

Fine!

dstepanov-yadro marked this conversation as resolved

aarifullin force-pushed feat/cli-ape from e2c49f4e6c to 71dc5c257a

2024-02-05 13:12:25 +00:00

Compare

aarifullin requested review from dstepanov-yadro 2024-02-05 14:39:42 +00:00

aarifullin requested review from acid-ant 2024-02-05 14:39:42 +00:00

aarifullin requested review from fyrchik 2024-02-05 14:39:43 +00:00

dstepanov-yadro approved these changes 2024-02-05 15:32:53 +00:00

acid-ant approved these changes 2024-02-06 08:08:26 +00:00