Skip to content
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

#1427: Prevent job triggering for non-matching webhooks #1720

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

mc1arke
Copy link
Member

@mc1arke mc1arke commented Nov 2, 2024

8ec69c7 introduced a change that does not handle jobs created using Jenkins DSL properly, as DSL jobs that do not specify triggerOpenMergeRequestOnPush have the triggerOpenMergeRequest value passed into
MergeRequestHookTriggerHandlerFactory as null, with the above change causing the value not to be handled as never where the plugin previously treated null as never. This results in webhooks triggering builds for DSL-created jobs even where the job is doing actions that aren't valid for a Merge Request in that state (e.g. triggering release builds even though the Merge Request hasn't been merged). This change returns to handling an unspecified triggerOpenMergeRequest value as being equivalent to specifying it as never.

Testing done

Automated test to cover failing scenario.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@mc1arke mc1arke requested a review from a team as a code owner November 2, 2024 13:09
@github-actions github-actions bot added the tests This PR adds/removes/updates test cases label Nov 2, 2024
@krisstern
Copy link
Member

Thanks @mc1arke for the PR! I will review it over the next few days

@krisstern
Copy link
Member

Looks like you may want to run mvn spotless:apply to fix the remaining failed checks

8ec69c7 introduced a change that does
not handle jobs created using Jenkins DSL properly, as DSL jobs that do
not specify `triggerOpenMergeRequestOnPush` have the
`triggerOpenMergeRequest` value passed into
`MergeRequestHookTriggerHandlerFactory` as `null`, with the above
change causing the value not to be handled as `never` where the plugin
previously treated `null` as `never`. This results in webhooks
triggering builds for DSL-created jobs even where the job is doing
actions that aren't valid for a Merge Request in that state (e.g.
triggering release builds even though the Merge Request hasn't been
merged). This change returns to handling an unspecified
`triggerOpenMergeRequest` value as being equivalent to specifying it as
`never`.
Copy link
Member

@krisstern krisstern left a comment

Choose a reason for hiding this comment

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

It makes sense to me @mc1arke! Thanks for the PR

@krisstern krisstern merged commit 5371e88 into jenkinsci:master Nov 5, 2024
18 checks passed
@krisstern
Copy link
Member

I will cut a release with this code change within the next 24 hours.

@krisstern
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests This PR adds/removes/updates test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants