Skip to content

[JENKINS-65481] Add choosing strategy with newrev support - #440

Open
eric-isakson wants to merge 2 commits into
jenkinsci:masterfrom
eric-isakson:build-chooser-with-merge-commit-support
Open

[JENKINS-65481] Add choosing strategy with newrev support#440
eric-isakson wants to merge 2 commits into
jenkinsci:masterfrom
eric-isakson:build-chooser-with-merge-commit-support

Conversation

@eric-isakson

@eric-isakson eric-isakson commented May 13, 2021

Copy link
Copy Markdown

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

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master 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

When gerrit automatically generates a merge commit, we should be building
the merge commit when the change is merged, not the patchset commit.
@eric-isakson

Copy link
Copy Markdown
Author

@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:

1f8d0df

@rsandell

Copy link
Copy Markdown
Member

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 = "#";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@rsandell rsandell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Besides the check for the unused field it looks good.

@eric-isakson

Copy link
Copy Markdown
Author

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.

@rsandell

Copy link
Copy Markdown
Member

There are plenty of configRoundTrip(p) tests in this plugin you can look at. But we haven't used RestartableJenkinsRule yet.
But there is an example one here https://github.com/jenkinsci/cloudevents-plugin/blob/0bb2b3079848c07e2158d8cc472d6d42963a545a/src/test/java/io/jenkins/plugins/sample/SampleConfigurationTest.java

And many more in other plugins. :)

…e for new choosing strategy.

Change-Id: I0cff3c3ef604d909b3e176101789a1a8ba9746ea
/**
* Used by XStream for something.
*/
@SuppressWarnings("unused")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think removing this would break old configurations written to disk during upgrade.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants