Refactor APE-related commands #1501
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1501
Loading…
Reference in a new issue
No description provided.
Delete branch "aarifullin/frostfs-node:feat/refactor_ape_cli"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
pkg/util
to make it importable fromfrostfs-node
pkg/util
as they can be useful out offrostfs-node
cmd/internal/common/ape
. The introduced methods can be used both forfrostfs-cli
andfrostfs-adm
commands. A noticeable difference betweenfrostfs-adm
andfrostfs-cli
adding commands:chain-id
is required (MarkAsRequired
) infrosfs-adm
while this flag is optional infrostfs-cli
.frostfs-adm morph ape
,frostfs-cli control
,frostfs-cli ape-manager
,bearer
.9df5ecc2ee
toc00222c491
c00222c491
to8a465b74ad
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 {
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.
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 iffrostfs-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 {
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.
Okay. Also, as flags are already marked Required, we don't need this extra checks at all - user will receive error from subcommands anyway
8a465b74ad
tofdbeaeba7b
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.
Refactor APE-related commandsto WIP: Refactor APE-related commandsfdbeaeba7b
toa2d592a610
Please, check this out:
required
Also, refer to PR description - I've written more details
WIP: Refactor APE-related commandsto Refactor APE-related commandsRefactor APE-related commandsto WIP: Refactor APE-related commandsWIP: Refactor APE-related commandsto Refactor APE-related commands@ -83,0 +32,4 @@
target := apeCmd.ParseTarget(cmd)
typ, ok := engineToControlSvcType[target.Type]
if !ok {
ParseTargetType
exists if target doesn't exist, so this condition is always false, right?I assume that we can add a new target in
policy-engine
parsable byapeCmd.ParseTarget
but not known for control service yet. In this case, we take this for unknown target@ -0,0 +97,4 @@
default:
commonCmd.ExitOnErr(cmd, "read target type error: %w", errUnknownTargetType)
}
return engine.Target{}
Here too. I would also move it to
default
to communicate that the switch is exhaustive.fixed
@ -0,0 +114,4 @@
return engine.Group
}
commonCmd.ExitOnErr(cmd, "parse target type error: %w", errUnknownTargetType)
return engine.TargetType(0)
I think it is more idiomatic to panic with
panic("unreachable")
.fixed
a2d592a610
todbdaa71e0b
New commits pushed, approval review dismissed automatically according to repository settings
dbdaa71e0b
to94ef76db43
94ef76db43
to61807fd24a
61807fd24a
to9249333c00
9249333c00
to462876291d
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 inape_util.go
forfrostfs-adm
). So, nothing changed neither from functionality nor user experience