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 Event Rules to process and send data to Scripts using Event Data #15063

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

renatoalmeidaoliveira
Copy link
Contributor

@renatoalmeidaoliveira renatoalmeidaoliveira commented Feb 6, 2024

Fixes: #14884

Passes Event Rule a jinja2 processed action_data to script execution as its input.

@jeremystretch
Copy link
Member

Fixes: #14884
Fixes: #14896

Neither of these FRs have been approved before submitting this PR, and even if they had been, they would each need a separate PR. I understand that it's tempting to save time by combining work, but that also makes effective review and documentation very difficult.

I've just marked #14884 as approved and assigned it to you, but please revise the PR to remove the introduction of additional context so that we can focus on its specific goal.

@renatoalmeidaoliveira
Copy link
Contributor Author

@jeremystretch, updated the PR, and the changes in the code reflects only the task of passing action_data to the script just like the other methods, i.e the script develop may access the input the same way he access the forms.

@jeremystretch
Copy link
Member

@renatoalmeidaoliveira did you push your changes? I don't see any updates.

@renatoalmeidaoliveira
Copy link
Contributor Author

renatoalmeidaoliveira commented Feb 14, 2024

@jeremystretch I may have missundertud what you asked. The changes I've made was:
1 create a method to Jinja2 process the action_data with the same context passed to the webhocks
2 passes this data to the script form to process it
3 passes the validated form to the script

You mean to just pass action_data as created by the user to the Script as a String without any processing? I think that might not be very usefull for the user because of the lack of context of the modified object

@renatoalmeidaoliveira
Copy link
Contributor Author

I can put all code inside process_event_rules too,
Or keep process_action_data method in EventRules model but process the Script form inside it and remove the changes in the run_script method

@renatoalmeidaoliveira
Copy link
Contributor Author

@jeremystretch updated the code to restrict the changes only to process_event_rule inside the Script context.

@aharrisson
Copy link
Contributor

Is there a reason why this implementation only affect EventRuleActionChoices.SCRIPT? My use-case is to be able to use more data in a webhook payload than what I get from the default event rule "data" dict.

@renatoalmeidaoliveira
Copy link
Contributor Author

@aharrisson imo the implementation should be the same to both Webhoocks and Scripts, giving the user more flexibility to pass data to his actions.
But anyways the Scripts it requires a different implementation to allow the user to use the data variable along with the input variable forms.
I can make further changes in that PR but I'm waiting some guidance from the maintainers.

@aharrisson
Copy link
Contributor

@renatoalmeidaoliveira , Thank you for the quick reply.
Indeed keeping the capabilities consistent (as far as possible) across both action choices would make sense. I hope the maintainers can extend the boundaries of this PR to also include webhooks, otherwise the action_data form field would be confusing for event rules triggering webhooks.

@llamafilm
Copy link
Contributor

Currently, if I have an event rule that triggers when an IP address is created, modified, or deleted — it's impossible for the script to know which action triggered it. Will this PR address that?

@renatoalmeidaoliveira
Copy link
Contributor Author

@llamafilm short answer, in the implementation of this PR yes.
My idea with this PR is to enable the user to build the input passed to the Script Input vars, in a similar way the webhocks body template does

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label May 21, 2024
@netbox-community netbox-community deleted a comment from llamafilm May 22, 2024
@arthanson
Copy link
Collaborator

@renatoalmeidaoliveira after reviewing with Jeremy, the jinja2 processing shouldn't be here, the data should just be passed as a json blob without going through jinja2. Can you please update the PR for this?

@arthanson
Copy link
Collaborator

Discussed with @renatoalmeidaoliveira - I need to review the points he brought up, will do later this week.

@arthanson arthanson self-assigned this May 28, 2024
'request_id': request_id,
'model': data,
}
rendered_data = render_jinja2(json.dumps(event_rule.action_data), context)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I don't see why the render_jinja2 is needed here - event_rule.action_data should just be the json (dictionary) which should be able to get directly passed to the script. I'm thinking you could just change the Job.enqueue to pass event_rule.action_data as the data param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arthanson If we don't render the jinja2 how the user gonna pass variables to the script, for example:
1 - For example the Script form expects an Interface ID, or some model attribute.
If It didn't run the jinja2 how the user gonna be able to pass models variables to be processed by the script form?

Copy link
Contributor Author

@renatoalmeidaoliveira renatoalmeidaoliveira Jun 11, 2024

Choose a reason for hiding this comment

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

The user doens't have the model params when he defines the event rule, so I think there must be some way for him to reference the models and the changelogs, to build the data that gonna be passed to the Script form

Copy link
Member

Choose a reason for hiding this comment

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

The action_data field on EventRule holds a dictionary of arbitrary additional data. This doesn't require any mutation; it just needs to be passed to the script, and the script needs to accommodate whatever data is being passed as variables.

I think you're trying to accomplish something different than what I agreed to here. There's no reason to manipulate the data before sending it to the script; the script just needs to be written to accept the data as passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeremystretch even if I pass the event_data as is to the script how the Script developer gonna be able to reference the event object like the changed object, since it doesn't have that data inside the Script? another thing is that way breaks the common script logic where the developer expects the data dict to return a parsed form

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going further if the action_data is a static value what's the meaning of having it at all, the Script dev could just create the script with that data hard coded

@arthanson arthanson removed their assignment Jun 11, 2024
@jeremystretch jeremystretch removed the pending closure Requires immediate attention to avoid being closed for inactivity label Jun 20, 2024
Copy link
Contributor

github-actions bot commented Jul 6, 2024

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending closure Requires immediate attention to avoid being closed for inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event Rule Action Data passed to Script
5 participants