Fix tfs push bug #1
Conversation
The fix is ready, which basically: Handles the case when SCM configuration of a job is empty, which was the root cause of the previous random TFS push trigger failure. Add an additional case when a job should be executed – the commit is NOT null: a. runPolling() actually checks SCM changes, which in our cases are none. b. As long as a commit in linked repro triggers this push (commit is NOT null), the job should be executed. Refactors original trigger logic into function “triggerJob”, to avoid dupe code as these code will be called twice in AbstractHookEvent.java. I choose not to remove existing code in AbstractHookEvent.java, which goes through all SCMs and Repos and might still be applied to some old free style projects. Instead I add the handling of the missing cases (when SCM configuration of a job is empty) on top of it. Now if you merge any change to dotnet-linker/enable-ci, all four jobs starting with “testing” (on link below) will be triggered, and ONLY those three will be triggered. https://dotnet-vsts.westus2.cloudapp.azure.com/job/DevDiv_dotnet-linker/job/enable-ci/
Still need this iteration to filter out unwanted runs.
Clean up spaces
This reverts commit 38d1c0f.
This reverts commit c345498.
Remove unnecessary continue
| @@ -119,13 +119,13 @@ public void run() { | |||
| boolean shouldSchedule = bypassPolling; | |||
| String changesDetected = ""; | |||
| if (!bypassPolling) { | |||
| if (runPolling()) { | |||
| if (runPolling() || gitCodePushedEventArgs.commit != null) { | |||
There was a problem hiding this comment.
I would prefer...
|| StringUtils.isNotBlank(gitCodePushedEventArgs.commit)
There was a problem hiding this comment.
Good suggestion Jason, I'll fix and verify it.
Address review comments.
|
@smile21prc - I few things about process:
Thanks for this contribution! |
|
@smile21prc - when the new PR is ready in the tfs-plugin branch, I will review it and merge it when ready. |
|
@jpricketMSFT , thanks for the review and instructions about the process. I'll follow your instructions and let you know when the new PR is ready. |
Add import of StringUtils.
|
@mmitche , I've verified our jobs can constantly be triggered now by any push in enable-ci branch after addressing Jason's comment. Any additional comment before I merge into my private master branch? |
|
@smile21prc I wouldn't merge this into your private master. This should just be a PR to jenkinsci/tfs-plugin. I'll take a look at this later. |
|
Thanks @mmitche , I'll hold it till you take a look. |
|
|
||
| if (!(project instanceof AbstractProject && ((AbstractProject) project).isDisabled())) { | ||
| if (project instanceof Job) { | ||
| // TODO: Add default parameters defined in the job |
There was a problem hiding this comment.
Is it really not queueing it with parameters?
There was a problem hiding this comment.
Verified the jobs being kicked off do use customized params, so this comment isn't needed any more, removed it.
| final int quietPeriod = scmTriggerItem.getQuietPeriod(); | ||
|
|
||
| boolean triggered = false; | ||
| if (!triggered) { |
There was a problem hiding this comment.
Yep, this is the code from original plugin. I've removed this unnecessary if statement.
| final SCMTrigger scmTrigger = TeamEventsEndpoint.findTrigger(job, SCMTrigger.class); | ||
| if (scmTrigger != null && scmTrigger.isIgnorePostCommitHooks()) { | ||
| // job has explicitly opted out of hooks | ||
| triggered = false; |
There was a problem hiding this comment.
There is a lot of use of triggered in here in sort of confusing ways. I think the if (triggered) block below should be inlined into this block (invert the above if condition), returning the new gitResponse directly rather than waiting for the end of the series of if statements.
There was a problem hiding this comment.
Refined the logic, and removed "triggered" var entirely as it becomes unnecessary. Also changed to return directly instead of waiting.
| } | ||
| } | ||
| } | ||
| if (!triggered) { |
There was a problem hiding this comment.
If you inline the if (triggered) block, then this if (!triggered) becomes unnecessary since it's always false if you get here.
There was a problem hiding this comment.
Yep, removed "triggered" var entirely as it becomes unnecessary.
| final Action[] actionArray = ActionHelper.create(actions, causeAction); | ||
| scmTriggerItem.scheduleBuild2(quietPeriod, actionArray); | ||
| gitResponse = new TeamEventsEndpoint.ScheduledResponseContributor(project); | ||
| triggered = true; |
There was a problem hiding this comment.
Again you could just return gitResponse directly from this block
There was a problem hiding this comment.
Good suggestion, did that.
| triggered = true; | ||
| } | ||
| } | ||
| if (!triggered) { |
There was a problem hiding this comment.
Which would make this always false again.
There was a problem hiding this comment.
Agree, removed this unnecessary statement.
| else { | ||
| gitResponse = new TeamEventsEndpoint.PollingScheduledResponseContributor(project); | ||
| } | ||
| triggered = true; |
| @@ -119,13 +120,13 @@ public void run() { | |||
| boolean shouldSchedule = bypassPolling; | |||
| String changesDetected = ""; | |||
| if (!bypassPolling) { | |||
| if (runPolling()) { | |||
| if (runPolling() || StringUtils.isNotBlank(gitCodePushedEventArgs.commit)) { | |||
There was a problem hiding this comment.
Can you add a comment here to indicate why the gitCodePushedEventArgs is needed?
There was a problem hiding this comment.
Sure, added comments below:
// pipeline jobs might have runPolling() returned as false, while still should be scheduled.
// we should shedule them as long as they are associated with a valid commit.
There was a problem hiding this comment.
Any idea why runPolling returns false in those cases?
There was a problem hiding this comment.
(and if so, can you add it as a comment here too)
Addressed Matt's comments.
Addressed more of Matt's comments.
|
Addressed all comments, and re-created this PR as below against the branch to be merged (master in jenkinsci/tfs-plugin) |
The fix is ready, which basically:
Handles the case when SCM configuration of a job is empty, which was the
root cause of the previous random TFS push trigger failure.
Add an additional case when a job should be executed – the commit is NOT
null:
a. runPolling() actually checks SCM changes, which in our cases are
none.
b. As long as a commit in linked repro triggers this push (commit is NOT
null), the job should be executed.
Refactors original trigger logic into function “triggerJob”, to avoid
dupe code as these code will be called twice in AbstractHookEvent.java.
I removed existing code in AbstractHookEvent.java, which goes through all SCMs and Repos and might still be applied to some old free style projects. This is because its logic is dupe with the one in runPolling().
Now if you merge any change to dotnet-linker/enable-ci, all four jobs
starting with “testing” (on link below) will be triggered, and ONLY
those three will be triggered.
https://dotnet-vsts.westus2.cloudapp.azure.com/job/DevDiv_dotnet-linker/job/enable-ci/