Refactor APE-related commands #1501

Merged
fyrchik merged 8 commits from aarifullin/frostfs-node:feat/refactor_ape_cli into master 2024-11-20 07:58:36 +00:00
Member
  • Converter from eACL to APE has been moved to pkg/util to make it importable from frostfs-node
  • Parsing methods have been moved to pkg/util as they can be useful out of frostfs-node
  • A common package for commands has been introduced: cmd/internal/common/ape. The introduced methods can be used both for frostfs-cli and frostfs-adm commands. A noticeable difference between frostfs-adm and frostfs-cli adding commands: chain-id is required (MarkAsRequired) in frosfs-adm while this flag is optional in frostfs-cli.
  • Since, these commands and flags are used over all ape-related subcommands: frostfs-adm morph ape, frostfs-cli control, frostfs-cli ape-manager, bearer.
* Converter from eACL to APE has been moved to `pkg/util` to make it importable from `frostfs-node` * Parsing methods have been moved to `pkg/util` as they can be useful out of `frostfs-node` * A common package for commands has been introduced: `cmd/internal/common/ape`. The introduced methods can be used both for `frostfs-cli` and `frostfs-adm` commands. A noticeable difference between `frostfs-adm` and `frostfs-cli` adding commands: `chain-id` is required (`MarkAsRequired`) in `frosfs-adm` while this flag is optional in `frostfs-cli`. * Since, these commands and flags are used over all ape-related subcommands: `frostfs-adm morph ape`, `frostfs-cli control`, `frostfs-cli ape-manager`, `bearer`.
aarifullin added the
frostfs-adm
frostfs-cli
refactoring
labels 2024-11-15 12:36:17 +00:00
aarifullin added 1 commit 2024-11-15 12:36:18 +00:00
[#XX] cli: Refactor APE-related commands
Some checks failed
Tests and linters / Run gofumpt (pull_request) Failing after 2m13s
DCO action / DCO (pull_request) Failing after 2m34s
Tests and linters / Staticcheck (pull_request) Successful in 3m47s
Vulncheck / Vulncheck (pull_request) Successful in 3m49s
Tests and linters / gopls check (pull_request) Successful in 4m8s
Pre-commit hooks / Pre-commit (pull_request) Successful in 4m50s
Build / Build Components (pull_request) Successful in 4m56s
Tests and linters / Lint (pull_request) Successful in 5m6s
Tests and linters / Tests (pull_request) Successful in 6m56s
Tests and linters / Tests with -race (pull_request) Successful in 7m2s
9df5ecc2ee
* Move common rule parsing logic to a common package that
  also can be imported out of `frostfs-node`;
* Move common flags and commands to `cmd/internal/common/ape` package to
  avoid duplication. Since `frostfs-adm` and `frostfs-cli` subcommands
  are able to use commands, thee same flag names and their descriptions.

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>
aarifullin requested review from storage-core-committers 2024-11-15 12:36:50 +00:00
aarifullin force-pushed feat/refactor_ape_cli from 9df5ecc2ee to c00222c491 2024-11-15 12:36:52 +00:00 Compare
aarifullin requested review from storage-core-developers 2024-11-15 12:38:32 +00:00
aarifullin force-pushed feat/refactor_ape_cli from c00222c491 to 8a465b74ad 2024-11-15 13:00:01 +00:00 Compare
fyrchik requested changes 2024-11-15 13:10:46 +00:00
Dismissed
fyrchik left a comment
Owner

Some of the changes are useful (reusing flag names, reusing constants from policy engine).
Please, split them in separate commits.

Firm NACK on the whole function moving thing.

Some of the changes are useful (reusing flag names, reusing constants from policy engine). Please, split them in separate commits. Firm NACK on the whole function moving thing.
@ -0,0 +74,4 @@
// ParseTarget handles target parsing of an APE chain. Names are optional for certain
// target types. When prompt is enabled, the parser will request confirmation
// for empty inputs.
func ParseTarget(cmd *cobra.Command, promptRequired bool) engine.Target {
Owner

This looks like a bad idea: you have taken two different functions and created a new one with a bool flag which switches the behaviour. The amount of different code between these functions is comparable to their size. And they were not that long before.
Let's leave them as they were.

This looks like a bad idea: you have taken two different functions and created a new one with a bool flag which switches the behaviour. The amount of different code between these functions is comparable to their size. And they were not that long before. Let's leave them as they were.
Author
Member

Now I am wondering: it's fine, if we omit this flag but leave prompt anyway. Prompt is used in frostfs-cli. frostfs-adm also correctly receives and processes --target-name root, but ignores empty input (--target-name "" is taken for fine input). I think that won't be wrong if frostfs-adm will also output this prompt. AFAIR, autotests use only --target-name root

Now I am wondering: it's fine, if we omit this flag but leave prompt anyway. Prompt is used in `frostfs-cli`. `frostfs-adm` also correctly receives and processes `--target-name root`, but ignores empty input (`--target-name ""` is taken for fine input). I think that won't be wrong if `frostfs-adm` will also output this prompt. AFAIR, autotests use only `--target-name root`
@ -0,0 +120,4 @@
return engine.TargetType(0)
}
func wasMarkedAsRequired(cmd *cobra.Command, flagName string) bool {
Owner

This is very dirty: you use some obscure cobra internal in order to be able to use a single function from 2 places. Just copy the function, no problem.
And again, it questions the benefit of this refactoring.

This is very dirty: you use some obscure cobra internal in order to be able to use a single function from 2 places. Just copy the function, no problem. And again, it questions the benefit of this refactoring.
Author
Member

Just copy the function, no problem

Okay. Also, as flags are already marked Required, we don't need this extra checks at all - user will receive error from subcommands anyway

> Just copy the function, no problem Okay. Also, as flags are already marked **Required**, we don't need this extra checks at all - user will receive error from subcommands anyway
aarifullin force-pushed feat/refactor_ape_cli from 8a465b74ad to fdbeaeba7b 2024-11-15 14:35:10 +00:00 Compare
Author
Member

Firm NACK on the whole function moving thing.

Please, clarify which functions you don't approve to move

> Firm NACK on the whole function moving thing. Please, clarify which functions you don't approve to move
Owner

Please, clarify which functions you don't approve to move

I won't bother reviewing the PR again until it is split into commits properly, so that it is feasible to review.

>Please, clarify which functions you don't approve to move I won't bother reviewing the PR again until it is split into commits properly, so that it is feasible to review.
fyrchik changed title from Refactor APE-related commands to WIP: Refactor APE-related commands 2024-11-16 05:55:20 +00:00
aarifullin force-pushed feat/refactor_ape_cli from fdbeaeba7b to a2d592a610 2024-11-18 11:20:52 +00:00 Compare
Author
Member

Please, clarify which functions you don't approve to move

I won't bother reviewing the PR again until it is split into commits properly, so that it is feasible to review.

Please, check this out:

  1. I have spitted the PR into a few commits
  2. I have removed these extra boolean parameters and trick with required

Also, refer to PR description - I've written more details

> >Please, clarify which functions you don't approve to move > > I won't bother reviewing the PR again until it is split into commits properly, so that it is feasible to review. Please, check this out: 1. I have spitted the PR into a few commits 2. I have removed these extra boolean parameters and trick with `required` Also, refer to PR description - I've written more details
aarifullin changed title from WIP: Refactor APE-related commands to Refactor APE-related commands 2024-11-18 11:35:25 +00:00
aarifullin requested review from fyrchik 2024-11-18 11:44:09 +00:00
dstepanov-yadro approved these changes 2024-11-18 13:48:36 +00:00
Dismissed
aarifullin changed title from Refactor APE-related commands to WIP: Refactor APE-related commands 2024-11-19 09:32:28 +00:00
aarifullin changed title from WIP: Refactor APE-related commands to Refactor APE-related commands 2024-11-19 09:32:32 +00:00
aarifullin requested review from dstepanov-yadro 2024-11-19 09:32:39 +00:00
fyrchik reviewed 2024-11-19 13:59:34 +00:00
@ -83,0 +32,4 @@
target := apeCmd.ParseTarget(cmd)
typ, ok := engineToControlSvcType[target.Type]
if !ok {
Owner

ParseTargetType exists if target doesn't exist, so this condition is always false, right?

`ParseTargetType` exists if target doesn't exist, so this condition is always false, right?
Author
Member

I assume that we can add a new target in policy-engine parsable by apeCmd.ParseTarget but not known for control service yet. In this case, we take this for unknown target

I assume that we can add a new target in `policy-engine` parsable by `apeCmd.ParseTarget` but not known for control service yet. In this case, we take this for unknown target
fyrchik marked this conversation as resolved
@ -0,0 +97,4 @@
default:
commonCmd.ExitOnErr(cmd, "read target type error: %w", errUnknownTargetType)
}
return engine.Target{}
Owner

Here too. I would also move it to default to communicate that the switch is exhaustive.

Here too. I would also move it to `default` to communicate that the switch is exhaustive.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -0,0 +114,4 @@
return engine.Group
}
commonCmd.ExitOnErr(cmd, "parse target type error: %w", errUnknownTargetType)
return engine.TargetType(0)
Owner

I think it is more idiomatic to panic with panic("unreachable").

I think it is more idiomatic to panic with `panic("unreachable")`.
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
aarifullin force-pushed feat/refactor_ape_cli from a2d592a610 to dbdaa71e0b 2024-11-19 15:25:54 +00:00 Compare
aarifullin dismissed dstepanov-yadro's review 2024-11-19 15:25:54 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

aarifullin force-pushed feat/refactor_ape_cli from dbdaa71e0b to 94ef76db43 2024-11-19 17:07:52 +00:00 Compare
aarifullin force-pushed feat/refactor_ape_cli from 94ef76db43 to 61807fd24a 2024-11-19 17:09:17 +00:00 Compare
aarifullin force-pushed feat/refactor_ape_cli from 61807fd24a to 9249333c00 2024-11-19 17:15:04 +00:00 Compare
aarifullin force-pushed feat/refactor_ape_cli from 9249333c00 to 462876291d 2024-11-19 17:23:31 +00:00 Compare
fyrchik approved these changes 2024-11-20 06:42:26 +00:00
Owner

I am not sure whether to merge this now or after we branch v0.44 release branch.
@aarifullin could you write a list of functional changes done by this PR?

I am not sure whether to merge this now or after we branch v0.44 release branch. @aarifullin could you write a list of functional changes done by this PR?
Author
Member

I am not sure whether to merge this now or after we branch v0.44 release branch.
@aarifullin could you write a list of functional changes done by this PR?

This PR makes no functional changes, only refactoring.
The only slight change I cancelled myself - the prompt for frostfs-adm, because that wasn't a good idea (parseTarget is duplicated in ape_util.go for frostfs-adm). So, nothing changed neither from functionality nor user experience

> I am not sure whether to merge this now or after we branch v0.44 release branch. > @aarifullin could you write a list of functional changes done by this PR? This PR makes no functional changes, only refactoring. The only slight change I cancelled myself - the prompt for `frostfs-adm`, because that wasn't a good idea (`parseTarget` is duplicated in `ape_util.go` for `frostfs-adm`). So, nothing changed neither from functionality nor user experience
fyrchik merged commit a339b52a60 into master 2024-11-20 07:58:36 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1501
No description provided.