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

Add Readarr Trigger #143

Closed
wants to merge 12 commits into from
Closed

Add Readarr Trigger #143

wants to merge 12 commits into from

Conversation

rg9400
Copy link

@rg9400 rg9400 commented Dec 12, 2021

First rough attempt at adding a Readarr trigger using Lidarr's trigger as a base. Additional events for Lidarr/Readarr pending until OnDelete is merged with both of them (Rename + OnDelete both pending)

@l3uddz l3uddz requested a review from m-rots March 4, 2022 21:11
@voltron4lyfe voltron4lyfe mentioned this pull request Oct 26, 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.

Unfortunately this PR still ages from before Autoscan's router was overhauled. We would still like to see this merged though.

However, as it stands, all -arr triggers are built on top of the old trigger architecture. Unlike A-Train, which is the sole trigger that makes use of the new architecture. So the question becomes: do we keep Readarr inline with the other -arrs or do we switch it over to the new architecture, as we do not have to account for backward compatibility? @l3uddz

Verbosity string `yaml:"verbosity"`
}

// New creates an autoscan-compatible HTTP Trigger for Lidarr webhooks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whoops, still mentions Lidarr

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to have the comments fixed and then merge this.

At which point; we can refactor all the triggers to be in line with the new architecture as a separate PR?

@m-rots
Copy link
Collaborator

m-rots commented Oct 26, 2022

So I'm afraid that bridging all triggers to the new architecture is a breaking change.

A-Train accepts /a-train/{drive-id} routes. However, the -arr triggers essentially match at the root: /{name}. Funnily enough, you can thus call your Sonarr trigger radarr and everything would work just fine.

Bridging the triggers to the new architecture would force us to use /{trigger name}/{id} across the board. However, this would require users to update their -arr installs to point from /sonarr to e.g. /sonarr/sonarr.

But I'm not keen on the route having sonarr in there twice. Ideally, /sonarr would resolve to some default sonarr config option and users would write 4k as the name in their config instead of sonarr-4k, so the path would become /sonarr/4k.

We could write a migration script for this to get away with introducing a breaking change though.

@m-rots
Copy link
Collaborator

m-rots commented Nov 4, 2022

Closing in favour of #174. Thanks for the initial groundwork @rg9400 :)

@m-rots m-rots closed this Nov 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants