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

report better s3Errors #1039

Merged
merged 2 commits into from
Jan 23, 2024
Merged

report better s3Errors #1039

merged 2 commits into from
Jan 23, 2024

Conversation

rakeshgm
Copy link
Member

use aws/awserr package to report
better errors with correct format

issue: #749
Bugzilla 2069759

BenamarMk
BenamarMk reviewed Aug 25, 2023
View reviewed changes
controllers/s3utils.go Outdated
return fmt.Errorf("%s: code: %s, message: %s", errMsgPrefix, awsErr.Code(), awsErr.Message())
}

return fmt.Errorf("%s: not an aws error", errMsgPrefix)
Copy link
Member

Choose a reason for hiding this comment

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

Just return errMsgPrefix

Copy link
Member Author

Choose a reason for hiding this comment

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

done

ShyamsundarR
ShyamsundarR reviewed Aug 25, 2023
View reviewed changes
Copy link
Member

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

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

LGTM!

Question: Why not handle this in other calls as well like DeleteBucket, CreateBucket, ...? They could also return malformed errors that would end up in the resource status.

@rakeshgm rakeshgm marked this pull request as draft August 31, 2023 10:21
@rakeshgm rakeshgm force-pushed the s3bz2069759 branch 2 times, most recently from 97e626a to 1905cce Compare November 16, 2023 06:39
@rakeshgm rakeshgm marked this pull request as ready for review November 20, 2023 10:19
ShyamsundarR
ShyamsundarR reviewed Dec 11, 2023
View reviewed changes
Copy link
Member

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

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

@rakeshgm Please handle these instances as well:

  • ramen/controllers/s3utils.go

    Line 579 in fe846a6

    _, err := s.client.DeleteObject(&s3.DeleteObjectInput{
    • We return the err back to higher layers, and should process it like in the other places

  • ramen/controllers/s3utils.go

    Line 627 in fe846a6

    return s.batchDeleter.Delete(ctx, &s3manager.DeleteObjectsIterator{
    • Here error is returned back to caller, would be good to process it right here and return instead

@rakeshgm
report better s3Errors
1e3ed20 
use aws/awserr package to report
better errors with correct format

Signed-off-by: rakeshgm <[email protected]>
@rakeshgm
addressing comments
217820c 
Signed-off-by: rakeshgm <[email protected]>
@BenamarMk BenamarMk merged commit 040137e into RamenDR:main Jan 23, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShyamsundarR ShyamsundarR ShyamsundarR left review comments

@BenamarMk BenamarMk BenamarMk 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
@rakeshgm @ShyamsundarR @BenamarMk