p4-plugin icon indicating copy to clipboard operation
p4-plugin copied to clipboard
verified publisher icon jenkinsci

[JENKINS-64800] Recursively check previous builds for changes during polling

Open stuartrowe opened this issue 2 years ago • 11 comments

Fix for https://issues.jenkins.io/browse/JENKINS-64800.

My team recently ran into this issue because of problems with authentication against our company's LDAP servers.

In our case a run would fail at the p4 checkout stage because of the authentication issues. Subsequent polling tasks for the job would fail to find new changes because the last run didn't have an associated TagAction.

As suggested in one of the comments on JENKINS-64800, our fix iterates through earlier builds in an attempt to find a baseline for polling.

Testing done

There aren't currently any unit tests related to polling. Instead my team created a manual test with the following steps:

  1. Temporarily swap a valid credential with an invalid one
  2. Run a job using that credential and note that it fails during checkout
  3. Restore the valid credential
  4. Submit a change to trigger polling and confirm that polling is successful

We confirmed that this test failed with p4-plugin 1.14.1 and succeeded with the fix.

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

stuartrowe avatar Jun 21 '23 20:06 stuartrowe

@p4paul could you please review this when you have a chance?

stuartrowe avatar Jun 25 '23 20:06 stuartrowe

Close + Reopen to re-trigger CI.

stuartrowe avatar Sep 15 '23 21:09 stuartrowe

@p4paul @skumar7322 are either of you available to review this?

stuartrowe avatar Sep 15 '23 21:09 stuartrowe

Thanks, @stuartrowe for initiating this pull request. This change will throw the NPE when lastRun is null. Because of this, there are test failures in PollingTest, PerforceSCMSourceTest suit. Please execute these test suites for your proposed changes. Also given our planned workload, we intend to address JENKINS-64800 in our upcoming releases.

skumar7322 avatar Sep 20 '23 06:09 skumar7322

@skumar7322 thanks for pointing that out, I had overlooked it somehow. Should be addressed now 🤞.

stuartrowe avatar Sep 21 '23 17:09 stuartrowe

@skumar7322 there seems to be an issue with org.jenkinsci.plugins.p4.SimpleTestServer that is affecting other PRs too.

stuartrowe avatar Sep 22 '23 17:09 stuartrowe

@skumar7322 is there any update on the next planned release and a fix for JENKINS-64800? If not, can this PR please be revisited?

stuartrowe avatar Jan 10 '24 20:01 stuartrowe

Hi @stuartrowe this change looks good to me. I will run the tests on this and merge it once all is good.

skumar7322 avatar Feb 08 '24 11:02 skumar7322