-
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
local adapter - respect namespace in objects full path #938
Conversation
Codecov Report
@@ Coverage Diff @@
## master #938 +/- ##
==========================================
+ Coverage 44.21% 44.37% +0.16%
==========================================
Files 143 143
Lines 11451 11483 +32
==========================================
+ Hits 5063 5096 +33
+ Misses 5769 5750 -19
- Partials 619 637 +18
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.
Great, thanks! All the code is nice, tested, makes sense, and obviously approved.
There may be backwards-compatiblity issues that we should discuss. I suggest we put it out as a release note but not more - local storage is not currently suitable for production use. Semver says it's a new minor version (because we're pre-1.0.0). @ozkatz WDYT?
return qualifiedKey, err | ||
} | ||
if qualifiedKey.StorageType != block.StorageTypeLocal { | ||
return qualifiedKey, block.ErrInvalidNamespace |
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.
How can this happen?
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.
@arielshaqed
This could happen if the repository is configured with a different namespace.
Will happen due to wrong configuration (that we currently aloud).
Or, for example, an old repository that was configured when we where using S3, and then we changed to local
Co-authored-by: arielshaqed <ariels@treeverse.io>
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.
Approved once release note is ready to go.
No description provided.