cli: Don't print help and don't skip `defer`ed funcs on failures #2942

End-rey · 2024-09-19T08:34:30Z

In neofs-cli I add func common.WrapError that wrap error to custom_errors.ExitErr and return it on every error, But I doubt it. Maybe it is necessary to wrap only the first appearance of this error, or vice versa, only the last one.

codecov · 2024-09-19T08:37:11Z

Codecov Report

Attention: Patch coverage is 0% with 1433 lines in your changes missing coverage. Please review.

Project coverage is 23.53%. Comparing base (a0447ce) to head (23c2dce).

Files with missing lines	Patch %	Lines
cmd/neofs-adm/internal/modules/storagecfg/root.go	0.00%	106 Missing ⚠️
cmd/neofs-cli/modules/object/util.go	0.00%	88 Missing ⚠️
cmd/neofs-cli/modules/object/range.go	0.00%	35 Missing ⚠️
cmd/neofs-cli/modules/container/create.go	0.00%	31 Missing ⚠️
cmd/neofs-cli/modules/control/shards_set_mode.go	0.00%	31 Missing ⚠️
cmd/neofs-cli/modules/object/put.go	0.00%	30 Missing ⚠️
cmd/neofs-cli/modules/container/get.go	0.00%	29 Missing ⚠️
cmd/neofs-cli/modules/container/set_eacl.go	0.00%	29 Missing ⚠️
cmd/neofs-cli/modules/storagegroup/put.go	0.00%	29 Missing ⚠️
cmd/neofs-cli/modules/object/get.go	0.00%	28 Missing ⚠️
... and 79 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2942      +/-   ##
==========================================
- Coverage   23.88%   23.53%   -0.35%     
==========================================
  Files         776      776              
  Lines       45893    46572     +679     
==========================================
  Hits        10961    10961              
- Misses      34070    34749     +679     
  Partials      862      862

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

roman-khimov

Conflicts, rebase pls. But looks nice in general. Probably we can add some tests for it as well.

roman-khimov · 2024-09-19T19:22:12Z

cmd/neofs-cli/internal/common/exit.go

 //
 //	0 if nil
 //	1 if [sdkstatus.ErrServerInternal] or untyped
 //	2 if [sdkstatus.ErrObjectAccessDenied]
 //	3 if [ErrAwaitTimeout]
-func ExitOnErr(cmd *cobra.Command, errFmt string, err error) {
+func WrapError(errFmt string, err error) *custom_errors.ExitErr {


I'd probably do it as WrapError(err error) and leave fmt.Errorf() to the caller if and when needed. You still can unwrap internally to get the code, but formatting would be easier, I think. You can have cases like "smth (additional data: %d): %w" and this won't fit the current interface.

roman-khimov · 2024-09-19T19:23:50Z

CHANGELOG.md

@@ -9,9 +9,11 @@ Changelog for NeoFS Node
 - Expose health status of inner ring via Prometheus (#2934)

 ### Fixed
+- `defer` func is not skipped in cobra-based programs (#2942)


Not sure it makes sense for outside reader, but don't have immediate suggestions either.

cthulhu-rider

in total, lux gut

afaik #623 can be resolved purely independent, so its better to dedicate a separate small (last) commit for it to not mix with heavy #2890.

also tag intermediate commits with Refs #2890.

cthulhu-rider · 2024-09-20T05:49:04Z

cmd/internal/custom_errors/errors.go

@@ -0,0 +1,27 @@
+package custom_errors


not a good choice of pkg name https://go.dev/blog/package-names. I can suggest cmderr

cthulhu-rider · 2024-09-20T06:02:38Z

cmd/neofs-adm/internal/modules/storagecfg/root.go

-			if getConfirmation(false, "Continue configuration? yes/[no]: ") {
-				return
+			if flag, err := getConfirmation(false, "Continue configuration? yes/[no]: "); flag {
+				if err != nil {


this never happens for flag==true, i think the condition should be

if flag, err := getConfirmation(false, "Continue configuration? yes/[no]: "); flag || err != nil { return err }

cthulhu-rider · 2024-09-20T06:24:01Z

cmd/neofs-cli/internal/common/exit.go

 //
 //	0 if nil
 //	1 if [sdkstatus.ErrServerInternal] or untyped
 //	2 if [sdkstatus.ErrObjectAccessDenied]
 //	3 if [ErrAwaitTimeout]
-func ExitOnErr(cmd *cobra.Command, errFmt string, err error) {
+func WrapError(errFmt string, err error) *custom_errors.ExitErr {


it should not return pointers to be compliant with ExitOnErr making

var e ExitErr if !errors.As(err, &e) { e.Code = 1 }

easy to check

func TestExit(t *testing.T) { custom_errors.ExitOnErr(common.WrapError("context; %w", common.ErrAwaitTimeout)) }

expected 3, got 1

cthulhu-rider · 2024-09-20T06:27:11Z

cmd/neofs-cli/internal/client/sdk.go

 	cli, err := getSDKClientByFlag(ctx, endpointFlag)
 	if err != nil {
-		common.ExitOnErr(cmd, "can't create API client: %w", err)
+		return nil, common.WrapError("can't create API client: %w", err)


i dont think this is a good place to throw ExitErr, this is the responsibility of RunEs. So, just return nil, fmt.Errorf(...)

cthulhu-rider · 2024-09-20T06:28:48Z

cmd/neofs-cli/internal/common/exit.go

+	}
+
+	var exitErr *custom_errors.ExitErr
+	if errors.As(err, &exitErr) {


having this means that code does not care who makes ExitErr while it should. Imo normally this should never be true. The most stable approach is to wrap by RunE functions only

cthulhu-rider · 2024-09-20T06:31:11Z

cmd/neofs-cli/internal/common/eacl.go

 	_, err := os.Stat(eaclPath) // check if `eaclPath` is an existing file
 	if err != nil {
-		ExitOnErr(cmd, "", errors.New("incorrect path to file with EACL"))
+		return eacl.Table{}, WrapError("", errors.New("incorrect path to file with EACL"))


here too enough to return eacl.Table{}, errors.New("incorrect path to file with EACL") and let caller how to wrap it. With this, u wont need to wrap all function returns

cthulhu-rider · 2024-09-20T06:39:32Z

cmd/neofs-lens/internal/meta/get.go

 	var addr oid.Address

 	err := addr.DecodeString(vAddress)
-	common.ExitOnErr(cmd, common.Errf("invalid address argument: %w", err))
+	if err != nil {
+		return common.Errf("invalid address argument: %w", err)


common.Errf made sense with ExitOnErr func since it allowed to avoid if statement. Now we should use fmt.Errorf (and purge Errf)

For all cobra-based programs add custom error with code and cause of error and exit func from programs. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

Use `RunE` instead of `Run` and throw an error to main function. Then handle error with custom error handler. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

Use `RunE` instead of `Run` and throw an error to main function. Wrap error with `common.WrapError` to specify the status code. Then handle error with custom error handler. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

Use `RunE` instead of `Run` and throw an error to main function. Then handle error with custom error handler. Update CHANGELOG.md. Closes #2890. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

Closes #623. Refs #2890. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

End-rey · 2024-09-23T16:25:23Z

Now, in the neofs-cli, func common.WrapError is used after receiving the cobra command error in main.

carpawell · 2024-09-23T18:47:21Z

cmd/internal/cmderr/errors.go

+	"os"
+)
+
+type ExitErr struct {


an exported type, should be documented according to ancient chronicles

End-rey requested review from roman-khimov, carpawell and cthulhu-rider as code owners September 19, 2024 08:34

End-rey force-pushed the 623-do-not-print-help-on-any-failure branch from 85bf87d to 24ab511 Compare September 19, 2024 08:54

roman-khimov reviewed Sep 19, 2024

View reviewed changes

cthulhu-rider requested changes Sep 20, 2024

View reviewed changes

End-rey marked this pull request as draft September 20, 2024 07:02

End-rey force-pushed the 623-do-not-print-help-on-any-failure branch from 24ab511 to b99e708 Compare September 23, 2024 16:03

End-rey added 5 commits September 23, 2024 19:04

cmd: add custom error and exit func from program

cd33d41

For all cobra-based programs add custom error with code and cause of error and exit func from programs. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

cmd/neofs-adm: add error handler after execution command

f4abca2

Use `RunE` instead of `Run` and throw an error to main function. Then handle error with custom error handler. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

cmd/neofs-cli: add error handler after execution command

8f39085

Use `RunE` instead of `Run` and throw an error to main function. Wrap error with `common.WrapError` to specify the status code. Then handle error with custom error handler. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

cmd/neofs-lens: add error handler after execution command

44dc1ac

Use `RunE` instead of `Run` and throw an error to main function. Then handle error with custom error handler. Update CHANGELOG.md. Closes #2890. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

cmd: silence errors and usage for cobra commands

23c2dce

Closes #623. Refs #2890. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

End-rey force-pushed the 623-do-not-print-help-on-any-failure branch from b99e708 to 23c2dce Compare September 23, 2024 16:05

End-rey marked this pull request as ready for review September 23, 2024 16:17

carpawell reviewed Sep 23, 2024

View reviewed changes

cmd/internal/cmderr/errors.go

"os"

)

type ExitErr struct {

Copy link

Member

carpawell Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an exported type, should be documented according to ancient chronicles

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: Don't print help and don't skip `defer`ed funcs on failures #2942

cli: Don't print help and don't skip `defer`ed funcs on failures #2942

End-rey commented Sep 19, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

roman-khimov left a comment

roman-khimov Sep 19, 2024

roman-khimov Sep 19, 2024

cthulhu-rider left a comment

cthulhu-rider Sep 20, 2024

cthulhu-rider Sep 20, 2024

cthulhu-rider Sep 20, 2024

cthulhu-rider Sep 20, 2024

cthulhu-rider Sep 20, 2024

cthulhu-rider Sep 20, 2024

cthulhu-rider Sep 20, 2024

End-rey commented Sep 23, 2024

carpawell Sep 23, 2024

cli: Don't print help and don't skip defered funcs on failures #2942

Are you sure you want to change the base?

cli: Don't print help and don't skip defered funcs on failures #2942

Conversation

End-rey commented Sep 19, 2024

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

roman-khimov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

End-rey commented Sep 23, 2024

Choose a reason for hiding this comment

cli: Don't print help and don't skip `defer`ed funcs on failures #2942

cli: Don't print help and don't skip `defer`ed funcs on failures #2942

codecov bot commented Sep 19, 2024 •

edited

Loading