Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-48924, JENKINS-48995] - Fix plugin compatibility with Jenkins 2.102+ #29

Merged
merged 7 commits into from
Jan 25, 2018

Conversation

oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented Jan 16, 2018

  • TestLinkBuildAction and TestLinkResult do not longer serialize build instance
  • Whitelist TestLink API model classes, which are being persisted in reports
  • Fix the Builder in ResultTestCaseSeeker class

https://issues.jenkins-ci.org/browse/JENKINS-48924
https://issues.jenkins-ci.org/browse/JENKINS-48995

@reviewbybees @jglick

import org.kohsuke.stapler.StaplerProxy;

/**
* @author Bruno P. Kinoshita - http://www.kinoshita.eti.br
* @since 1.0
*/
public class TestLinkBuildAction implements Action, Serializable, StaplerProxy {
public class TestLinkBuildAction implements RunAction2, Serializable, StaplerProxy {
Copy link
Member Author

Choose a reason for hiding this comment

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

Surprisingly, #25 does not do that

@ghost
Copy link

ghost commented Jan 16, 2018

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev
Copy link
Member Author

I currently cannot say whether it work on agents actually, the autotests do not seem to cover that

@oleg-nenashev
Copy link
Member Author

PCT passes with this fix BTW


private static final long serialVersionUID = -914904584770393909L;

public static final String DISPLAY_NAME = "TestLink";
public static final String ICON_FILE_NAME = "/plugin/testlink/icons/testlink-24.png";
public static final String URL_NAME = "testLinkResult";

private AbstractBuild<?, ?> build;
private transient Run<?, ?> build;
Copy link
Member

Choose a reason for hiding this comment

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

Good, making this transient should solve a host of issues.

@oleg-nenashev oleg-nenashev changed the title [JENKINS-48924] - Fix plugin compatibility with Jenkins 2.102+ [JENKINS-48924, JENKINS-48995] - Fix plugin compatibility with Jenkins 2.102+ Jan 17, 2018
@oleg-nenashev
Copy link
Member Author

Also fixed TestNG logic, which serialization has not been covered by tests
@jglick @kinow PTAL

@@ -58,7 +57,7 @@

public static final String TEXT_XML_CONTENT_TYPE = "text/xml";

protected final TestNGParser parser = new TestNGParser();
protected transient TestNGParser _parser = null;
Copy link
Member Author

Choose a reason for hiding this comment

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

The class could be actually made static, but it needs update of the bundled lib

Copy link
Member

Choose a reason for hiding this comment

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

Maybe should be volatile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does not matter much. In the worst case there will be two instances of a same class which actually has no dynamic context at all. The class methods should be reworked to static, I just decided not to do breaking changes in this PR

import javax.management.Descriptor;

@For(ResultSeeker.class)
@Ignore
Copy link
Member

Choose a reason for hiding this comment

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

specify a reason in the annotation value

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I pushed an unfinished test.
It does not hurt in the current state, but I will remove the file

@@ -58,7 +57,7 @@

public static final String TEXT_XML_CONTENT_TYPE = "text/xml";

protected final TestNGParser parser = new TestNGParser();
protected transient TestNGParser _parser = null;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe should be volatile.

@oleg-nenashev
Copy link
Member Author

@kinow I noticed the plugin is up for adoption. Do you have time to review/release it or should we proceed on our own?

@kinow
Copy link
Member

kinow commented Jan 24, 2018

@oleg-nenashev either way works for me. I stopped keeping up to date with Jenkins plugin/core API when the workflow-plugin was still in the works, and haven't had time to catch up with the changes.

Looks to me like most of the changes are for the JEP and Jenkins/plug-in API. No changes in the way the plug-in interacts with TestLink or test reports. So everything should work the same for users (just much better, and in the latest versions of Jenkins :-) ).

So +1, LGTM, and thanks a lot!

Happy to merge it, or feel free to merge it. Do we need #25 merged as well?

If you'd like to take over the plug-in, I'd be OK with that too :) tho I suspect you are probably busy with other things and just needs to fix this issue and have a new release out?

Ta
B

@ghost
Copy link

ghost commented Jan 24, 2018 via email

@oleg-nenashev
Copy link
Member Author

oleg-nenashev commented Jan 24, 2018 via email

@kinow
Copy link
Member

kinow commented Jan 25, 2018

Sure, works for me @oleg-nenashev.

What about the #25? Is that required as well for a release?

@kinow kinow merged commit 9d4b1cc into jenkinsci:master Jan 25, 2018
@kinow
Copy link
Member

kinow commented Jan 25, 2018

Will cut a release as is (and plus #25 if @oleg-nenashev says it needs to be included for this PR) this weekend (this is an extended weekend, with Monday being Auckland anniversary, so will either release Friday night, or Monday evening NZ time).

@oleg-nenashev
Copy link
Member Author

oleg-nenashev commented Jan 25, 2018 via email

@kinow
Copy link
Member

kinow commented Jan 25, 2018

Thanks for the quick reply. Had some spare time tonight, enough to cut a new release. Just released it, 3.13 should make the update centre in the next hours I believe.

Let me know if you need anything else, and thanks for working on it 👍
B

@oleg-nenashev
Copy link
Member Author

@kinow Thanks a lot, I will update the JEP-200 status page. If you have some time, review/release of jenkinsci/tap-plugin#20 would be great

@kinow
Copy link
Member

kinow commented Jan 25, 2018

Oh! Was going to forget that one and probably look into that only next week. Merged, preparing a release now :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants