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-30135] Consistent behavior even when the label instance replaced. #3

Merged
merged 2 commits into from Sep 13, 2015

Conversation

Projects
None yet
7 participants
@ikedam
Copy link
Member

commented Aug 30, 2015

JENKINS-30135

Groovy-postbuild-plugin-1.1.0 holds Label instances in GroovyLabelAssignmentAction.
This results in the inconsistent behavior when the label string is once removed from all nodes and added to a node as the Label instance is newly created and no longer same to the one GroovyLabelAssignmentAction holds.

@ikedam

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2015

The problem reproduced like this:
https://jenkins.ci.cloudbees.com/job/plugins/job/groovy-label-assignment-plugin/jp.ikedam.jenkins.plugins$groovy-label-assignment/8/testReport/junit/jp.ikedam.jenkins.plugins.groovy_label_assignment/GroovyLabelAssignmentPropertyJenkinsTest/testLabelIsOnceRemoved/

java.util.concurrent.TimeoutException
    at hudson.remoting.AsyncFutureImpl.get(AsyncFutureImpl.java:85)
    at jp.ikedam.jenkins.plugins.groovy_label_assignment.GroovyLabelAssignmentPropertyJenkinsTest.testLabelIsOnceRemoved(GroovyLabelAssignmentPropertyJenkinsTest.java:644)
@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Aug 30, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@joshshinn

This comment has been minimized.

Copy link

commented Sep 8, 2015

@reviewbybees

This comment has been minimized.

Copy link

commented Sep 8, 2015

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.

@jgbiii

This comment has been minimized.

Copy link

commented Sep 8, 2015

Just so there isn't any confusion we used the reviewbybees simply to have additional folks look into the PR for ACK's. This particular PR WAS NOT originated from a CloudBees employee.

Much gratitude and kudos goes out to @ikedam for this PR!

{
if(label != null)
{
return new GroovyLabelAssignmentAction(label.getExpression());

This comment has been minimized.

Copy link
@tfennelly

tfennelly Sep 9, 2015

Member

I see the use of label here (and above) while label has been @Deprecated. Did you mean to replace these? Maybe call getAssignedLabel?

This comment has been minimized.

Copy link
@ikedam

ikedam Sep 9, 2015

Author Member

I soppose you don't know readResolve
https://wiki.jenkins-ci.org/display/JENKINS/Hint+on+retaining+backward+compatibility

I provide this method to allow users upgrade GroovyLabelAssignmentAction created with older versions of groovy-label-assignment-plugin.
Consequently, readResolve HAVE TO access deprecated fields to retrieve values created in the older version.
That's logical.

readResolve is called by xstream.
I believe xstream call it even private: http://x-stream.github.io/faq.html#Serialization

ikedam added a commit that referenced this pull request Sep 13, 2015

Merge pull request #3 from ikedam/feature/JENKINS-30135_LostLabel
[JENKINS-30135] Consistent behavior even when the label instance replaced.

@ikedam ikedam merged commit 0ec2c9c into jenkinsci:master Sep 13, 2015

1 check passed

Jenkins This pull request looks good
Details
@@ -102,6 +126,15 @@ public Label getAssignedLabel(SubTask task)
*/
public Label getAssignedLabel()
{
return label;
return Jenkins.getInstance().getLabel(getLabelString());

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 13, 2015

Member

NIT (?): Jenkins.getInstance() may return null. Realistic case: somebody uses this method in the new readResolve implementation. I'm not sure what happens during the queue loading

*/
public GroovyLabelAssignmentAction(String labelString)
{
this.labelString = labelString;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 13, 2015

Member

It would be great to trim inputs and to convert empty ones to nulls.

@Deprecated
transient private Label label;

private final String labelString;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 13, 2015

Member

It would be better to define the behavior in the case of null values in the code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.