TrueCloudLab
/
frostfs-node
18
1
Fork 20
Code Issues 88 Pull Requests 8 Actions Releases Activity

[#390] cli: Support more tree service API calls in CLI #488

Merged
fyrchik merged 3 commits from aarifullin/frostfs-node:feature/390-tree_cli into master 2023-07-11 14:11:31 +00:00
Conversation 2 Commits 3 Files Changed 10
aarifullin commented 2023-07-04 13:30:09 +00:00
Collaborator
  • Support healthcheck API call
  • Support move API call
  • Support remove API call
  • Support getSubtree API call
  • Fix reading bearer token in other commands

Signed-off-by: Airat Arifullin a.arifullin@yadro.com

* Support healthcheck API call * Support move API call * Support remove API call * Support getSubtree API call * Fix reading bearer token in other commands Signed-off-by: Airat Arifullin a.arifullin@yadro.com
aarifullin commented 2023-07-04 13:34:15 +00:00
Poster
Collaborator

In the issue @alexvanin said

There are two more which are not used by the clients, so reconsider adding them in the CLI.
Apply
GetOpLog

I am not sure that someone can use these API calls with clear intention. I think the only reason is to disable sync service and debug something.
Let's discuss here if really need these methods. If it's necessary, I will add two more command

In the issue @alexvanin said > There are two more which are not used by the clients, so reconsider adding them in the CLI. Apply GetOpLog I am not sure that someone can use these API calls with clear intention. I think the only reason is to disable `sync` service and debug something. Let's discuss here if really need these methods. If it's necessary, I will add two more command
aarifullin requested review from storage-core-committers 2023-07-04 13:34:40 +00:00
aarifullin requested review from storage-core-developers 2023-07-04 13:34:40 +00:00
aarifullin force-pushed feature/390-tree_cli from 8135e79e9e to 1a8b0db078 2023-07-04 13:35:39 +00:00 Compare
ale64bit approved these changes 2023-07-04 13:47:35 +00:00
cmd/frostfs-cli/modules/tree/healthcheck.go Outdated
@ -0,0 +10,4 @@
var healthcheckCmd = &cobra.Command{
Use: "healthcheck",
Short: "Move node",
ale64bit commented 2023-07-04 13:41:37 +00:00
Collaborator

copy pasta

copy pasta
👍 1
ale64bit marked this conversation as resolved
cmd/frostfs-cli/modules/tree/move.go Outdated
@ -0,0 +73,4 @@
}
commonCmd.ExitOnErr(cmd, "message signing: %w", tree.SignMessage(subTreeReq, pk))
resp, err := cli.GetSubTree(ctx, subTreeReq)
commonCmd.ExitOnErr(cmd, "rpc call: %w", err)
ale64bit commented 2023-07-04 13:44:49 +00:00
Collaborator

it's harder to understand what went wrong if all errors say "rpc call: %w". Maybe use more specific error messages? e.g. "getting subtree: %v" and so on.

it's harder to understand what went wrong if all errors say `"rpc call: %w"`. Maybe use more specific error messages? e.g. `"getting subtree: %v"` and so on.
aarifullin commented 2023-07-04 14:14:43 +00:00
Poster
Collaborator

Okay. I'll make these errors more verbose 👍

Okay. I'll make these errors more verbose 👍
aarifullin commented 2023-07-05 09:36:53 +00:00
Poster
Collaborator

Fixed

Fixed
ale64bit marked this conversation as resolved
cmd/frostfs-cli/modules/tree/move.go Outdated
@ -0,0 +99,4 @@
},
}
commonCmd.ExitOnErr(cmd, "message signing: %w", tree.SignMessage(req, pk))
ale64bit commented 2023-07-04 13:43:33 +00:00
Collaborator

"signing message: %w"?

`"signing message: %w"`?
ale64bit marked this conversation as resolved
aarifullin force-pushed feature/390-tree_cli from 1a8b0db078 to 45be78f281 2023-07-04 13:54:02 +00:00 Compare
fyrchik commented 2023-07-04 14:24:47 +00:00
Owner

Apply is not needed indeed.
GetOpLog can be useful for debugging (as an example -- check whether a particular log operation is in tree)

`Apply` is not needed indeed. `GetOpLog` can be useful for debugging (as an example -- check whether a particular log operation is in tree)
👍 1
aarifullin force-pushed feature/390-tree_cli from 45be78f281 to d99960021d 2023-07-05 08:24:43 +00:00 Compare
aarifullin requested review from ale64bit 2023-07-05 08:26:46 +00:00
dstepanov-yadro reviewed 2023-07-05 13:45:30 +00:00
cmd/frostfs-cli/modules/tree/get_op_log.go Outdated
@ -0,0 +46,4 @@
ctx := cmd.Context()
cli, err := _client(ctx)
commonCmd.ExitOnErr(cmd, "client: %w", err)
dstepanov-yadro commented 2023-07-05 13:45:30 +00:00
Collaborator

`failed to create clietn' will be better, I think.

`failed to create clietn' will be better, I think.
👍 1
dstepanov-yadro approved these changes 2023-07-05 13:46:15 +00:00
acid-ant approved these changes 2023-07-06 10:48:09 +00:00
aarifullin force-pushed feature/390-tree_cli from d99960021d to 4a48ca6cb3 2023-07-06 15:20:56 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2023-07-06 15:21:06 +00:00
aarifullin requested review from acid-ant 2023-07-06 15:21:09 +00:00
acid-ant approved these changes 2023-07-07 06:04:51 +00:00
ale64bit approved these changes 2023-07-07 07:09:15 +00:00
dstepanov-yadro reviewed 2023-07-07 07:18:37 +00:00
cmd/frostfs-cli/modules/tree/add.go Outdated
@ -65,3 +70,3 @@
}
commonCmd.ExitOnErr(cmd, "message signing: %w", tree.SignMessage(req, pk))
commonCmd.ExitOnErr(cmd, "signing message: %w", tree.SignMessage(req, pk))
dstepanov-yadro commented 2023-07-07 07:18:37 +00:00
Collaborator

signing message -> failed to sign request and so on. I think error messages must be clear for user, not only for developer. Or am I bothering too much? @fyrchik what do you think?

`signing message` -> `failed to sign request` and so on. I think error messages must be clear for user, not only for developer. Or am I bothering too much? @fyrchik what do you think?
ale64bit commented 2023-07-07 08:23:12 +00:00
Collaborator

IMHO, it's shorter/easier to structure it this way (i.e. present/past continuous form: actual error). This usually matches well and intuitively looks like what was happening: what went wrong while doing that.

IMHO, it's shorter/easier to structure it this way (i.e. present/past continuous form: actual error). This usually matches well and intuitively looks like `what was happening: what went wrong while doing that`.
dstepanov-yadro approved these changes 2023-07-07 08:57:15 +00:00
fyrchik requested changes 2023-07-11 07:25:57 +00:00
cmd/frostfs-cli/modules/tree/add.go Outdated
@ -51,3 +51,3 @@
cli, err := _client(ctx)
commonCmd.ExitOnErr(cmd, "client: %w", err)
commonCmd.ExitOnErr(cmd, "failed to create client: %w", err)
fyrchik commented 2023-07-11 07:24:09 +00:00
Owner

Unrelated to the commit.

Unrelated to the commit.
cmd/frostfs-cli/modules/tree/add.go Outdated
@ -55,3 +55,4 @@
rawCID := make([]byte, sha256.Size)
cnr.Encode(rawCID)
var bt []byte
fyrchik commented 2023-07-11 07:25:07 +00:00
Owner

This also seems like a separate commit with bearer addition to existing methods.

This also seems like a separate commit with bearer addition to existing methods.
cmd/frostfs-cli/modules/tree/add.go Outdated
@ -68,3 +73,3 @@
resp, err := cli.Add(ctx, req)
commonCmd.ExitOnErr(cmd, "rpc call: %w", err)
commonCmd.ExitOnErr(cmd, "add: %w", err)
fyrchik commented 2023-07-11 07:24:44 +00:00
Owner

Please, could you move all wording-only changes to a separate commit?

Please, could you move all wording-only changes to a separate commit?
aarifullin commented 2023-07-11 09:17:29 +00:00
Poster
Collaborator

Ofc

Ofc
aarifullin commented 2023-07-11 10:37:10 +00:00
Poster
Collaborator

I have splitted the PR into 3 commits. Check this out, please!

I have splitted the PR into 3 commits. Check this out, please!
aarifullin force-pushed feature/390-tree_cli from 4a48ca6cb3 to 6ce35c0a2d 2023-07-11 10:36:04 +00:00 Compare
fyrchik approved these changes 2023-07-11 10:48:03 +00:00
cmd/frostfs-cli/modules/tree/move.go Outdated
@ -0,0 +103,4 @@
_, err = cli.Move(ctx, req)
commonCmd.ExitOnErr(cmd, "failed to call move: %w", err)
cmd.Println("Success.")
fyrchik commented 2023-07-11 10:48:00 +00:00
Owner

Maybe common.PrintVerbose?

Maybe `common.PrintVerbose`?
cmd/frostfs-cli/modules/tree/root.go Outdated
@ -26,2 +36,4 @@
treeIDFlagKey = "tid"
parentIDFlagKey = "pid"
nodeIDFlagKey = "nid"
rootIDFlagKey = "rid"
fyrchik commented 2023-07-11 10:46:21 +00:00
Owner

What about --root? It is easy to get lost in 3-letter words.

What about `--root`? It is easy to get lost in 3-letter words.
aarifullin commented 2023-07-11 12:14:32 +00:00
Poster
Collaborator

Good point

Good point
fyrchik marked this conversation as resolved
aarifullin force-pushed feature/390-tree_cli from 6ce35c0a2d to d92569f2df 2023-07-11 12:35:13 +00:00 Compare
aarifullin requested review from ale64bit 2023-07-11 12:36:07 +00:00
aarifullin requested review from dstepanov-yadro 2023-07-11 12:36:07 +00:00
aarifullin requested review from fyrchik 2023-07-11 12:36:08 +00:00
fyrchik approved these changes 2023-07-11 14:11:24 +00:00
fyrchik merged commit 0754e6e654 into master 2023-07-11 14:11:31 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
ale64bit
dstepanov-yadro
fyrchik
Labels
Clear labels   
P0

Highest   
P1

High   
P2

Medium   
P3

Low   
badger

Badger DB related issues   
frostfs-adm
   
frostfs-cli
   
frostfs-ir
   
frostfs-lens
   
frostfs-node
   
good first issue

Good for newcomers   
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff   
blocked

The issue or PR is blocked by something   
bug

Something is not working   
config

Configuration format update or breaking change   
discussion

Talking about something in order to reach a decision or to exchange ideas   
documentation

Documentation related   
duplicate

This issue or pull request already exists   
enhancement

New feature   
go

Language features adoption   
help wanted

Extra attention is needed   
internal
   
invalid

Something is wrong   
kludge

This is a kludge and we know it   
observability

Related to metrics, tracing and logs   
perfomance

Perfomance related   
question

More information is needed   
refactoring

Refactoring   
wontfix

This won't be fixed
No Label
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
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
5 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#488
There is no content yet.
Delete Branch "aarifullin/frostfs-node:feature/390-tree_cli"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?