-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 GetBucketWebsite <NotImplemented> errors gracefully #3620
Conversation
… that do not support websites.
Added handler for 'NoSuchWebsiteConfiguration' error for S3 endpoints…
This PR broke creation of native AWS S3 buckets. commit a900f5f resolves this. |
return err | ||
} | ||
log.Printf("[WARN] S3 bucket: %s, website configuration not supported by storage server.", d.Id()) | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning here will prevent the rest of the Read function from working (versioning, acceleration, request payer, logging, lifecycle rules, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be err := nil
instead?
Sorry, Go n00b :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line shouldn't be present at all 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surely if we remove it all together then the error object will be passed back and cause an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error are you seeing? I briefly looked at the code below this and it seemed fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I thought I was returning from the 'if' rather than the function.
I think rather than this I probably need to reset the error object to nil to prevent it being caught later on.
so rather than return nil
we should have err = nil
Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do that, but its unnecessary. The line does not need to be there at all. The function will continue just fine since line 721 already does the right thing and err
is overwritten at line 774.
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Fixes #3603