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.

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

Handle "Not implemented" error codes in functional test #838

Merged
merged 1 commit into from Oct 7, 2017

Conversation

kannappanr
Copy link
Collaborator

Check for the error message "Not Implemented" in ComposeObject
API call. If it is seen, log NA for the test and log the error, but continue
with the test.

Fixes #834

if strings.Contains(err.Error(), NotImplemented) {
ignoredLog(function, args, startTime, err.Error()).Info()
return
} else {

Choose a reason for hiding this comment

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

if block ends with a return statement, so drop this else and outdent its block

if strings.Contains(err.Error(), NotImplemented) {
ignoredLog(function, args, startTime, err.Error()).Info()
return
} else {

Choose a reason for hiding this comment

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

if block ends with a return statement, so drop this else and outdent its block

if strings.Contains(err.Error(), NotImplemented) {
ignoredLog(function, args, startTime, err.Error()).Info()
return
} else {

Choose a reason for hiding this comment

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

if block ends with a return statement, so drop this else and outdent its block

@@ -62,6 +62,7 @@ const (
secretKey = "SECRET_KEY"
enableHTTPS = "ENABLE_HTTPS"
)
const NotImplemented = "A header you provided implies functionality that is not implemented"

Choose a reason for hiding this comment

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

exported const NotImplemented should have comment or be unexported

@kannappanr kannappanr force-pushed the handle_not_implemented branch 2 times, most recently from b07b9e7 to d322dca Compare October 3, 2017 17:24
@ebozduman
Copy link
Contributor

LGTM

@@ -62,6 +62,7 @@ const (
secretKey = "SECRET_KEY"
enableHTTPS = "ENABLE_HTTPS"
)
const notImplemented = "A header you provided implies functionality that is not implemented"
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to look the string we can simply look for the error code.

@@ -4478,6 +4479,10 @@ func testComposeMultipleSources(c *minio.Client) {
}
err = c.ComposeObject(dst, srcs)
if err != nil {
if strings.Contains(err.Error(), notImplemented) {
Copy link
Member

Choose a reason for hiding this comment

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

Convert this into a function lets like

isErrNotImplemented()
func isErrNotImplemented(err error)  bool {
   errResp := ToErrorResponse(err)
   return errResp.Code != "NotImplemented"
}

@kannappanr kannappanr force-pushed the handle_not_implemented branch 3 times, most recently from 9fd2d9d to 5a8146b Compare October 6, 2017 23:25
@@ -158,6 +158,11 @@ func cleanupBucket(bucketName string, c *minio.Client) error {
return err
}

func isErrNotImplemented(err error) bool {
errResp := minio.ToErrorResponse(err)
Copy link
Member

Choose a reason for hiding this comment

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

This can be written in even one liner..

func isErrNotImplemented(err error) bool {
         return minio.ToErrorResponse(err).Code != "NotImplemented"
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

Check for the error message "Not Implemented" in ComposeObject
API call. If it is seen, log NA for the test and log the error, but continue
with the test.

Fixes minio#834
@deekoder deekoder merged commit 3bb02b8 into minio:master Oct 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants