-
Notifications
You must be signed in to change notification settings - Fork 610
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
Handle (un)labeled pull request hooks #639
base: master
Are you sure you want to change the base?
Conversation
@@ -389,7 +389,8 @@ void onPullRequestHook(PullRequest pr) throws IOException { | |||
doSave = true; | |||
} else if (!trigger.isActive()) { | |||
LOGGER.log(Level.FINE, "Not processing Pull request since the build is disabled"); | |||
} else if ("edited".equals(action) || "opened".equals(action) || "reopened".equals(action) || "synchronize".equals(action)) { | |||
} else if ("edited".equals(action) || "labeled".equals(action) || "opened".equals(action) |
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.
Could you move the list of allowed labels to a static defined lists and than check if the label is inside the list?
This would improve the readability :)
Also, do we need to alter any kind of tests?
I assume you made a manual test of this? |
Although this solved the issue with removing blacklist labels, I just noticed that adding new labels to a PR after it was merged, caused it to be rebuild. see |
Thanks! |
any update on this? |
Would love to get this change in. |
d9a01ab
to
d9ab004
Compare
@Rechi do you have an update on your issue? |
Also wondering when it will be merged? |
Sorry for the extra comment, but I'm really interested in having this change merged. |
+1 to this feature as well! |
It looks good enough to me, even if there are some edge cases that might still not be covered, most of them are. |
+1 too |
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.
+1 i really need to trigger jenkins if labeled or unlabeled
@Rechi I tried to build your edits and added the plugin manually to jenkins but still changes labels couldn't be triggered by jenkins |
Hi all, is there anything I can do to help move this forward? We'd love to utilize this feature in our workflow. Thanks! |
@janinko I really would like to get your opinion on this PR My team strongly relies on label events to trigger builds. Can you consider accepting and merging the pull request as it is (with conflict fixes)? I would prefer not to fork your project in order to get this functionality working. If the pull request needs any fixes, I would be happy to take care of them. |
Even though I'm the original author of this plugin, I'm not its maintainer anymore for a long time. If there are no active maintainer I suggest to look at https://www.jenkins.io/doc/developer/plugin-governance/adopt-a-plugin/ |
@janinko I really appreciate you getting back about this! It's understandable if you don't want to maintain the plugin. @bjoernhaeuser Are you also no longer an active maintainer? |
@janinko In the off chance that this PR is acceptable, I'm going to file a duplicate PR that addresses comments and fixes the merge conflict. |
Without handling the
unlabeled
hook jenkins still things the ignore label is present (#431) and skips building the PR.This might also fix #496.