Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Feature: Webhooks for workflow and task status #228

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Feature: Webhooks for workflow and task status #228

wants to merge 41 commits into from

Conversation

charybr
Copy link
Contributor

@charybr charybr commented May 24, 2023 •
edited
Loading

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes (Please run ./gradlew generateLock saveLock to refresh dependencies)
  • WHOSUSING.md
  • Other (please describe):

Changes in this PR

Describe the new behavior from this PR, and why it's needed
Issue #212

Webhooks for workflow and task status

Problem:
We have a requirement to publish status of workflow and task to a Webhook.

Approach:
WorkflowStatusListener and TaskStatusListener interfaces can be implemented to call webhooks when there is a change in status of workflow and task. We can add new module called 'webhook'.

This PR is originated from the changes done by @techyragu (Rahul Gupta) done on CiscoM31/conductor CiscoM31/conductor#26.

Alternatives considered

Describe alternative implementation you have considered

Raghavendra Chary B and others added 20 commits April 19, 2023 11:16
Added readme.
892d0ed
Added gradle build files.
312a25f
Added webhook module in gradle files.
2e7f722
Added Publisher related files.
d5c9cd6
Added ConfigProp interface in contribs, as it was removed from
70db9c0 
conductor-core.
Added javax.inject dependency explicitly.
9097cbf
TaskStatusListener interface is not present in conductor-core.
314b173
ExecutionDAOFacade package changed from orhestration to dal.
e5c0ab4
Added apache commons-lang3 dependency.
e8c63dc
Added http client dependency.
05dff97
Added conductor-annotations dependecy.
52c432f 
ExecutionDAO indexTask commented out.
Workflow to WorkflowModel.
indexWorkflow call commented out.
Convert WorkflowModel to Workflow.
a469d54
Merge branch 'main' of https://github.com/charybr/conductor-community …
3a7e8eb 
…into feature/webhook
Able to build after removing webhook project from dependencies.lock.
d5d0f99
Include only required modules. Jar reduced from 141M to 50M.
c643e9d
Added new config files.
a856d34
Merge branch 'main' of https://github.com/charybr/conductor-community …
4a8bdfd 
…into feature/webhook
@charybr
Uncomment lines that are required for community repo
7ba9608
TaskStatusListener implementation. For now using local libs till
bfbe6a5 
Netflix/conductor#3623 gets merged.
Task SCHEDULED status notification going out from conductor.
a6eeb4e
@charybr charybr changed the title Feature/task webhook Feature: Webhooks for workflow and task status May 24, 2023
@charybr charybr mentioned this pull request May 26, 2023
Merged
6 tasks
v1r3n
v1r3n reviewed Jun 18, 2023
View reviewed changes
webhook/src/main/java/com/netflix/conductor/contribs/publisher/TaskNotification.java Outdated

public String workflowTaskType;
private String domainGroupMoId = "";
private String accountMoId = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an application specific field. The notification should be a generic class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Viren for reviewing! Yes, both accountMoId and domainGroupMoId are custom fields. I will work on it and get back.

Copy link

@youngledo youngledo Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hello, this property configuration takes effect when the conductor server is started, but there may be multiple clients using it in practice. Can I set different URLs and endpoint according to different clients?

image

for example：
image
image

Copy link

Choose a reason for hiding this comment

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

#charybr#1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you Huang! for the comments. i will check if we can build url dynamically.

Copy link

Choose a reason for hiding this comment

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

thank you Huang! for the comments. i will check if we can build url dynamically.

Hi, @charybr ! This is my PR regarding this issue(charybr#1). Could you consider merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats great Huang! Thank you. Give me 3-4 days i will check and merge by this weekend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Huang, i have merged your PR. thank you!

Copy link

@youngledo youngledo Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, how's it going? @charybr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

review is pending

@youngledo youngledo mentioned this pull request Jul 11, 2023
Merged
6 tasks
@charybr
Copy link
Contributor Author

charybr commented Aug 24, 2023

Hi @youngledo , your changes were in branch charybr:feature/webhook. I cherry-picked your commits into branch of this PR i.e. charybr:feature/task_webhook.

So please review below commits:
20a609d
42c7ae2

@youngledo
Copy link

Hi @youngledo , your changes were in branch charybr:feature/webhook. I cherry-picked your commits into branch of this PR i.e. charybr:feature/task_webhook.

So please review below commits: 20a609d 42c7ae2

OK, I've already reviewed it.

@charybr charybr mentioned this pull request Sep 28, 2023
Closed
6 tasks
@manan164
Copy link
Contributor

manan164 commented Oct 3, 2023

Hi @charybr , Please make the below changes,

  1. Remove usage of webhook as it is conflicting with system task webhooks in workflow
  2. Make workflow publisher part of workflow-status-event-listerner module
  3. Add new module task-status-listener and move task status related changes there.

@github-actions GitHub Actions
Copy link

This PR is stale, because it has been open for 45 days with no activity. Remove the stale label or comment, or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 18, 2023
@charybr
Copy link
Contributor Author

charybr commented Nov 20, 2023

Commenting so that Stale label gets removed. Will work on the review comments in Dec.

@github-actions github-actions bot removed the Stale label Nov 21, 2023
@CollinDewey CollinDewey mentioned this pull request Apr 2, 2024
Merged
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@youngledo youngledo youngledo left review comments

@v1r3n v1r3n v1r3n left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@charybr @youngledo @manan164 @v1r3n