Add Slskd support: Both for indexer and download client#465
Add Slskd support: Both for indexer and download client#465T4g1 wants to merge 4 commits intoListenarrs:canaryfrom
Conversation
2daa9d8 to
7bff277
Compare
c9b2926 to
2192bd6
Compare
therobbiedavis
left a comment
There was a problem hiding this comment.
Looks great! There's some small logic issues we need to handle and I haven't done a real-world test of the implementation yet, but overall it looks good so far.
| if (transfer.Status == "completed") | ||
| { | ||
| // Download completed | ||
| await service.FinalizeDownloadAsync(download, download.DownloadPath, client, ct); |
There was a problem hiding this comment.
3/3 fire-and-forget awaits.
There was a problem hiding this comment.
@T4g1 i just noticed here that downloadpath may not actually be where slskd stores the files.. User could be using docker. We need to get the path from Slskd (MaybeOutputPath from GetItemsAsync and compare and translate it to the users remote mapping directory.
There was a problem hiding this comment.
Good point, i'll investigate that
There was a problem hiding this comment.
So, after investigation (I figured that I completely forgot to actually test that part), I added several things:
- Configuration of Download path is now correctly used (simply ignored before)
- If not present, API call to get the configured one on Slskd side (so the tooltip in the frontend stays valid eh eh)
- Added actual test for that part
- Also added a block long comment explaining how Slskd handles the file/directory thing and collisions, I might do a PR on Slskd side to update the API and simplify things in that regards if i remember that issue long enough (might be worth an issue somewhere)
Lastly, I did not use path mapping in any way here, I assumed that if Slskd is dockerized, the configured path will be a docker specific one and hence not accessible by Listenarr but in that case, user can still override it in Download Client configuration and input a correct one as visible by Listenarr. UI/documentation about that behavior might need to be updated though
There was a problem hiding this comment.
Lastly, I did not use path mapping in any way here, I assumed that if Slskd is dockerized, the configured path will be a docker specific one and hence not accessible by Listenarr but in that case, user can still override it in Download Client configuration and input a correct one as visible by Listenarr.
Yes, so that remote path mapping option for the download clients is exactly the use-case you described here. It would take the path from slskd, compare against the remote path mapping, and translate it to the destination path the the user has setup.
For example, my listenarr and qbit run both in docker with different paths. Listenarr can't access /downloads/listenarr because that doesn't exist in the listenarr docker container, only in the qbit docker container, but /server/downloads/complete/listenarr/ does exist in listenarr. Both paths map though to the same destination on the file system, it's just how they're mounted in the docker config.
There was a problem hiding this comment.
After investigation and according to the codebase, as long as QueueItem content path is filled, path mapping is already applied (I do not understand why some specific adapters are still calling that service manually, imho, Adapters should not care about this), I'll add some test to make sure it's the case though
There was a problem hiding this comment.
Test added, indeed, the QueueItem contentPath is already processed if the job contains it as sourcePath.
I removed the custom normalization from remote path mapping too to normalize normalization methods
|
@therobbiedavis I think this is ready for the second round of review 💯 |
| if (transfer.Status == "completed") | ||
| { | ||
| // Download completed | ||
| await service.FinalizeDownloadAsync(download, download.DownloadPath, client, ct); |
There was a problem hiding this comment.
@T4g1 i just noticed here that downloadpath may not actually be where slskd stores the files.. User could be using docker. We need to get the path from Slskd (MaybeOutputPath from GetItemsAsync and compare and translate it to the users remote mapping directory.
therobbiedavis
left a comment
There was a problem hiding this comment.
Found a couple more things that I have questions about
4f2c1a7 to
363d5a2
Compare
|
@therobbiedavis I believe I've processed all your comments, let me know if I missed anything |
therobbiedavis
left a comment
There was a problem hiding this comment.
I think we need some branching logic for slskd otherwise I don't think automatic downloads will work.
| { | ||
| Name = string.IsNullOrEmpty(nameFromPayload) ? (string.IsNullOrEmpty(baseUrlFromPayload) ? "Prowlarr Indexer" : baseUrlFromPayload) : nameFromPayload, | ||
| Implementation = string.IsNullOrEmpty(implementationFromPayload) ? "Custom" : implementationFromPayload, | ||
| Implementation = Implementation.Custom, // TODO: Prowlarr use Cardigann |
There was a problem hiding this comment.
I think this will break prowlarr imported indexers. Prowlarr sends over Torznab or Newznab for the implementation in the payload.
There was a problem hiding this comment.
It should be fixed but im not sure about the schema sent from Prowlarr so I based the logic on what was already there, feel free to check
Summary
Fix #183: Adds Slskd (https://github.com/slskd/slskd) support both for:
I also tried to streamline the process of managing those with the objective of making it easier to add indexers and/or download clients
This PR is way too big, sorry about that :x
Added
IDownloadClientAdapterand supported protocols to fetch list ofDownloadClientConfigurationtypes to look forChanged
Test coverage
Risks
Next steps
DownloadConfigurationClient.Typeshould probably be an enumeration or at least restricted by theDownloadClientAdapterconfiguredIDownloadOrchestratorthat could be removedIDownloadOrchestratorand other interfaces in general should not expose or expect models specifity like technical object ID but rather pass objects except for API interfaces