-
Notifications
You must be signed in to change notification settings - Fork 360
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
use gaul/s3proxy #1113
use gaul/s3proxy #1113
Conversation
gateway/handler.go
Outdated
@@ -269,7 +275,16 @@ func RepoOperationHandler(sc *ServerContext, repoID string, handler operations.R | |||
repo, err := authOp.Cataloger.GetRepository(sc.ctx, repoID) | |||
if errors.Is(err, db.ErrNotFound) { | |||
authOp.Log().WithField("repository", repoID).Warn("the specified repo does not exist") |
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.
warning in both cases will be probably bad on the performance
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.
I don't think we should warn on invalid requests or missing resources
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.
Looks great!! elegant and simple solution.
I would strongly suggest changing the configuration setting name
gateway/handler.go
Outdated
@@ -269,7 +275,16 @@ func RepoOperationHandler(sc *ServerContext, repoID string, handler operations.R | |||
repo, err := authOp.Cataloger.GetRepository(sc.ctx, repoID) | |||
if errors.Is(err, db.ErrNotFound) { | |||
authOp.Log().WithField("repository", repoID).Warn("the specified repo does not exist") |
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.
I don't think we should warn on invalid requests or missing resources
Codecov Report
@@ Coverage Diff @@
## master #1113 +/- ##
==========================================
- Coverage 46.98% 46.98% -0.01%
==========================================
Files 174 174
Lines 14016 14028 +12
==========================================
+ Hits 6586 6591 +5
- Misses 6576 6583 +7
Partials 854 854
Continue to review full report at Codecov.
|
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.
Nice work! I'm fine with merging this, only a couple of small comments
authOp.Log().WithField("repository", repoID).Warn("the specified repo does not exist") | ||
authOp.EncodeError(gatewayerrors.ErrNoSuchBucket.ToAPIErr()) | ||
if sc.fallbackProxy == nil { | ||
authOp.Log().WithField("repository", repoID).Warn("the specified repo does not exist") |
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.
For anything that is the user's fault I recommend using Debug
@@ -74,6 +74,7 @@ This reference uses `.` to denote the nesting of values. | |||
(`*.s3.local.lakefs.io` always resolves to 127.0.0.1, useful for | |||
local development | |||
* `gateways.s3.region` `(string : "us-east-1")` - AWS region we're pretending to be. Should match the region configuration used in AWS SDK clients | |||
* `gateways.s3.fallback_url` `(string)` - If specified, requests with a non-existing repository will be forwarded to this url. |
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.
Maybe add a note about s3 proxy and the use case?
No description provided.