Skip to content
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

Enable webhooks for JobResults #8958

Closed
kkthxbye-code opened this issue Mar 24, 2022 · 10 comments
Closed

Enable webhooks for JobResults #8958

kkthxbye-code opened this issue Mar 24, 2022 · 10 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Milestone

Comments

@kkthxbye-code
Copy link
Contributor

NetBox version

v3.1.9

Feature type

Change to existing functionality

Proposed functionality

Add the @extra_features('webhooks') to the JobResult model.

Use case

When triggering Netbox scripts via. the API it would be useful to be able to send a webhook when the status changes. This would prevent the need for polling the API for scripts with longer runtime.

Database changes

None

External dependencies

No response

@kkthxbye-code kkthxbye-code added the type: feature Introduction of new functionality to the application label Mar 24, 2022
@kkthxbye-code
Copy link
Contributor Author

I tried just adding @extras_features('webhooks') to the model, but it doesn't trigger webhooks. I'm sure there are some pre-requisites for using webhooks I'm not aware of. I wouldn't mind implementing if accepted and with a small pointer as to what is needed to make it work.

@kkthxbye-code
Copy link
Contributor Author

kkthxbye-code commented Apr 1, 2022

Looking into this more, I guess JobResults would be to be changed to inherit from ChangeLoggedModel. Not sure how feasible that is or if it just works out of the box. The migration would make it a significant change I guess.

@jeremystretch
Copy link
Member

I guess JobResults would be to be changed to inherit from ChangeLoggedModel

We can enable webhooks for the model separately, without having to inherit the other stuff ChangeLoggedModel provides.

@jeremystretch jeremystretch added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Apr 7, 2022
@kkthxbye-code kkthxbye-code changed the title Enabled webhooks for JobResults Enable webhooks for JobResults Apr 8, 2022
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Apr 8, 2022
@jeremystretch jeremystretch added status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation and removed status: accepted This issue has been accepted for implementation labels Apr 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Do not attempt to circumvent this process by "bumping" the issue; doing so will result in its immediate closure and you may be barred from participating in any future discussions. Please see our contributing guide.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Jun 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2022

This issue has been automatically closed due to lack of activity. In an effort to reduce noise, please do not comment any further. Note that the core maintainers may elect to reopen this issue at a later date if deemed necessary.

@github-actions github-actions bot closed this as completed Jul 8, 2022
@jeremystretch jeremystretch added needs milestone Awaiting prioritization for inclusion with a future NetBox release and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation pending closure Requires immediate attention to avoid being closed for inactivity labels Jul 8, 2022
@jeremystretch jeremystretch reopened this Oct 5, 2022
@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed needs milestone Awaiting prioritization for inclusion with a future NetBox release labels Jan 5, 2023
@jeremystretch jeremystretch added this to the v3.5 milestone Jan 5, 2023
@jeremystretch
Copy link
Member

@kkthxbye-code would we want to evaluate webhook on every call to save(), or only for jobs with a terminal status (e.g. completed or errored)? The former would provide the most flexibility, though I worry about people creating non-conditional webhooks that fire each time a job is created, executed, and completed.

@kkthxbye-code
Copy link
Contributor Author

@jeremystretch - The original issue we were having, was that we had to poll to check for script completion when triggering scripts from external systems. So only triggering for terminal statuses would probably be fine for that use-case. However I don't think it should be an issue doing it on all saves, unless JobResults are saved multiple times during execution? A normal script run would be Pending -> Running -> Completed right?

@jeremystretch
Copy link
Member

Right, what I mean is that each change to a job's status has to be written to the database (although it might not actually call save()), which could in theory trigger a webhook each time.

@jeremystretch
Copy link
Member

jeremystretch commented Feb 28, 2023

I suppose for the initial implementation at least we can enable webhooks for all statuses, and then constrain to only terminal statuses if needed.

@jeremystretch jeremystretch self-assigned this Feb 28, 2023
@jeremystretch
Copy link
Member

I ended up introducing two new event types, job_start and job_end, which trigger at the beginning and end of a job's execution respectively. These map to the start() and (new) terminate() methods on the JobResult class, which handle triggering webhooks in addition to updating the job's status.

jeremystretch added a commit that referenced this issue Mar 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Projects
None yet
Development

No branches or pull requests

2 participants