TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 101 Pull requests 5 Projects Releases 1 Activity Actions

[#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 +455 -20
aarifullin commented 2023-07-04 13:30:09 +00:00
Member
Copy link
  • 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
Author
Member
Copy link

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
Member
Copy link

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
Member
Copy link

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
Author
Member
Copy link

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
Author
Member
Copy link

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
Member
Copy link

"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
Copy link

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
Member
Copy link

`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
Member
Copy link

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
Member
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Author
Member
Copy link

Ofc

Ofc
aarifullin commented 2023-07-11 10:37:10 +00:00
Author
Member
Copy link

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
Copy link

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
Copy link

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
Author
Member
Copy link

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 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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
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
No description provided.
Delete branch "aarifullin/frostfs-node:feature/390-tree_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?