-
Notifications
You must be signed in to change notification settings - Fork 77
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
[JENKINS-48924, JENKINS-48995] - Fix plugin compatibility with Jenkins 2.102+ #29
Conversation
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 { |
There was a problem hiding this comment.
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
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. |
I currently cannot say whether it work on agents actually, the autotests do not seem to cover that |
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; |
There was a problem hiding this comment.
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.
@@ -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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should be volatile
.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe should be volatile
.
@kinow I noticed the plugin is up for adoption. Do you have time to review/release it or should we proceed on our own? |
@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 |
Yes, I definitely have no capacity to maintain new plugins, sorry. Was
about marking about 15 of my plugins for adoption. If you could just spin a
release, it would be great.
…On Jan 24, 2018 21:32, "Bruno P. Kinoshita" ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKBqVHABcBoAx5g7utebHc74MrqNzzO5ks5tN5NLgaJpZM4RgHiS>
.
--
You received this message because you are subscribed to the Google Groups
"engineering-code-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to ***@***.***
To post to this group, send email to engineering-code-reviews@
cloudbees.com.
To view this discussion on the web visit https://groups.google.com/a/
cloudbees.com/d/msgid/engineering-code-reviews/jenkinsci/testlink-plugin/
pull/29/review/91320432%40github.com
<https://groups.google.com/a/cloudbees.com/d/msgid/engineering-code-reviews/jenkinsci/testlink-plugin/pull/29/review/91320432%40github.com?utm_medium=email&utm_source=footer>
.
|
Sorry, responded from wrong email
On Jan 24, 2018 23:26, "Code Reviewed by CloudBees" <
notifications@github.com> wrote:
… Yes, I definitely have no capacity to maintain new plugins, sorry. Was
about marking about 15 of my plugins for adoption. If you could just spin a
release, it would be great.
On Jan 24, 2018 21:32, "Bruno P. Kinoshita" ***@***.***>
wrote:
> ***@***.**** approved this pull request.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#29#
pullrequestreview-91320432>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/
AKBqVHABcBoAx5g7utebHc74MrqNzzO5ks5tN5NLgaJpZM4RgHiS>
> .
>
> --
> You received this message because you are subscribed to the Google Groups
> "engineering-code-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to ***@***.***
> To post to this group, send email to engineering-code-reviews@
> cloudbees.com.
> To view this discussion on the web visit https://groups.google.com/a/
> cloudbees.com/d/msgid/engineering-code-reviews/
jenkinsci/testlink-plugin/
> pull/29/review/91320432%40github.com
> <https://groups.google.com/a/cloudbees.com/d/msgid/
engineering-code-reviews/jenkinsci/testlink-plugin/
pull/29/review/91320432%40github.com?utm_medium=email&utm_source=footer>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AC3IoGlv7cKmgwKY7sqBorkHeSXIbjR6ks5tN64CgaJpZM4RgHiS>
.
|
Sure, works for me @oleg-nenashev. What about the #25? Is that required as well for a release? |
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). |
I re ommend to release without #25, it needs more reviews imo
On Jan 25, 2018 01:09, "Bruno P. Kinoshita" <notifications@github.com> wrote:
Will cut a release as is (and plus #25
<#25> if @oleg-nenashev
<https://github.com/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).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AC3IoJ57BRXq-fmQ03qaGLA4ScFOXVk3ks5tN8ZLgaJpZM4RgHiS>
.
|
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 👍 |
@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 |
Oh! Was going to forget that one and probably look into that only next week. Merged, preparing a release now :-) |
https://issues.jenkins-ci.org/browse/JENKINS-48924
https://issues.jenkins-ci.org/browse/JENKINS-48995
@reviewbybees @jglick