Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle client errors according to NeoFS SDK RC-9 #2468

Merged
merged 2 commits into from
Aug 8, 2023
Merged

Handle client errors according to NeoFS SDK RC-9 #2468

merged 2 commits into from
Aug 8, 2023

Conversation

cthulhu-rider
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Aug 3, 2023 •
edited
Loading

Codecov Report

Merging #2468 (0588021) into master (4a0198f) will increase coverage by 0.00%.
The diff coverage is 50.00%.

❗ Current head 0588021 differs from pull request most recent head 5387290. Consider uploading reports for the commit 5387290 to get more accurate results

@@           Coverage Diff           @@
##           master    #2468   +/-   ##
=======================================
  Coverage   29.38%   29.38%           
=======================================
  Files         399      399           
  Lines       30385    30381    -4     
=======================================
- Hits         8928     8927    -1     
+ Misses      20714    20711    -3     
  Partials      743      743
Files Changed Coverage Δ
pkg/services/object/get/util.go 14.17% <0.00%> (+0.11%) ⬆️
pkg/services/object/get/remote.go 83.87% <60.00%> (-1.43%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cthulhu-rider cthulhu-rider mentioned this pull request Aug 3, 2023
Closed
@roman-khimov
Copy link
Member

Code looks OK, but there are some problems with tests and CHANGELOG.

carpawell
carpawell reviewed Aug 4, 2023
View reviewed changes
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

group_test.go:19: 
        	Error Trace:	/home/runner/work/neofs-node/neofs-node/pkg/morph/deploy/group_test.go:19
        	Error:      	Received unexpected error:
        	            	create AES cipher block: crypto/aes: invalid key size 31
        	Test:       	TestKeySharing

Wow.

cmd/neofs-cli/internal/common/exit.go
internalErr = new(sdkstatus.ServerInternal)
accessErr = new(sdkstatus.ObjectAccessDenied)
)
var code int
Copy link
Member

Choose a reason for hiding this comment

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

single var -> multi var does not relate to this commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

really think deserves separate commit?

Copy link
Member

Choose a reason for hiding this comment

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

no, ofc, no. but do we need to have 6 line change if it may be 1-line removal (related to any style changes)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gofmt will change other line and var group will become obsolete. I don't mind to not remove braces, but imo code became more laconic

Copy link
Member

Choose a reason for hiding this comment

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

ok, up to you. just wanted to highlight that i spent some time looking at changes and see what exactly was removed and what just moved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sry bout that mate

@cthulhu-rider
node: Process API client errors according to NeoFS SDK RC-9 docs
a6bcf16 
Currently used SDK revision changed/improved docs and UX of error
handling. Storage node missed these changes on SDK upgrade, so we have
to adjust to them.

Use `errors.Is` when exact error structure is not needed. Actualize docs
of errors returned by internal client.

Fixes nspcc-dev#2465.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider
cli: Handle client's internal server error according to NeoFS SDK RC-9
5387290 
We don't have to use `errors.As` to just check error type now,
`errors.Is` is more appropriate for this.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@cthulhu-rider cthulhu-rider merged commit d67d5d3 into nspcc-dev:master Aug 8, 2023
7 of 8 checks passed
@cthulhu-rider cthulhu-rider deleted the bugfix/client-errors branch August 8, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carpawell carpawell carpawell approved these changes

@roman-khimov roman-khimov roman-khimov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cthulhu-rider @roman-khimov @carpawell