[JENKINS-65481] Add choosing strategy with newrev support #440
Conversation
When gerrit automatically generates a merge commit, we should be building the merge commit when the change is merged, not the patchset commit.
|
@rsandell the offending field that SpotBugs is complaining about is: /**
* Used by XStream for something.
*/
@SuppressWarnings("unused")
private final String separator = "#";Which I copied from GerritTriggerBuildChooser, not sure how to make SpotBugs happy or if this field is still needed. Any advice? Looks like it was added here: |
|
It is probably not needed in the new class since it has never been serialized before. |
| * Used by XStream for something. | ||
| */ | ||
| @SuppressWarnings("unused") | ||
| private final String separator = "#"; |
There was a problem hiding this comment.
I don't think you need this. But write some tests to verify just in case. For example a restartable jenkins rule with a config roundtrip of a freestyle projectand then check that it got loaded correctly on the next startup.
|
Thank you for the feedback. I'll see if I can figure out how to write the test you asked for. Any chance you can point me to something similar that I can derive from? I'm new to this testing framework and don't normally work on Jenkins plugins so I'm going to be digging around the codebase trying to figure that out. |
|
There are plenty of And many more in other plugins. :) |
…e for new choosing strategy. Change-Id: I0cff3c3ef604d909b3e176101789a1a8ba9746ea
| /** | ||
| * Used by XStream for something. | ||
| */ | ||
| @SuppressWarnings("unused") |
There was a problem hiding this comment.
I think removing this would break old configurations written to disk during upgrade.
There was a problem hiding this comment.
Add a test annotated @LocalData with an old configuration to verify.
| * Tests for {@link GerritTriggerBuildChooser}. | ||
| * @author Jacob Keller | ||
| * Tests for {@link GerritTriggerBuildChooserWithMergeCommitSupport}. | ||
| * @author Eric Isakson |
When gerrit automatically generates a merge commit, we should be building
the merge commit when the change is merged, not the patchset commit.
Added a new choosing strategy that honors newrev and updated README.adoc with instructions on how to use the new strategy.
I originally attempted modifying the original strategy here: https://github.com/eric-isakson/gerrit-trigger-plugin/tree/choose-newrev-for-change-merged
But that attempt caused build failures for existing jobs when an automatic merge commit is created so I decided to add a new strategy rather than trying to adapt the old one so as not to break existing behavior.
Details here: https://issues.jenkins.io/browse/JENKINS-65481