chore: make adjustments to enable cd - #478
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new test synchronization/teardown code has a couple of correctness and robustness issues (mismatched CloudStatistics polling key and swallowed thread interrupts) that should be fixed to avoid continued flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the plugin’s build/test setup to support continuous delivery and to stabilize tests under newer Java/Jenkins baselines by pinning tool versions and reducing teardown/provisioning races.
Changes:
- Bumped Jenkins baseline to 2.541 and pinned several Maven/tooling versions (license, SpotBugs, Palantir format) to avoid JDK-related crashes.
- Added Spotless configuration to force a specific Palantir Java Format version.
- Adjusted test harness and tests to reduce CloudStatistics/agent teardown races and provisioning flakiness; updated CD workflow trigger configuration.
File summaries
| File | Description |
|---|---|
| pom.xml | Updates Jenkins baseline and pins formatting/static-analysis related versions; configures Spotless Palantir formatter version. |
| plugin/src/test/java/jenkins/plugins/openstack/PluginTestRule.java | Adds CloudStatistics synchronization and more robust teardown/cleanup logic for agents and stats persistence. |
| plugin/src/test/java/jenkins/plugins/openstack/compute/ProvisioningTest.java | Adds polling to avoid racy assertions around CloudStatistics activity naming. |
| plugin/src/test/java/jenkins/plugins/openstack/compute/JCloudsCloudTest.java | Adds waits for node provisioning and forces agent termination to avoid teardown races. |
| .github/workflows/cd.yaml | Adjusts workflow triggers (removes check_run) in support of CD workflow usage. |
Review details
Suppressed comments (1)
plugin/src/test/java/jenkins/plugins/openstack/PluginTestRule.java:757
- flushCloudStatistics catches Exception and ignores InterruptedException, which clears the interrupt flag and can make shutdown/timeout behavior less predictable. Catch InterruptedException separately and restore the interrupt status.
} catch (Exception e) {
// Jenkins may already be shutting down
}
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The updated test-rule teardown can mask original test failures by throwing from finally, and one CloudStatistics assertion remains potentially racy due to inconsistent activity lookup.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
plugin/src/test/java/jenkins/plugins/openstack/PluginTestRule.java:649
- The teardown work is now executed in a
finallyblock, but any exception thrown while resettingProcessTree.vetoersExistor killing JNLP agent processes can mask the original test failure (or fail an otherwise-successful test). Since this is best-effort cleanup, it should not override the primary test result; handle cleanup exceptions locally and preserve interrupt status.
plugin/src/test/java/jenkins/plugins/openstack/compute/ProvisioningTest.java:523 waitForActivityNamepollsCloudStatistics.get().getActivityFor(slave), but the following assertion reads the activity name viagetActivityFor(computer). If those two lookups are not equivalent, the wait does not actually guard the assertion and the test can remain racy. Use a consistent lookup (theJCloudsSlave), and assert the activity exists before dereferencing.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Tasks completed
license-maven-pluginversion to fix Java 25 license-plugin crash.spotbugs-maven-pluginversion.Testing done
Ran
mvn clean verifylocally successfully.Submitter checklist