-
Notifications
You must be signed in to change notification settings - Fork 395
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-30626] Handle pings from organization webhooks #89
[JENKINS-30626] Handle pings from organization webhooks #89
Conversation
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
Of course! 🐝 and 👍 |
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
🐝 , but seems to be significant enough to have a JIRA issue. Does the original code fail with NPE? |
🐝 |
Please add a test with such payload same as for the ping. |
@@ -10,7 +10,7 @@ | |||
import java.util.Set; | |||
|
|||
import static com.google.common.collect.Sets.immutableEnumSet; | |||
import static net.sf.json.JSONObject.fromObject; | |||
import net.sf.json.JSONObject; |
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.
then it should be not in static block of imports
👍 thanks. I'll going to squash this and merge manually |
Please read what GH wrote: |
@KostyaSha Thanks. I have to learn why this way is better (than traditional merge). |
It not better, GH closes relevant PR when changes committed in target branch directly. Faster fix ourself instead of commenting and waiting for changes from PR author. |
@KostyaSha ah ok. Thanks for your time. |
JSONObject parsedPayload = fromObject(payload); | ||
JSONObject repository = parsedPayload.optJSONObject("repository"); | ||
if (repository != null) { | ||
// something like <https://github.com/bar/foo> |
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.
Not sure it matters, but not that this is not actually the form of the URL; it is something involving api.github.com
.
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.
@jglick For that, there are GHEventsSubscriber
validating the URL.
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.
My point was just that the comment is wrong.
Well, when the special |
Special keyword is not required. When diff has nothing GH closes it. |
JENKINS-30626
Reference
@reviewbybees esp. @recena who discovered this bug.