Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Trigger workflow on database event #6480
Trigger workflow on database event #6480
Changes from all commits
47db158
a850682
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider handling cases where
payload
might be undefined or null more explicitly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Ensure WorkflowModule is correctly configured to avoid circular dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Verify that adding WorkflowModule does not introduce performance bottlenecks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Removing the MANUAL trigger type might limit future flexibility. Consider if this could be needed again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Ensure all references to WorkflowManualTrigger are removed or updated across the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Reordering imports to maintain logical grouping. Ensure this does not affect dependency initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be consistent between those
handle
function parameters and therunWorkflow
parameters. Lets use the workflowId inrunWorkflow
then. Also, we should update theworkflowCommonService.getWorkflowVersion
so that we don't needtwentyORMGlobalManager
service hereThen the
runWorkflow
method can also use the updated version ofworkflowCommonService.getWorkflowVersion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martmull agreed with the input update. But thinking again about the endpoint, I think this is the version we want to run. This endpoint will be used to test versions. I rename it to be consistent. Happy to discuss it if you disagree!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Fetching the repository for each job execution might introduce performance overhead. Consider caching the repository if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding more specific error handling or logging to provide better insights into why a workflow might not have a published version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check: Ensure that the payload structure is validated before passing it to the workflow runner service to prevent runtime errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding error handling to log or manage failures when adding a job to the message queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check: Ensure that the payload data structure is validated before adding it to the message queue to prevent potential runtime errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Ensure
WorkflowRunnerModule
can handle the additional load from new event triggers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Verify
WorkflowEventTriggerJob
implementation covers all edge cases and error handling.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Ensure proper error handling and logging for missing workspaceId or eventName to avoid silent failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Verify that the feature flag check is performant and does not introduce latency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Consider caching the repository instance to avoid repeated database lookups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think you can move that in
workflow-common.service.ts
in agetEventListeners
methodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only place where we fetch eventListeners and not sure we will have others. I prefer put in common functions that will be used in several places, across sub-modules. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Ensure that the message queue service handles retries and failures gracefully to avoid job loss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Ensure the
FeatureFlagModule
is correctly configured and handles all relevant feature flags.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Verify that
DatabaseEventTriggerListener
handles all edge cases and potential errors gracefully.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Ensure
WorkflowTriggerJobModule
does not introduce circular dependencies.