-
Notifications
You must be signed in to change notification settings - Fork 61
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
[JENKINS-55400] Provide an extension to notify input event #33
base: master
Are you sure you want to change the base?
Conversation
ping @jenkinsci/code-reviewers I'm looking for some reviewing. Thanks. |
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.
It's unclear what's special about input. Wouldn't think be useful for all types of pipeline steps?
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputExtension.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputExtension.java
Outdated
Show resolved
Hide resolved
|
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
Outdated
Show resolved
Hide resolved
@daniel-beck I did some new changes. Notifying all status should be more helpful for who wants to integrate this to their system. Open questions:
|
ping @dwnusbaum @daniel-beck |
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
Outdated
Show resolved
Hide resolved
Anyone could help me to review this? |
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.
Added some minor comments. It would help to see some proposed uses of the new extension point to know what it should like.
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputExtension.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputExtension.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
Outdated
Show resolved
Hide resolved
@dwnusbaum Are there anything that I need to modify? It's been a long time. |
Do I need to make any changes? |
@dwnusbaum @daniel-beck |
Any progress? |
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.
In principle sounds fine. Is there a downstream PR demonstrating this being used to implement something useful?
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputExtension.java
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/InputStepExecution.java
Outdated
Show resolved
Hide resolved
Note that you can get mostly the same effect using existing APIs such as |
I can send an email to someone if I can get the notification of input event. This is my main purpose here. |
The main thing I would like to see here before merging is some new tests, but ideally also a downstream PR of an example consumer. |
Merge with
Likely possible without any new API. If |
@jglick As you said that it's possible to do it by a bunch of code lines. But it's complicated especially for a person who still does not familiar with those API. I would suggest that giving a high-level extension point. But still, I need to thanks you for providing the tech details of it. |
ping @jenkinsci/code-reviewers |
Code reviewers will be unable to merge, so these pings don't accomplish what you want. |
delete the interface method of getName change NotifyEvent to InputEvent
Co-authored-by: Jesse Glick <[email protected]>
No description provided.