Skip to content
This repository has been archived by the owner on Aug 3, 2024. It is now read-only.

feat(triggers): readarr #174

Merged
merged 5 commits into from
Nov 4, 2022
Merged

feat(triggers): readarr #174

merged 5 commits into from
Nov 4, 2022

Conversation

voltron4lyfe
Copy link
Contributor

@voltron4lyfe voltron4lyfe commented Nov 2, 2022

Picking up adding Readarr support from @rg9400's original . (I checked with him on Discord). Adds basic Readarr support by supporting the Download event. Future PR's will be needed to add support for Delete and Rename events.

There was discussion on the original a new router architecture but my understanding of the original PR discussion was that this should move forward and we can figure out how to move this (and other *arr hooks) to the new architecture.

Closes #128, closes #143.

@m-rots m-rots requested a review from l3uddz November 4, 2022 09:55
@m-rots m-rots added triggers Specifically targets one or multiple trigger modules feature A new feature! :D labels Nov 4, 2022
@m-rots m-rots self-assigned this Nov 4, 2022
m-rots
m-rots previously approved these changes Nov 4, 2022
Copy link
Collaborator

@m-rots m-rots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! As stated in the PR description, we will just move forward with the "old trigger design" as it currently works fine anyway.

@l3uddz I performed the usual "go test ./..." and manual testing with a http client to make sure the router works correctly. I'm not sure if you're using Readarr (I doubt it) but I'd love some real-world testing. Perhaps we can launch a separate Docker tag for a couple of weeks?

Comment on lines +108 to +111
l.Info().
Str("path", scans[0].Folder).
Str("event", event.Type).
Msg("Scan moved to processor")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@l3uddz unsure if this logic is sound, but it's inline with how the Lidarr trigger approaches it.

@m-rots m-rots mentioned this pull request Nov 4, 2022
@m-rots m-rots changed the title Add Readarr trigger based on Lidarr's existing one feat(triggers): readarr Nov 4, 2022
m-rots
m-rots previously approved these changes Nov 4, 2022
@m-rots
Copy link
Collaborator

m-rots commented Nov 4, 2022

@l3uddz could you have a look at CI? 😅

@l3uddz l3uddz requested a review from m-rots November 4, 2022 21:06
@m-rots m-rots changed the base branch from master to readarr November 4, 2022 21:08
@m-rots m-rots merged commit 5337c0b into Cloudbox:readarr Nov 4, 2022
@m-rots
Copy link
Collaborator

m-rots commented Nov 4, 2022

We've decided to merge this PR into a temporary readarr branch to await some community feedback regarding any bugs which might exist. If no issues arise, then we will stabilise the Readarr trigger in #175.

m-rots added a commit that referenced this pull request Nov 24, 2022
Co-authored-by: voltron4lyfe <55123373+voltron4lyfe@users.noreply.github.com>
Co-authored-by: l3uddz <l3uddz@gmail.com>
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
feature A new feature! :D triggers Specifically targets one or multiple trigger modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

trigger: Readarr
3 participants