feat(actions): improve webhooks #1461
Conversation
zze0s
changed the title
|
Hey! Thanks for the PR. Good effort for someone new to Go! 👍 I see some critical issues with this current solution, mainly that it will fail if there is no body sent with the request and that will likely cause a Panic. The body it accepts and expects is hard-coded and only takes your specific usage into account. Doing this dynamically in a safe, good and user friendly way is not an easy feat either. Is your use case doing a custom webhook to an arr or some custom script? If it's the former why can't you use the built in arr handlers that has all the rejection handling already? When I re-did the external filters the intent was to later improve the actions to be similar with the status code handling, but that's still on the todo list. Could you explain what exact use case you are trying to solve with this? |
|
@MagiX13 could you respond to my previous comment? 👍 |
|
@zze0s I have used some of the time to think about it a bit more. In my specific use case, I have a custom script that handles all the details and for some of the features, a freeleech flag is required which is not submitted by the arr handlers, so I went for the webhook instead. However, I see a few ways forward
Not sure which is the way forward here, but for my local install I went for option number 3 for now. What are your views? |
|
Is this basically to be able to pass the freeleech flag to the arr actions? If that's the case then it would be easier and more appropriate to add that functionality to the arr actions, if the release push endpoint of the arrs accept it as a flag. What do you pass it as? And why not just filter for freeleech first and let autobrr handle that part and anything that gets past and sent to the arr is freeleech? |
|
Many thanks for the suggestion, @zze0s. My initial plan was to touch the code as little as possible, but I think the suggestion to just pass it along via the arrs makes a lot of sense and I have adjusted the code accordingly. I also refactored the code slightly, meaning moving the webhooks part into it's own file and added some handling based on the status codes. let me know if this is fine for you or not. Furthermore, I also added a few macros that can be exposed (e.g. the Freeleech itself was needed). In my view, there is nothing more that needs to be done and I had it running stable for close to three weeks already. Thus happy to hear your thoughts on the topic. Let me know if you prefer to have this in different PRs - or if I should just close it... |
There was a problem hiding this comment.
Many thanks for the suggestion, @zze0s. My initial plan was to touch the code as little as possible, but I think the suggestion to just pass it along via the arrs makes a lot of sense and I have adjusted the code accordingly.
I could find no reference in the radarr api docs for the freeleech tag being supported. Might be an indexerFlags and then I think we need the arr indexer to autobrr indexer mapping which still needs to be done.
I also refactored the code slightly, meaning moving the webhooks part into it's own file and added some handling based on the status codes. let me know if this is fine for you or not.
Moving to separate file is 👍. But the status code handling I still think is catering too much to your personal needs here. I can only imagine how many existing action webhooks will start to get rejected when treating HTTP 201 Created as implicit rejected.
As mentioned in the code comments the longer term plan is to improve the Actions like External Filters where you can set expected statuses and choose what to do with that.
Furthermore, I also added a few macros that can be exposed (e.g. the Freeleech itself was needed).
Nice! 👍
In my view, there is nothing more that needs to be done and I had it running stable for close to three weeks already. Thus happy to hear your thoughts on the topic. Let me know if you prefer to have this in different PRs - or if I should just close it...
Maybe add the Macros in a new PR and we make this the pr for the bigger webhooks overhaul, or revert the other stuff and keep the macros in this and we start fresh with the actions in a new pr.
| @@ -72,6 +72,7 @@ type Release struct { | |||
| PublishDate string `json:"publishDate"` | |||
| DownloadClientId int `json:"downloadClientId,omitempty"` | |||
| DownloadClient string `json:"downloadClient,omitempty"` | |||
| Freeleech bool `json:"freeleech,omitempty"` | |||
There was a problem hiding this comment.
Are you sure this is supported? Can't find anything about it in the Radarr API spec.
There was a problem hiding this comment.
Talked to bogdan in Servarr and this field is not supported, nor is indexerFlags at the moment. They are working on refactoring changes and it will likely be in radarr v6.
There was a problem hiding this comment.
Supported can mean different things here. It is supported in the sense that *arr accept the respective request, but it's not supported as the Freeleech is ignored for now. Would propose to remove it and work on a bigger webhooks overhaul instead
| } else if res.StatusCode == 201{ | ||
| return []string{"Rejected release"}, nil | ||
| } |
There was a problem hiding this comment.
I don't think we should universally treat HTTP 201 as a rejected status since we have no idea what users are doing for their existing webhooks, especially not because the action webhooks has had no concept of ok/fail except in the case it could not make the request and get a response.
The longer term plan has been to refactor the actions handling to be more like External Filters where you can set expected status etc and also add sub actions that can trigger depending on status. It's for sure a bigger task to do but would be a great improvement to users workflows.
There was a problem hiding this comment.
I'm happy to give this a go (in go), but would need some help on the typescript side of things, i.e. if the respective variables could be set in the UI and passed along to the webhook service, I can work on handling the issue.
What options would you envisage?
At the top of my head, I would expect the content and status code to be the most realistic scenarios, to distinguish between webhook outcomes.
- Accept/Reject/Error depending on regex the content matches
- Accept/Reject/Error depending on status code
- Parse the resulting content as json and see if a given key-value combination matches. Determining Accept/Reject/Error based on key-value matches, as well as rejection reason.
|
As mentioned in the comments, I'm happy to take a shot at the dynamic part, i.e. expose potential status codes or content to be returned and handling that accordingly, but have no idea on how to achieve that in typescript :) |
This PR refactors the webhook into a new file and gives the option to read from the reject, partially addressing #1002
I have also added the Freeleech* variables to the the macro to allow passing them along via webhook.
Happy to also update the documentation if you are fine with the PR
PS: First time using go, so suggestions are much appreciated :)