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

Jump to bottom

[FIXED JENKINS-22665] Fixes for JENKINS-22665 and JENKINS-19755 #64

Merged
merged 1 commit into from Mar 13, 2015
Merged

[FIXED JENKINS-22665] Fixes for JENKINS-22665 and JENKINS-19755 #64

merged 1 commit into from Mar 13, 2015

Conversation

patbos
Copy link

@patbos patbos commented Dec 27, 2014

Remove the serialization of the User object within MyUserIdCause. Kept MyUserIdCause for backwards compatibility but is now just a simple subclass of the core class UserIdCause.
This also fixes the JENKINS-19755 "Manual step "started by" username changes to anonymous after restart" since it now can load the correct user who triggered the build.
After upgrading it is possible to "Manage Old Data" to remove User object serialization of MyUserIdCause.

@jenkinsadmin
Copy link
Member

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

@jglick
Copy link
Member

jglick commented Dec 29, 2014

Continuing to extend UserIdCause but deleting the user field will not work well since the userId field will be null, so legacy build records will incorrectly be shown as having been started by an anonymous user.

I think you need to introduce a String userId2 field, then add a XStream2.PassthruConverter<MyUserIdCause> to replace User user with this new field as well as calling OldDataMonitor.report so that Manage Old Data can be used to save the new format. See the source code for UpstreamCause to see how to do this.

@patbos
Copy link
Author

patbos commented Dec 29, 2014

Thanks for your input. I have done some tests and userId is not null in legacy build records when an authenticated user has started the build. When trying to implement a XStream2.PassthruConverter the User will not have an id to use since id is transient.

@jglick
Copy link
Member

jglick commented Jan 5, 2015

OK, if userId is set in XML then it should work. I would recommend verifying all this with a @LocalData test if you can.

@patbos
Copy link
Author

patbos commented Jan 6, 2015

Added tests for reading old builds and still get the userId and to verify that UserIdCause it used when triggering new build with the manual trigger.

jglick
jglick reviewed Jan 6, 2015
View reviewed changes
...son/plugin/buildpipeline/BuildPipelineViewTest/testMyUserIdCauseConversion/jobs/A/config.xml
@@ -0,0 +1,22 @@
<?xml version='1.0' encoding='UTF-8'?>
Copy link
Member

Choose a reason for hiding this comment

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

BTW you seem not to be using this job in the test so I guess it could be omitted.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, removed it.

@patbos
Copy link
Author

patbos commented Jan 13, 2015

Is this ok to merge now?

@patbos
Copy link
Author

patbos commented Feb 5, 2015

Merged changes from master

KostyaSha
KostyaSha reviewed Mar 11, 2015
View reviewed changes
src/test/java/au/com/centrumsystems/hudson/plugin/buildpipeline/BuildPipelineViewTest.java
import hudson.model.FreeStyleBuild;
import hudson.model.ItemGroup;
import hudson.model.TopLevelItem;
import hudson.model.*;
Copy link
Member

Choose a reason for hiding this comment

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

See no reason for replacing with single line.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@KostyaSha
Copy link
Member

@patbos see my answer in maillist and please join IRC

KostyaSha
KostyaSha reviewed Mar 12, 2015
View reviewed changes
src/test/java/au/com/centrumsystems/hudson/plugin/buildpipeline/BuildPipelineViewTest.java
@@ -341,6 +344,18 @@ public void testHasPermission() throws IOException {
assertTrue(testView.hasPermission(Permission.READ));
}

@Test
@LocalData
@Bug(19755)
Copy link
Member

Choose a reason for hiding this comment

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

@Bug is deprecated, replace with @Issue if core version allows it

Copy link
Author

Choose a reason for hiding this comment

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

Core version is to low, does not contain the new annotation.

Copy link
Member

Choose a reason for hiding this comment

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

wow..

Copy link
Member

Choose a reason for hiding this comment

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

Yes, appeared only from test-annotations-1.1 :(

@KostyaSha
Copy link
Member

And please reword commit to [FIXED JENKINS-22665] description :)

@patbos patbos changed the title Fixes for JENKINS-22665 and JENKINS-19755 [FIXED JENKINS-22665] Fixes for JENKINS-22665 and JENKINS-19755 Mar 12, 2015
@KostyaSha
Copy link
Member

I mean git commit :) Then it will automatically close jira issue after merge :P

KostyaSha
KostyaSha reviewed Mar 12, 2015
View reviewed changes
src/main/java/au/com/centrumsystems/hudson/plugin/buildpipeline/BuildPipelineView.java
return hashCode() == o.hashCode();
}
@Deprecated
private static class MyUserIdCause extends Cause.UserIdCause {

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line...

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@patbos
[FIXED JENKINS-22665] [FIXED JENKINS-19755] Changed MyUserIdCause to …
bd77518 
…not include the whole User object serialized.
@patbos
Copy link
Author

patbos commented Mar 12, 2015

Fixed the commit description :)

@KostyaSha KostyaSha self-assigned this Mar 13, 2015
@KostyaSha KostyaSha mentioned this pull request Mar 13, 2015
Closed
@KostyaSha
Copy link
Member

👍

KostyaSha added a commit that referenced this pull request Mar 13, 2015
@KostyaSha
Merge pull request #64 from patbos/JENKINS-22665
7e03b73 
[FIXED JENKINS-22665] Fixes for JENKINS-22665 and JENKINS-19755
@KostyaSha KostyaSha merged commit 7e03b73 into jenkinsci:master Mar 13, 2015
@daniel-beck
Copy link
Member

Caused JENKINS-27748 (with some assistance by Xstream).

@KostyaSha KostyaSha mentioned this pull request Apr 28, 2015
Closed
@dserodio
Copy link

dserodio commented Oct 8, 2015

Forgive me for digging up this PR, but is the "converter who converts all au.com.centrumsystems.hudson.plugin.buildpipeline.BuildPipelineView$MyUserIdCause to hudson.model.Cause$UserIdCause in the build.xmls" implemented? I'm still seeing problems after upgrading from 1.4.3 to 1.4.8 and I'm fairly sure it's caused by this.

@KostyaSha
Copy link
Member

You can't update this crappy deserialized xml as it can't load back at all. Just wrote simple script that will physically remove this section from build.xml.

@dserodio
Copy link

dserodio commented Oct 8, 2015

Thanks @KostyaSha should I rip the entire "MyUserIdCause" tag? Do you by any chance have a script ready? :)

@KostyaSha
Copy link
Member

@dserodio unfortunately i have no script. I suggested it to one of our customers and they resolved this issue. Afair it should be enough to cut one tag (don't remember, probably triple* something).

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

@KostyaSha KostyaSha

Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@patbos @jenkinsadmin @jglick @KostyaSha @daniel-beck @dserodio