-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(storage): add WebDAV provider support #649
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
base: main
Are you sure you want to change the base?
Conversation
…nd-webdav Add WebDAV storage backend
Updated actions/setup-go to v5, actions/upload-artifact to v4, and softprops/action-gh-release to v2 in .github/workflows/release.yml.
@obeone , thank you very much for this! i'll review as soon as possible |
@obeone , sorry for the very late feedback. indeed i reviewed your PR immediately and from what I remember the only main concern from my side it's that I just prefer if you use a different webdav library. under x/net there's https://pkg.go.dev/golang.org/x/net/webdav I was going to tell you I could migrate to that library, if you had no willing to do the effort in your PR, please let me know :) |
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.
see also the comment in the PR conversation about the webdav library change, please
// Type returns the storage type | ||
func (s *WebDAVStorage) Type() string { return "webdav" } | ||
|
||
func (s *WebDAVStorage) fullPath(token, filename string) string { |
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.
this looks an helper that could be moved outside of the specific storage implementation, if we don't have already one somewhere.
could you please do?
func (s *WebDAVStorage) Head(_ context.Context, token, filename string) (uint64, error) { | ||
fi, err := s.client.Stat(s.fullPath(token, filename)) | ||
if err != nil { | ||
if s.logger != 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.
we don't do this kind of logging (unfortunately, I can admit) throughout all the project. if you want extra information for the user could you please wrap with en error with your specific message instead?
feel free to open a new PR reviewing the whole logging mechanism ;)
var rc io.ReadCloser | ||
var err error | ||
if rng != nil { | ||
rc, err = s.client.ReadStreamRange(p, int64(rng.Start), int64(rng.Limit)) |
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.
have you tested this on playing a video or audio media with scrobbling?
(I updated the release workflow because upload-artifact@v2 wasn't exist anymore)